Re: [PATCH 4/4] fanotify: Expose the file changes to the user

[Eric Paris <eparis@redhat.com>]

On Fri, 2010-11-26 at 10:11 +0000, Tvrtko Ursulin wrote:
> On Monday 22 Nov 2010 00:37:21 Alexey Zaytsev wrote:

> >  struct fanotify_event_metadata {
> > -       __u16 event_len;
> > +       __u16 event_len; /* Including the options */
> >         __u8 vers;
> > -       __u8 reserved;
> > +       __u8 options_offset; /* Aka header length */
> >         __s32 fd;
> >         __aligned_u64 mask;
> >         __s32 pid;
> > +       /* Options go here. */
> >  };

> Also, options_offset is, if I understood it correctly, basically the lenght of
> fanotify_event_metadata. As such, isn't that field redundant since the lenght
> is implied from the protocol version?

Ok, the way I envision the interface is the fanotify_event_metadata is
going to be information that is common for all event types.  If a
particular event type (in the case being brought forth MODIFY and
CLOSE_WRITE) needs more information it will use the optional headers.
I'd still like to be able to add generic info, ex: the uid of the
requesting process, to the generic event metadata.  We might also
someday want to use the optional headers for things like the inotify
move cookie.  Why waste space in every event when only the move type
events cared about this cookie?

This necessitates options_offset.  Lets say I compile my userspace app
as version 3.  I know that fanotify_event_metadata is 14 bytes long in
version 3.  Thus I start looking at byte 15 for the first optional
header.  But the kernel is version 4 and fanotify_event_metadata is
actually 18 bytes long.  Whoops, userspace should be looking at byte 19
for the header but accidentally looked at byte 15.  options_offset will
allow a version 3 userspace to run on a version 4 kernel while skipping
bytes 15-18 which it doesn't understand.

At the end of all of this discussion what did people finally agree on?
Something like this?

struct fanotify_opt_hdr {
        __u16 type;
        __u16 len;
        /* Payload goes here. */
};

struct fanotify_event_metadata {
        __u32 event_len; /* Including the options */
        __u16 vers;
        __u16 options_offset; /* Aka header length */
        __s32 fd;
        __aligned_u64 mask;
        __s32 pid;
        /* Options go here. */
};

We could be ABI compatible if we let vers as a __u32 and added
options_offset at the end of the structure...   Do we have strong
feelings on either side about maintaining compatibility or about
structure lengths?

-Eric