On Mon 20-06-22 16:45:50, Amir Goldstein wrote:
> Setting flags FAN_ONDIR FAN_EVENT_ON_CHILD in ignore mask has no effect.
> The FAN_EVENT_ON_CHILD flag in mask implicitly applies to ignore mask and
> ignore mask is always implicitly applied to events on directories.
>
> Define a mark flag that replaces this legacy behavior with logic of
> applying the ignore mask according to event flags in ignore mask.
>
> Implement the new logic to prepare for supporting an ignore mask that
> ignores events on children and ignore mask that does not ignore events
> on directories.
>
> To emphasize the change in terminology, also rename ignored_mask mark
> member to ignore_mask and use accessor to get only ignored events or
> events and flags.
>
> This change in terminology finally aligns with the "ignore mask"
> language in man pages and in most of the comments.
>
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
Looks mostly good to me. Just one question / suggestion: You are
introducing helpers fsnotify_ignore_mask() and fsnotify_ignored_events().
So shouldn't we be using these helpers as much as possible throughout the
code? Because in several places I had to check the code around whether
using mark->ignore_mask directly is actually fine. In particular:
> @@ -315,19 +316,23 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> return 0;
> } else if (!(fid_mode & FAN_REPORT_FID)) {
> /* Do we have a directory inode to report? */
> - if (!dir && !(event_mask & FS_ISDIR))
> + if (!dir && !ondir)
> return 0;
> }
>
> fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
> - /* Apply ignore mask regardless of mark's ISDIR flag */
> - marks_ignored_mask |= mark->ignored_mask;
> + /*
> + * Apply ignore mask depending on whether FAN_ONDIR flag in
> + * ignore mask should be checked to ignore events on dirs.
> + */
> + if (!ondir || fsnotify_ignore_mask(mark) & FAN_ONDIR)
> + marks_ignore_mask |= mark->ignore_mask;
>
> /*
> * If the event is on dir and this mark doesn't care about
> * events on dir, don't send it!
> */
> - if (event_mask & FS_ISDIR && !(mark->mask & FS_ISDIR))
> + if (ondir && !(mark->mask & FAN_ONDIR))
> continue;
>
> marks_mask |= mark->mask;
So for example here I'm wondering whether a helper should not be used...
> @@ -336,7 +341,7 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group,
> *match_mask |= 1U << type;
> }
>
> - test_mask = event_mask & marks_mask & ~marks_ignored_mask;
> + test_mask = event_mask & marks_mask & ~marks_ignore_mask;
Especially because here if say FAN_EVENT_ON_CHILD becomes a part of
marks_ignore_mask it can result in clearing this flag in the returned
'mask' which is likely not what we want if there are some events left
unignored in the 'mask'?
> @@ -344,14 +344,16 @@ static int send_to_group(__u32 mask, const void *data, int data_type,
> fsnotify_foreach_iter_mark_type(iter_info, mark, type) {
> group = mark->group;
> marks_mask |= mark->mask;
> - marks_ignored_mask |= mark->ignored_mask;
> + if (!(mask & FS_ISDIR) ||
> + (fsnotify_ignore_mask(mark) & FS_ISDIR))
> + marks_ignore_mask |= mark->ignore_mask;
> }
>
> - pr_debug("%s: group=%p mask=%x marks_mask=%x marks_ignored_mask=%x data=%p data_type=%d dir=%p cookie=%d\n",
> - __func__, group, mask, marks_mask, marks_ignored_mask,
> + pr_debug("%s: group=%p mask=%x marks_mask=%x marks_ignore_mask=%x data=%p data_type=%d dir=%p cookie=%d\n",
> + __func__, group, mask, marks_mask, marks_ignore_mask,
> data, data_type, dir, cookie);
>
> - if (!(test_mask & marks_mask & ~marks_ignored_mask))
> + if (!(test_mask & marks_mask & ~marks_ignore_mask))
> return 0;
And I'm wondering about similar things here...
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
