Re: [LTP] [PATCH 2/3] fanotify10: Add support for multiple event files

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>>  static void drop_caches(void)
>> @@ -482,6 +515,7 @@ static int create_fanotify_groups(unsigned int n)
>>  	int evictable_ignored = (tc->ignore_mark_type == FANOTIFY_EVICTABLE);
>>  	int ignore_mark_type;
>>  	int ignored_onchild = tc->ignored_flags & FAN_EVENT_ON_CHILD;
>> +	char path[PATH_MAX];
>>  
>>  	mark = &fanotify_mark_types[tc->mark_type];
>>  	ignore_mark = &fanotify_mark_types[tc->ignore_mark_type];
>> @@ -501,11 +535,12 @@ static int create_fanotify_groups(unsigned int n)
>>  			 * FAN_EVENT_ON_CHILD has no effect on filesystem/mount
>>  			 * or inode mark on non-directory.
>>  			 */
>> -			SAFE_FANOTIFY_MARK(fd_notify[p][i],
>> +			foreach_path(tc, path, mark_path)
>> +				SAFE_FANOTIFY_MARK(fd_notify[p][i],
>>  					    FAN_MARK_ADD | mark->flag,
>>  					    tc->expected_mask_without_ignore |
>>  					    FAN_EVENT_ON_CHILD | FAN_ONDIR,
>> -					    AT_FDCWD, tc->mark_path);
>> +					    AT_FDCWD, path);
>
> This is minor, but I would have named the macro FOREACH_PATH() and
> added

This is actually not in line with the kernel style. At least IIRC
foreach macros are lower case in the instances I have seen. This is also
what we have in tst_cgroup.c for e.g.

> curly braces around the block. And the same for the rest of the
> invocations.

At some point checkpatch stopped complaining about this, so it's now the
authors discretion whether to use curly braces in these cases. Unless
there is something wrong with our checkpatch.

IMO there is no mistaking that it is a loop macro with one function call
in the body.

>
>>  			/* Do not add ignore mark for first priority groups */
>>  			if (p == 0)
>> @@ -519,9 +554,10 @@ static int create_fanotify_groups(unsigned int n)
>>  			mark_ignored = tst_variant ? FAN_MARK_IGNORE_SURV : FAN_MARK_IGNORED_SURV;
>>  			mask = FAN_OPEN | tc->ignored_flags;
>>  add_mark:
>> -			SAFE_FANOTIFY_MARK(fd_notify[p][i],
>> +			foreach_path(tc, path, ignore_path)
>> +				SAFE_FANOTIFY_MARK(fd_notify[p][i],
>>  					    FAN_MARK_ADD | ignore_mark->flag | mark_ignored,
>> -					    mask, AT_FDCWD, tc->ignore_path);
>> +					    mask, AT_FDCWD, path);
>>  
>>  			/*
>>  			 * FAN_MARK_IGNORE respects FAN_EVENT_ON_CHILD flag, but legacy
>> @@ -578,9 +614,24 @@ add_mark:
>>  	if (ignore_mark_type == FAN_MARK_INODE) {
>>  		for (p = 0; p < num_classes; p++) {
>>  			for (i = 0; i < GROUPS_PER_PRIO; i++) {
>> -				if (fd_notify[p][i] > 0)
>> +				if (fd_notify[p][i] > 0) {
>> +					int minexp, maxexp;
>> +
>> +					if (p == 0) {
>> +						minexp = maxexp = 0;
>> +					} else if (evictable_ignored) {
>> +						minexp = 0;
>> +						/*
>> +						 * Check at least half the
>> +						 * marks get evicted by reclaim
>> +						 */
>> +						maxexp = tc->ignore_path_cnt / 2;
>> +					} else {
>> +						minexp = maxexp = tc->ignore_path_cnt ? : 1;
>> +					}
>>  					show_fanotify_ignore_marks(fd_notify[p][i],
>> -								   p > 0 && !evictable_ignored);
>> +								   minexp, maxexp);
>> +				}
>>  			}
>>  		}
>>  	}
>> @@ -613,7 +664,7 @@ static void mount_cycle(void)
>>  	bind_mount_created = 1;
>>  }
>>  
>> -static void verify_event(int p, int group, struct fanotify_event_metadata *event,
>> +static int verify_event(int p, int group, struct fanotify_event_metadata *event,
>>  			 unsigned long long expected_mask)
>>  {
>>  	/* Only FAN_REPORT_FID reports the FAN_ONDIR flag in events on dirs */
>> @@ -626,33 +677,35 @@ static void verify_event(int p, int group, struct fanotify_event_metadata *event
>>  			(unsigned long long) event->mask,
>>  			(unsigned long long) expected_mask,
>>  			(unsigned int)event->pid, event->fd);
>> +		return 0;
>>  	} else if (event->pid != child_pid) {
>>  		tst_res(TFAIL, "group %d (%x) got event: mask %llx pid=%u "
>>  			"(expected %u) fd=%u", group, fanotify_class[p],
>>  			(unsigned long long)event->mask, (unsigned int)event->pid,
>>  			(unsigned int)child_pid, event->fd);
>> -	} else {
>> -		tst_res(TPASS, "group %d (%x) got event: mask %llx pid=%u fd=%u",
>> -			group, fanotify_class[p], (unsigned long long)event->mask,
>> -			(unsigned int)event->pid, event->fd);
>> +		return 0;
>>  	}
>> +	return 1;
>>  }
>>  
>> -static int generate_event(const char *event_path,
>> -			  unsigned long long expected_mask)
>> +static int generate_event(struct tcase *tc, unsigned long long expected_mask)
>>  {
>>  	int fd, status;
>>  
>>  	child_pid = SAFE_FORK();
>>  
>>  	if (child_pid == 0) {
>> -		if (expected_mask & FAN_OPEN_EXEC) {
>> -			SAFE_EXECL(event_path, event_path, NULL);
>> -		} else {
>> -			fd = SAFE_OPEN(event_path, O_RDONLY);
>> +		char path[PATH_MAX];
>> +
>> +		foreach_path(tc, path, event_path) {
>> +			if (expected_mask & FAN_OPEN_EXEC) {
>> +				SAFE_EXECL(path, path, NULL);
>> +			} else {
>> +				fd = SAFE_OPEN(path, O_RDONLY);
>>  
>> -			if (fd > 0)
>> -				SAFE_CLOSE(fd);
>> +				if (fd > 0)
>> +					SAFE_CLOSE(fd);
>> +			}
>>  		}
>>  
>>  		exit(0);
>> @@ -665,6 +718,37 @@ static int generate_event(const char *event_path,
>>  	return 0;
>>  }
>>  
>> +struct fanotify_event_metadata *fetch_event(int fd, int *retp)
>> +{
>> +	int ret;
>> +	struct fanotify_event_metadata *event;
>> +
>> +	*retp = 0;
>> +	if (event_buf_pos >= event_buf_len) {
>> +		event_buf_pos = 0;
>> +		ret = read(fd, event_buf, EVENT_BUF_LEN);
>> +		if (ret < 0) {
>> +			if (errno == EAGAIN)
>> +				return NULL;
>> +			tst_brk(TBROK | TERRNO, "reading fanotify events failed");
>> +			*retp = -1;
>
> If you call tst_brk(TBROK ...) the test exists since that is supposed to
> signal unrecoverable error. There is no need to propagate the retp from
> this function.

Yes although it's confusing for static analysis becuase tst_brk can
return in cleanup code. Also this function needs to be static. So I'll
do this before merge.

Will merge with gnu99 patch, after CI completes, thanks!

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp