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

[Cyril Hrubis <chrubis@suse.cz>]

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
curly braces around the block. And the same for the rest of the
invocations.

>  			/* 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.

> +			return NULL;
> +		}
> +		event_buf_len = ret;
> +	}
> +	if (event_buf_len - event_buf_pos < (int)FAN_EVENT_METADATA_LEN) {
> +		tst_brk(TBROK,
> +			"too short event when reading fanotify events (%d < %d)",
> +			event_buf_len - event_buf_pos,
> +			(int)FAN_EVENT_METADATA_LEN);
> +		*retp = -1;
> +		return NULL;
> +	}
> +	event = (struct fanotify_event_metadata *)(event_buf + event_buf_pos);
> +	event_buf_pos += event->event_len;
> +	return event;
> +}
> +
>  static void test_fanotify(unsigned int n)
>  {
>  	struct tcase *tc = &tcases[n];
> @@ -672,6 +756,7 @@ static void test_fanotify(unsigned int n)
>  	int ret;
>  	unsigned int p, i;
>  	struct fanotify_event_metadata *event;
> +	int event_count;
>  
>  	tst_res(TINFO, "Test #%d: %s", n, tc->tname);
>  
> @@ -715,7 +800,7 @@ static void test_fanotify(unsigned int n)
>  	ignore_mark = &fanotify_mark_types[tc->ignore_mark_type];
>  
>  	/* Generate event in child process */
> -	if (!generate_event(tc->event_path, tc->expected_mask_with_ignore))
> +	if (!generate_event(tc, tc->expected_mask_with_ignore))
>  		tst_brk(TBROK, "Child process terminated incorrectly");
>  
>  	/* First verify all groups without matching ignore mask got the event */
> @@ -724,64 +809,52 @@ static void test_fanotify(unsigned int n)
>  			break;
>  
>  		for (i = 0; i < GROUPS_PER_PRIO; i++) {
> -			ret = read(fd_notify[p][i], event_buf, EVENT_BUF_LEN);
> -			if (ret < 0) {
> -				if (errno == EAGAIN) {
> -					tst_res(TFAIL, "group %d (%x) "
> -						"with %s did not get event",
> -						i, fanotify_class[p], mark->name);
> -					continue;
> -				}
> -				tst_brk(TBROK | TERRNO,
> -					"reading fanotify events failed");
> -			}
> -			if (ret < (int)FAN_EVENT_METADATA_LEN) {
> -				tst_brk(TBROK,
> -					"short read when reading fanotify "
> -					"events (%d < %d)", ret,
> -					(int)EVENT_BUF_LEN);
> +			event_count = 0;
> +			event_buf_pos = event_buf_len = 0;
> +			while ((event = fetch_event(fd_notify[p][i], &ret))) {
> +				event_count++;
> +				if (!verify_event(p, i, event, p == 0 ?
> +						tc->expected_mask_without_ignore :
> +						tc->expected_mask_with_ignore))
> +					break;
> +				if (event->fd != FAN_NOFD)
> +					SAFE_CLOSE(event->fd);
>  			}
> -			event = (struct fanotify_event_metadata *)event_buf;
> -			if (ret > (int)event->event_len) {
> +			if (ret < 0)
> +				continue;
> +			if (event_count != (tc->event_path_cnt ? : 1)) {
>  				tst_res(TFAIL, "group %d (%x) with %s "
> -					"got more than one event (%d > %d)",
> -					i, fanotify_class[p], mark->name, ret,
> -					event->event_len);
> +					"got unexpected number of events (%d != %d)",
> +					i, fanotify_class[p], mark->name,
> +					event_count, tc->event_path_cnt);
>  			} else {
> -				verify_event(p, i, event, p == 0 ?
> -						tc->expected_mask_without_ignore :
> -						tc->expected_mask_with_ignore);
> +				tst_res(TPASS, "group %d (%x) got %d events: mask %llx pid=%u",
> +					i, fanotify_class[p], event_count,
> +					(unsigned long long)(p == 0 ?
> +					tc->expected_mask_without_ignore :
> +					tc->expected_mask_with_ignore),
> +					(unsigned int)child_pid);
>  			}
> -			if (event->fd != FAN_NOFD)
> -				SAFE_CLOSE(event->fd);
>  		}
>  	}
>  	/* Then verify all groups with matching ignore mask did got the event */
>  	for (p = 1; p < num_classes && !tc->expected_mask_with_ignore; p++) {
>  		for (i = 0; i < GROUPS_PER_PRIO; i++) {
> -			ret = read(fd_notify[p][i], event_buf, EVENT_BUF_LEN);
> -			if (ret >= 0 && ret < (int)FAN_EVENT_METADATA_LEN) {
> -				tst_brk(TBROK,
> -					"short read when reading fanotify "
> -					"events (%d < %d)", ret,
> -					(int)EVENT_BUF_LEN);
> -			}
> -			event = (struct fanotify_event_metadata *)event_buf;
> -			if (ret > 0) {
> -				tst_res(TFAIL, "group %d (%x) with %s and "
> -					"%s ignore mask got unexpected event (mask %llx)",
> -					i, fanotify_class[p], mark->name, ignore_mark->name,
> -					event->mask);
> +			event_count = 0;
> +			event_buf_pos = event_buf_len = 0;
> +			while ((event = fetch_event(fd_notify[p][i], &ret))) {
> +				event_count++;
>  				if (event->fd != FAN_NOFD)
>  					SAFE_CLOSE(event->fd);
> -			} else if (errno == EAGAIN) {
> -				tst_res(TPASS, "group %d (%x) with %s and "
> -					"%s ignore mask got no event",
> -					i, fanotify_class[p], mark->name, ignore_mark->name);
> -			} else {
> -				tst_brk(TBROK | TERRNO,
> -					"reading fanotify events failed");
>  			}
> +			if (ret < 0)
> +				continue;
> +			if (event_count > tc->event_path_cnt / 2)
> +				tst_res(TFAIL, "group %d (%x) with %s and "
> +					"%s ignore mask got unexpectedly many events (%d > %d)",
> +					i, fanotify_class[p], mark->name,
> +					ignore_mark->name, event_count,
> +					tc->event_path_cnt / 2);

Apart from the two minor issues I pointed out the rest looks good to me,
but honestly the code is getting way to complicated I would refrain from
adding anything else into the test without some refactoring.

-- 
Cyril Hrubis
chrubis@suse.cz

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