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

[Alexey Zaytsev]

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

> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/