On Tue 20-11-18 16:58:31, Amir Goldstein wrote:
> On Tue, Nov 20, 2018 at 1:59 PM Jan Kara <jack@xxxxxxx> 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.
But we never generate FS_MODIFY|FS_ISDIR events so I don't think there's a
big space for confusion (and I've deliberately used CHANGE instead of
MODIFY to make the distinction even clearer). FWIW
FANOTIFY_DIRENT_MODIFY_EVENTS would also look better than _FILENAME_EVENTS
to me.
> So I went for a name that described the information reported in the events
> which is parent+filename.
Well, but you don't report parent + filename, you report file handle and it
isn't clear whether name will ever get reported. What initially confused me
about filename events was that it sounded like you want to report all
events (so also open / read / write / ...) for a _filename_.
> Since you say you can live with this choice, I will add FS_FILENAME_EVENTS
> with a similar comment in this patch.
Talking about this more I'd really prefer if we could change the name to
something else...
> > > +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.
I find the current name confusing because, as I wrote above, it creates a
feeling in me that we are notifying filename that something happens with
it. But instead we notify a directory that something happens with filenames
inside it... Generally we have names of form fsnotify_<what-happened> or
fsnotify_<who-to-tell> and fsnotify_filename() does not fall into either of
the cathegories which confused my brain trying to fit it in one of the two
cathegories and failing. Maybe fsnotify_dir_op() because it is just a helper
function for various directory operations?
> > > +{
> > > + 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.
And same objection to the original name here...
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
