On Fri, Jan 4, 2019 at 2:18 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Fri 04-01-19 13:42:33, Amir Goldstein wrote:
> > On Fri, Jan 4, 2019 at 12:57 PM Jan Kara <jack@xxxxxxx> wrote:
> > > The reporting of FAN_ONDIR when the event was mkdir / rmdir could be useful
> > > I guess. E.g. when implementing recursive watching of a directory. Or what
> > > is your intended usecase? It should be said explicitely in the changelog.
> >
> > Recursive watch of directory tree is certainly a use case that could benefit
> > from "mkdir" events. I will add that to commit message.
>
> Hum, so it does not seem like you had any particular usecase in mind? :)
Our application does have special handling (scanning) for mkdir events.
> Before complicating the interface with reporting FAN_ONDIR in some cases
> I'd prefer we considered whether the usecases are worth it. So let me start
> that: Reporting FAN_ONDIR can distinguish between file/directory
> creation/deletion. That can save userspace some rescans of the changed
> directory if it is interested only in subdirectory creation / deletion (if
> the application is interested only in file changes it just does not set
> FAN_ONDIR and that's it).
>
> Another motivation is the compatibility with inotify and it's IN_ISDIR flag
> I guess.
>
> Hum, OK, I guess the complication is worth it.
>
Let's see.
The fact that FAN_ONDIR is an "out" flag IFF FAN_REPORT_FID
is set, is something that needs to be clarified.
The fact that reported FID refers to the modified directory in dirent events
but FAN_ONDIR flag refers to the created/removed/renamed child is
another thing that needs to be clarified.
Sigh! "things that need to be clarified" are things we need to avoid.
Therefore, I am going to make another suggestion.
How about if we introduced a new flag FAN_DIRENT_ISDIR?
Like IN_ISDIR, it is an out-only flag that cannot be requested.
As its name suggests, it is only applicable to dirent events, so will always
be set when a sub directory entry is created/renamed/removed.
FAN_ONDIR has no effect on dirent events, because FAN_ONDIR
refers to the "victim" inode and the "victim" is the modified directory.
If user specified FAN_CREATE|FAN_DELETE|FAN_MOVE, user
wants to get notified when *directories* are modified and specifying
FAN_ONDIR explicitly is not needed.
How about that?
> > > > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> > > > index e9d45387089f..f5f86566c277 100644
> > > > --- a/include/linux/fanotify.h
> > > > +++ b/include/linux/fanotify.h
> > > > @@ -61,13 +61,16 @@
> > > > #define FANOTIFY_PERM_EVENTS (FAN_OPEN_PERM | FAN_ACCESS_PERM | \
> > > > FAN_OPEN_EXEC_PERM)
> > > >
> > > > +/* Events types that may be reported from vfs */
> > > > +#define FANOTIFY_EVENT_TYPES (FANOTIFY_EVENTS | \
> > > > + FANOTIFY_PERM_EVENTS)
> > > > +
> > > > /* Extra flags that may be reported with event or control handling of events */
> > > > #define FANOTIFY_EVENT_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR)
> > > >
> > > > /* Events that may be reported to user */
> > > > -#define FANOTIFY_OUTGOING_EVENTS (FANOTIFY_EVENTS | \
> > > > - FANOTIFY_PERM_EVENTS | \
> > > > - FAN_Q_OVERFLOW)
> > > > +#define FANOTIFY_OUTGOING_EVENTS (FANOTIFY_EVENT_TYPES | \
> > > > + FAN_Q_OVERFLOW | FAN_ONDIR)
> > > >
> > > > #define ALL_FANOTIFY_EVENT_BITS (FANOTIFY_OUTGOING_EVENTS | \
> > > > FANOTIFY_EVENT_FLAGS)
> > >
> > > I don't like this renaming. FAN_ONDIR essentially becomes the same type of
> > > thing as FAN_EVENT_ON_CHILD - i.e., an event flag. So I'd just leave these
> > > defines as is...
> > >
> >
> > Sorry. I don't understand what you mean.
> > FAN_EVENT_ON_CHILD is not in FANOTIFY_OUTGOING_EVENTS
> > FAN_ONDIR is in FANOTIFY_OUTGOING_EVENTS after this change.
> > copy_event_to_user() masks out with FANOTIFY_OUTGOING_EVENTS.
> > Do you not like the new group definition FANOTIFY_EVENT_TYPES?
>
> Sorry, I've got confused and thought that FAN_EVENT_ON_CHILD gets reported
> to userspace. I don't like the FANOTIFY_EVENT_TYPES name and
> FANOTIFY_OUTGOING_EVENTS becomes somewhat a misnomer after adding FAN_ONDIR
> there. So how about renaming FANOTIFY_OUTGOING_EVENTS to
> FANOTIFY_OUTGOING_MASK and have FANOTIFY_OUTGOING_EVENTS what your
> FANOTIFY_EVENT_TYPES is?
>
Still a bit confusing to me.
I think that with FAN_DIRENT_ISDIR being a different flag than FAN_ONDIR
I can do without the FANOTIFY_EVENT_TYPES group.
something like this:
#define FANOTIFY_EVENT_IN_FLAGS (FAN_EVENT_ON_CHILD | FAN_ONDIR)
#define FANOTIFY_EVENT_OUT_FLAGS (FAN_Q_OVERFLOW | FAN_DIRENT_ISDIR)
#define FANOTIFY_OUTGOING_EVENTS (FANOTIFY_EVENTS | \
FANOTIFY_PERM_EVENTS | \
FANOTIFY_EVENT_OUT_FLAGS)
...
test_mask = event_mask & marks_mask & ~marks_ignored_mask;
if ((event_mask & FS_ISDIR) &&
(test_mask & ALL_FSNOTIFY_DIRENT_EVENTS))
test_mask |= FAN_DIRENT_ISDIR;
return test_mask & FANOTIFY_OUTGOING_EVENTS;
Thanks,
Amir.
