On Monday 29 Nov 2010 16:14:54 Eric Paris wrote:
> 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.
I see now what you want to do here. I assumed metadata is fixed for ever and
all expansions will come via options. That is why I said offset field is
redundant. If you want to leave option for changing metadata open then yes, I
agree it is needed.
> 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?
No strong feelings here on either. I do not really think maintaining
compatibility with an unreleased protocol is worth it. And I would not like to
limit any field to 8-bits if possible.
Tvrtko
Sophos Limited, The Pentagon, Abingdon Science Park, Abingdon, OX14 3YP, United Kingdom.
Company Reg No 2096520. VAT Reg No GB 991 2418 08.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
