[LTP] [PATCH v3] Rewrite fcnt14 test

[Andrea Cervesato]

Hi!

On 8/30/24 15:58, Cyril Hrubis wrote:
> 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.
We can make it.
>
>> +#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?
I don't know, is a silent version of EQ_LI really needed? It's a 
particular use case, where also *FAILED is set, so I don't know...
>
>> +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;
>>   }
>>   
Andrea