On Mon, Nov 29, 2010 at 19:14, Eric Paris <eparis@redhat.com> wrote:
quoted text
> 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. */
> };
>

Yep, except mask should go after options_offset to avoid the gap, and
let's make both vers and options_offset u8, just in case we need the
other 2 bytes in the future:

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

quoted text
> 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 about3
> structure lengths?

Actually, with event_len being __u32, and mask moved back, we are
already abi-compatible. The users would see the wrong version, but
it's always > 2, so everyone happy. Together with the hypothetical
huge options, this fully justifies spending 2 bytes on event_len to
me.
--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/