Messages in this thread | | | Date | Mon, 29 Nov 2010 19:51:26 +0300 | Subject | Re: [PATCH 4/4] fanotify: Expose the file changes to the user | From | Alexey Zaytsev <> |
| |
On Mon, Nov 29, 2010 at 19:14, Eric Paris <eparis@redhat.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |