On Thu, Nov 29, 2018 at 12:16 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Thu 29-11-18 10:16:27, Amir Goldstein wrote:
> > > > 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...
> > >
> >
> > Sure. While you are here, before I start reworking, wanted to run by you
> > a few minor suggestions:
> >
> > struct fanotify_event_info_fid {
> > struct fanotify_event_info_header hdr;
> > ....
> >
> > Seems more appropriate name than the shorter fanotify_event_fid name
> > that you suggested.
>
> Fine by me.
>
> > It bothers me a bit not to use struct file_handle in the definition,
> > but I don't with to start moving struct file_handle into uapi.
> > I am a bit lost trying to understand which uapi include file I need
> > to include in fanotify.h if I want to use it. fcntl.h?
>
> Hum, so far we got away with not defining file_handle to userspace and
> userspace headers don't define it either (it's just anonymouns pointer).
> The manpage for name_to_handle_at() and open_by_handle_at() does show it's
> internal structure but I'm not sure that really counts. But userspace is
> actually expected to fill in handle_bytes when passing handle to
> name_to_handle_at() so people using this syscall must have the definition
> somehow. Probably their private one...
>
> So I think moving the struct file_handle definition into uapi is the right
> thing (with the justification above). That's a cleanup on its own. I would
> probably move the definition into include/uapi/linux/fs.h as that's where
> other similar structures are defined and from kernel POV it gets included
> as a part of include/linux/fs.h as previously.
>
That makes perfectly sense.
I couldn't figure out what it means for uapi headers that struct file_handle
is defined in /usr/include/x86_64-linux-gnu/bits/fcntl-linux.h
under ifdef __USE_GNU, but I see that SYNC_FILE_RANGE_* are also
defined at the same place and they are already in include/uapi/linux/fs.h
so that should be safe for struct file_handle as well.
Thanks,
Amir.
