Re: [PATCH v3 04/13] fanotify: define the structures to report a unique file identifier

[Jan Kara <jack@suse.cz>]

On Wed 28-11-18 20:34:47, Amir Goldstein wrote:
> On Wed, Nov 28, 2018 at 7:43 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Wed 28-11-18 18:24:11, Amir Goldstein wrote:
> > > On Wed, Nov 28, 2018 at 5:27 PM Jan Kara <jack@suse.cz> wrote:
> > > > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > > > > index 2dbb2662a92f..93e1aa2a389f 100644
> > > > > --- a/fs/notify/fanotify/fanotify_user.c
> > > > > +++ b/fs/notify/fanotify/fanotify_user.c
> > > > > @@ -133,9 +133,10 @@ static int fill_event_metadata(struct fsnotify_group *group,
> > > > >       metadata->reserved = 0;
> > > > >       metadata->mask = fsn_event->mask & FANOTIFY_OUTGOING_EVENTS;
> > > > >       metadata->pid = pid_vnr(event->pid);
> > > > > -     if (unlikely(fsn_event->mask & FAN_Q_OVERFLOW))
> > > > > +     if (FAN_GROUP_FLAG(group, FAN_REPORT_FID) ||
> > > > > +         unlikely(fsn_event->mask & FAN_Q_OVERFLOW)) {
> > > > >               metadata->fd = FAN_NOFD;
> > > > > -     else {
> > > > > +     } else {
> > > >
> > > > Use FANOTIFY_HAS_FID() helper here?
> > >
> > > Not here. FAN_REPORT_FID implies that event->fd will never be used.
> > > But I need to check FANOTIFY_HAS_FID() before copy_fid_to_user(),
> > > because we could fail to decode fid and still report the event without fid.
> >
> > OK. So maybe something like would be more obvious?
> >
> >         if (fanotify_event_has_path(event)) {
> >                 create and store fd
> >         } else if (fanotify_event_has_fid(event))
> >                 store fid
> >         } else {
> >                 clear fd & fid
> >         }
> 
> The issue is that fill_event_metadata() function fills a fixed size header
> and later copy_event_to_user() copies that header to user and then
> copies the variable sized fid to user, so this is not the place to "store"
> fid, but I will work on readability of this code.

Aha, I see. Thanks for your patience with me :). So then how about just
folding fill_event_metadata() into copy_event_to_user() (as a preparatory
patch). It is relatively small function, has a single call site and with
FID changes the distinction isn't so clear anymore...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR