On Wed, May 11, 2022 at 3:54 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Wed 11-05-22 12:29:13, Amir Goldstein wrote:
> > fsnotify_foreach_iter_mark_type() is used to reduce boilerplate code
> > of iteratating all marks of a specific group interested in an event
> > by consulting the iterator report_mask.
> >
> > Use an open coded version of that iterator in fsnotify_iter_next()
> > that collects all marks of the current iteration group without
> > consulting the iterator report_mask.
> >
> > At the moment, the two iterator variants are the same, but this
> > decoupling will allow us to exclude some of the group's marks from
> > reporting the event, for example for event on child and inode marks
> > on parent did not request to watch events on children.
> >
> > Fixes: 2f02fd3fa13e ("fanotify: fix ignore mask logic for events on child and on dir")
> > Reported-by: Jan Kara <jack@xxxxxxxx>
> > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
>
> Mostly looks good. Two nits below.
>
> > /*
> > - * Pop from iter_info multi head queue, the marks that were iterated in the
> > + * Pop from iter_info multi head queue, the marks that belong to the group of
> > * current iteration step.
> > */
> > static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
> > {
> > + struct fsnotify_mark *mark;
> > int type;
> >
> > fsnotify_foreach_iter_type(type) {
> > - if (fsnotify_iter_should_report_type(iter_info, type))
> > + mark = iter_info->marks[type];
> > + if (mark && mark->group == iter_info->current_group)
> > iter_info->marks[type] =
> > fsnotify_next_mark(iter_info->marks[type]);
>
> Wouldn't it be more natural here to use the new helper
> fsnotify_foreach_iter_mark_type()? In principle we want to advance mark
> types which were already reported...
Took me an embarrassing amount of time to figure out why this would be wrong
and I must have known this a few weeks ago when I wrote the patch, so
a comment is in order:
/*
* We cannot use fsnotify_foreach_iter_mark_type() here because we
* may need to check if next group has a mark of type X even if current
* group did not have a mark of type X.
*/
>
> > @@ -438,6 +438,9 @@ FSNOTIFY_ITER_FUNCS(sb, SB)
> >
> > #define fsnotify_foreach_iter_type(type) \
> > for (type = 0; type < FSNOTIFY_ITER_TYPE_COUNT; type++)
> > +#define fsnotify_foreach_iter_mark_type(iter, mark, type) \
> > + for (type = 0; type < FSNOTIFY_ITER_TYPE_COUNT; type++) \
> > + if (!(mark = fsnotify_iter_mark(iter, type))) {} else
>
> Hum, you're really inventive here ;) I'd rather go for something a bit more
> conservative and readable like:
It's good that you are here to restrain me ;-)
>
> static inline int fsnotify_iter_step(struct fsnotify_iter_info *iter, int type,
> struct fsnotify_mark **markp)
> {
> while (type < FSNOTIFY_ITER_TYPE_COUNT) {
> *markp = fsnotify_iter_mark(iter, type);
> if (*markp)
> break;
> type++;
> }
> return type;
> }
>
> #define fsnotify_foreach_iter_mark_type(iter, mark, type) \
> for (type = 0; \
> (type = fsnotify_iter_step(iter, type, &mark)) < FSNOTIFY_ITER_TYPE_COUNT; \
> type++)
>
>
That looks nicer.
Thanks,
Amir.
