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

[Jan Kara <jack@suse.cz>]

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.

> I am going to add a separate internal struct for storing fid in event
> (either embedded of allocated) because I am going to make it more compact
> (similar to struct ovl_fh)
> 
> struct fanotify_fid {
>         __kernel_fsid_t fsid;
>         u8 handle_bytes;
>         u8 handle_type;
>         u16 pad;
>         unsigned char f_handle[0];
> };
> 
> Will let you know when I have something ready.

OK.

> AFAICT, this rework should not affect the rest of the patches in the
> series (i.e. cached fsid
> and dirent events), so if you have time, you don't need to wait on my
> rework to continue
> review of v3.

OK, thanks for info. I'm slowly crunching through the patches but it
takes time and I have also other things to do...

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