Re: [PATCH 2/2] fanotify: introduce FAN_MARK_IGNORE

[Jan Kara <jack@suse.cz>]

On Mon 20-06-22 16:45:51, Amir Goldstein wrote:
> This flag is a new way to configure ignore mask which allows adding and
> removing the event flags FAN_ONDIR and FAN_EVENT_ON_CHILD in ignore mask.
> 
> The legacy FAN_MARK_IGNORED_MASK flag would always ignore events on
> directories and would ignore events on children depending on whether
> the FAN_EVENT_ON_CHILD flag was set in the (non ignored) mask.
> 
> FAN_MARK_IGNORE can be used to ignore events on children without setting
> FAN_EVENT_ON_CHILD in the mark's mask and will not ignore events on
> directories unconditionally, only when FAN_ONDIR is set in ignore mask.
> 
> The new behavior is sticky.  After calling fanotify_mark() with
> FAN_MARK_IGNORE once, calling fanotify_mark() with FAN_MARK_IGNORED_MASK
> will update the ignore mask, but will not change the event flags in
> ignore mask nor how these flags are treated.

IMHO this stickyness is not very obvious. Wouldn't it be less error-prone
for users to say that once FAN_MARK_IGNORE is used for a mark, all
subsequent modifications of ignore mask have to use FAN_MARK_IGNORE? I mean
if some program bothers with FAN_MARK_IGNORE, I'd expect it to use it for
all its calls as otherwise the mixup is kind of difficult to reason
about...

Also it follows the behavior we have picked for FAN_MARK_EVICTABLE AFAIR
but that's not really important to me.

> @@ -1591,10 +1601,20 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
>  
>  	/*
>  	 * Event flags (FAN_ONDIR, FAN_EVENT_ON_CHILD) have no effect with
> -	 * FAN_MARK_IGNORED_MASK.
> +	 * FAN_MARK_IGNORED_MASK.  They can be updated in ignore mask with
> +	 * FAN_MARK_IGNORE and then they do take effect.
>  	 */
> -	if (ignore)
> +	switch (ignore) {
> +	case 0:
> +	case FAN_MARK_IGNORE:
> +		break;
> +	case FAN_MARK_IGNORED_MASK:
>  		mask &= ~FANOTIFY_EVENT_FLAGS;
> +		umask = FANOTIFY_EVENT_FLAGS;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

I think this would be easier to follow as two ifs:

	/* We don't allow FAN_MARK_IGNORE & FAN_MARK_IGNORED_MASK together */
	if (ignore == FAN_MARK_IGNORE | FAN_MARK_IGNORED_MASK)
		return -EINVAL;
	/*
	 * Event flags (FAN_ONDIR, FAN_EVENT_ON_CHILD) have no effect with
	 * FAN_MARK_IGNORED_MASK.
	 */
	if (ignore == FAN_MARK_IGNORED_MASK) {
  		mask &= ~FANOTIFY_EVENT_FLAGS;
		umask = FANOTIFY_EVENT_FLAGS;
	}

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR