Re: [PATCH v2 2/5] fsnotify: annotate filename events

[Amir Goldstein <amir73il@gmail.com>]

On Tue, Nov 20, 2018 at 1:59 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 14-11-18 19:43:41, Amir Goldstein wrote:
> > Filename events are referring to events that modify directory entries,
> > such as create,delete,rename. Those events should always be reported
> > on a watched directory, regardless if FS_EVENT_ON_CHILD is set
> > on the watch mask.
>
> OK, I find 'directory modification events' clearer than 'filename events'.
> But I can live with your name since I don't really have a better
> alternative :). Just please define these events in terms of all FS_<foo>
> events that are involved so that everyone is on the same page which events
> you mean.
>

>From a later fanotify patch:

/*
 * Events whose reported fid is the parent directory.
 * fanotify may get support for reporting the filename in the future.
 * For now, listener only gets notified that a create/delete/rename took
 * place in that directory.
 */
#define FANOTIFY_FILENAME_EVENTS        (FAN_MOVE | FAN_CREATE | FAN_DELETE)

I went back and forth with this trying to come up with a better
name and DIR_MODIFY_EVENTS did cross my mind, but the
problem is that FS_MODIFY|FS_ISDIR is technically also a directory
modification event, so we are really looking at "directory entry modification"
and I didn't like the sounds of DIRENT_EVENTS.

So I went for a name that described the information reported in the events
which is parent+filename.

Since you say you can live with this choice, I will add FS_FILENAME_EVENTS
with a similar comment in this patch.

> > fsnotify_nameremove() and fsnotify_move() were modified to no longer
> > set the FS_EVENT_ON_CHILD event bit. This is a semantic change to
> > align with the filename event definition. It has no effect on any
> > existing backend, because dnotify and inotify always requets the
> > child events and fanotify does not get the delete,rename events.
>
> You keep forgetting about audit ;) But in this case it is fine as it always
> sets FS_EVENT_ON_CHILD as well.
>

Right... :-/

> > The fsnotify_filename() helper is used to report all the filename
> > events. It gets a reference on parent dentry and passes it as the
> > data for the event along with the filename.
> >
> > fsnotify_filename() is different from fsnotify_parent().
> > fsnotify_parent() is intended to report any events that happened on
> > child inodes when FS_EVENT_ON_CHILD is requested.
> > fsnotify_filename() is intended to report only filename events,
> > such as create,mkdir,link. Those events must always be reported
> > on a watched directory, regardless if FS_EVENT_ON_CHILD was requested.
> >
> > fsnotify_d_name() is a helper for the common case where the
> > filename to pass is dentry->d_name.name.
> >
> > It is safe to use these helpers with negative or not instantiated
> > dentries, such as the case with fsnotify_link() and
> > fsnotify_nameremove().
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  include/linux/fsnotify.h | 40 +++++++++++++++++++++++++++++++---------
> >  1 file changed, 31 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> > index 9dadc0bcd7a9..d00ec5838d6e 100644
> > --- a/include/linux/fsnotify.h
> > +++ b/include/linux/fsnotify.h
> > @@ -17,8 +17,31 @@
> >  #include <linux/slab.h>
> >  #include <linux/bug.h>
> >
> > +/*
> > + * Notify this parent about a filename event (create,delete,rename).
> > + * Unlike fsnotify_parent(), the event will be reported regardless of the
> > + * FS_EVENT_ON_CHILD mask on the parent inode
> > + */
>
> How about specifying this as 'Notify directory 'parent' about a change of
> some of its directory entries'? That way you avoid using 'filename' event
> in the description which is not defined.
>

Sure. Although I am going to define filename events now...

> > +static inline int fsnotify_filename(struct dentry *parent, __u32 mask,
> > +                                 const unsigned char *file_name, u32 cookie)
>
> And how about calling the function fsnotify_dir_change()?

Not comfortable with this name because of fsnotify_change()
being passed a directory sounds like it should call here.
The name of this helper signifies that it takes a filename argument
and pass a non NULL filename argument to fsnotify().
Nothing more, nothing less.

>
> > +{
> > +     return fsnotify(d_inode(parent), mask, parent, FSNOTIFY_EVENT_DENTRY,
> > +                     file_name, cookie);
> > +}
> > +
> > +/*
> > + * Call fsnotify_filename() with parent and d_name of this dentry.
> > + * Safe to call with negative dentry, e.g. from fsnotify_nameremove()
> > + */
> > +static inline int fsnotify_d_name(struct dentry *dentry, __u32 mask)
>
> Maybe call this fsnotify_dir_dentry_change()?
>

Similar reasoning as above.
The name of this wrapper signifies that it passed dentry->d_name as
a non NULL filename argument to fsnotify().
Nothing more, nothing less.

Thanks,
Amir.