[LTP] [PATCH v3] Rewrite fcnt14 test

[Cyril Hrubis]

Hi!
> +fcntl14_01 fcntl14
> +fcntl14_01_64 fcntl14_64
> +fcntl14_02 fcntl14 -l
> +fcntl14_02_64 fcntl14_64 -l

I would slightly prefer having a .test_variants than adding a command
line options.

> +#define CHECK_EQ_SILENT_(VAL_A, SVAL_A, VAL_B, SVAL_B, TYPE, PFS, FAILED) do {\
> +	TYPE tst_tmp_a__ = VAL_A; \
> +	TYPE tst_tmp_b__ = VAL_B; \
> +	if (tst_tmp_a__ != tst_tmp_b__) { \
> +		tst_res(TFAIL, \
> +			SVAL_A " == " SVAL_B " (" PFS ")", tst_tmp_a__); \
> +			*FAILED = 1; \
> +	} \
> +} while (0)
> +
> +#define CHECK_EQ_SILENT(VAL_A, VAL_B, FAILED) \
> +		CHECK_EQ_SILENT_(VAL_A, #VAL_A, VAL_B, #VAL_B, long long, "%lli", FAILED)

Maybe this should go to the tst_test_macros.h?

> +struct file_conf {
> +	short type;
> +	short whence;
> +	long start;
> +	long len;
>  };
>  
> -static testcase *thiscase;
> +struct testcase {
> +	struct file_conf parent;           /* parent parameters for fcntl() */
> +	struct file_conf child;            /* child parameters for fcntl() */
> +	short blocking;                    /* blocking/non-blocking flag */
> +	long pos;                          /* starting file position */
> +};
> +
> +struct tc_results {
> +	int num_pass;
> +	int last_failed;
> +};
> +
> +static const char filepath[] = "unlocked.txt";
> +static const char filedata[] = "Here some bytes!";
>  static struct flock flock;
> -static int parent, child, status, fail = 0;
> -static int got1 = 0;
> -static int fd;
> -static int test;
> -static char tmpname[40];
> +static char *str_op_nums;
> +static char *locking_file;
> +static int op_nums = 5000;
> +static int file_mode = 0777;
> +static struct tc_results *results;
>  
> -#define FILEDATA	"ten bytes!"
> +static void dochild(struct testcase *tc, const int fd, const pid_t parent_pid)
> +{
> +	results->last_failed = 0;
>  
> -void catch1(int sig);
> -void catch_alarm(int sig);
> +	flock.l_type = tc->child.type;
> +	flock.l_whence = tc->child.whence;
> +	flock.l_start = tc->child.start;
> +	flock.l_len = tc->child.len;
> +	flock.l_pid = 0;
>  
> -char *TCID = "fcntl14";
> -int TST_TOTAL = 1;
> -int NO_NFS = 1;
> +	SAFE_FCNTL(fd, F_GETLK, &flock);
>  
> -void cleanup(void)
> -{
> -	tst_rmdir();
> -}
> +	if (tc->blocking) {
> +		tst_res(TDEBUG, "Child: expecting blocked file by parent");
>  
> -void setup(void)
> -{
> -	struct sigaction act;
> -
> -	tst_sig(FORK, DEF_HANDLER, cleanup);
> -	signal(SIGHUP, SIG_IGN);
> -	umask(0);
> -	TEST_PAUSE;
> -	tst_tmpdir();
> -	parent = getpid();
> -
> -	sprintf(tmpname, "fcntl2.%d", parent);
> -
> -	/* setup signal handler for signal from child */
> -	memset(&act, 0, sizeof(act));
> -	act.sa_handler = catch1;
> -	sigemptyset(&act.sa_mask);
> -	sigaddset(&act.sa_mask, SIGUSR1);
> -	if ((sigaction(SIGUSR1, &act, NULL)) < 0) {
> -		tst_resm(TFAIL, "SIGUSR1 signal setup failed, errno = %d",
> -			 errno);
> -		cleanup();
> -	}
> +		CHECK_EQ_SILENT(flock.l_pid, parent_pid, &results->last_failed);
> +		if (results->last_failed)
> +			return;
>  
> -	memset(&act, 0, sizeof(act));
> -	act.sa_handler = catch_alarm;
> -	sigemptyset(&act.sa_mask);
> -	sigaddset(&act.sa_mask, SIGALRM);
> -	if ((sigaction(SIGALRM, &act, NULL)) < 0) {
> -		tst_resm(TFAIL, "SIGALRM signal setup failed");
> -		cleanup();
> -	}
> -}
> +		CHECK_EQ_SILENT(flock.l_type, tc->parent.type, &results->last_failed);
> +		if (results->last_failed)
> +			return;
>  
> -void wake_parent(void)
> -{
> -	if ((kill(parent, SIGUSR1)) < 0) {
> -		tst_resm(TFAIL, "Attempt to send signal to parent " "failed");
> -		tst_resm(TFAIL, "Test case %d, errno = %d", test + 1, errno);
> -		fail = 1;
> +		flock.l_type = tc->child.type;
> +		flock.l_whence = tc->child.whence;
> +		flock.l_start = tc->child.start;
> +		flock.l_len = tc->child.len;
> +		flock.l_pid = 0;
> +
> +		TST_EXP_FAIL_SILENT(fcntl(fd, F_SETLK, &flock), EWOULDBLOCK);
> +		if (TST_RET != -1)
> +			results->last_failed = 1;
> +	} else {
> +		tst_res(TDEBUG, "Child: expecting no blocking errors");
> +
> +		CHECK_EQ_SILENT(flock.l_type, F_UNLCK, &results->last_failed);
> +		if (results->last_failed)
> +			return;
> +
> +		CHECK_EQ_SILENT(flock.l_whence, tc->child.whence, &results->last_failed);
> +		if (results->last_failed)
> +			return;
> +
> +		CHECK_EQ_SILENT(flock.l_start, tc->child.start, &results->last_failed);
> +		if (results->last_failed)
> +			return;
> +
> +		CHECK_EQ_SILENT(flock.l_len, tc->child.len, &results->last_failed);
> +		if (results->last_failed)
> +			return;
> +
> +		CHECK_EQ_SILENT(flock.l_pid, 0, &results->last_failed);
> +		if (results->last_failed)
> +			return;

Why do we have to exit after each failed check here? I would just a
bunch of the checks here without the returns. All that we need is to
make sure the last_failed is set if child fails so that parent can
detect it.


> +	/* set the initial parent lock on the file */
> +	flock.l_type = tc->parent.type;
> +	flock.l_whence = tc->parent.whence;
> +	flock.l_start = tc->parent.start;
> +	flock.l_len = tc->parent.len;
>  	flock.l_pid = 0;

I wonder if it would be better to use struct flock in the tc directory
so that we could just assign the structure here. I.e. we would do here
just:

	struct flock flock = tc->flock;

	flock.l_pid = 0;


> +	flock.l_type = F_UNLCK;
> +	flock.l_whence = 0;
> +	flock.l_start = 0;
> +	flock.l_len = 0;
>  	flock.l_pid = 0;
>  
> +	SAFE_FCNTL(fd, F_SETLK, &flock);
> +	SAFE_CLOSE(fd);

Why bother with unlocking? Aren't the locks dropped when we close the fd
here anyways?

> +}
> +
> +static void genconf(struct file_conf *conf, const int size, const long pos)
> +{
> +	conf->type = rand() % 2 ? F_RDLCK : F_WRLCK;
> +	conf->whence = SEEK_CUR;
> +
> +	if (pos > 0 && (rand() % 2)) {
> +		conf->start = -(rand() % pos);
> +		conf->len = rand() % (size + conf->start - 1) + 1;
>  	} else {
> -		exit(0);
> +		conf->start = rand() % (size - 1);
> +		conf->len = rand() % (size - conf->start - 1) + 1;
>  	}
>  }

We have a file in which we seek at pos offset and then need a start
relative to that and lenght so that it fits into the file.

The start should be just the whole available range, which is random
offset moved back by pos. Then we need a size which is limited to
whatever is left in the file and is not relative. So if I'm not mistaken
we need what is in the else branch but we have to move the start by pos
at the end, i.e. the whole start and len generation should look like:

	conf->start = rand() % (size - 1);
	conf->len = rand() % (size - conf->start - 1) + 1;
	conf->start -= pos;

That way we generate a valid part of the file and make it realtive to
the pos at the end.

> -void run_test(int file_flag, int file_mode, int seek, int start, int end)
> +static short fcntl_overlap(
> +	struct file_conf *parent,
> +	struct file_conf *child,
> +	const long pos)
>  {
> +	long start[2];
> +	long length[2];
> +	short overlap = 0;
> +
> +	if (parent->start > child->start) {
> +		start[0] = pos + child->start;
> +		start[1] = pos + parent->start;
> +		length[0] = child->len;
> +		length[1] = parent->len;
> +	} else {
> +		start[0] = pos + parent->start;
> +		start[1] = pos + child->start;
> +		length[0] = parent->len;
> +		length[1] = child->len;
>  	}

If you add pos to both of the starts you just move the two ranges by the
same amount. So the end result is the same regardless of if you add it
or not.

> +
> +	overlap = start[0] <= start[1] && start[1] < (start[0] + length[0]);

Isn't the start[0] <= start[1] because we made it so? We set the
start[0] to the smaller of the two. So all we need to do is to check
that that start[0] + lenght[0] does not end before the start[1] which is
the second part of the equation.

And we do not use lenght[1] either. So maybe we just need:

	if (child->start < parent->start)
		overlap = parent->start < (child->start + child->len);
	else
		overlap = child->start < (parent->start + parent->len);

> +	if (overlap)
> +		tst_res(TDEBUG, "child/parent fcntl() configurations overlap");
> +
> +	return overlap;
>  }
>  

-- 
Cyril Hrubis
chrubis@suse.cz