| Message ID | 20181008101229.7923-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fanotify: self describing event metadata | expand |
Hi Amir, On Mon 08-10-18 13:12:29, Amir Goldstein wrote: > Use the 'reserved' u8 field in event metadata to describe event > optional information. > > When event->pid is thread id, set the flag FAN_EVENT_INFO_TID in > event->flags. > > Rename the init flag that is used to request reporting thread id > in event to FAN_REPORT_TID. > > This change is semantic, because in the only case that user requests > FAN_REPORT_TID and fanotify is not able to meet that request, > event->pid will be zero anyway. > > However, for future event info requests, it is better to be explicit > about whether fanotify was able to meet the request, similar to how > statx() API behaves. > > Cc: <linux-api@vger.kernel.org> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > > Jan, > > While working on reporting file handles, I realized that the API would > be more solid if the event info flags are bi-directional similar to > statx(). That makes some sense but I'd really like to see how you apply this to other things because e.g. with PID vs TGID I don't really see the need for any flags. It might be interesting to have PID vs TGID flag there for consistency once we really start to send them for other things but I don't see a need to rush it now. Plus we are at rc7 already so we are out of time for changes going to the coming merge window. > Even if you disapprove of the way this patch modifies the event foramt, > or if you would like to take more time to consider that, I would still > like to rename the fanotify_init flag to FAN_REPORT_TID, becasue I think > the name is better describing. See example man page with new name [1]. I agree the name is somewhat better. I did the renaming. > I realize that the reserved/flags union is a bit ugly, but I think it > will be less painful than introducing event metadata format v4 at this > time. > > What do you thing? Why would be a new metadata format problem? You will quickly run out of bits in that u8 anyways... Honza
On Mon, Oct 8, 2018 at 2:54 PM Jan Kara <jack@suse.cz> wrote: > > Hi Amir, > > On Mon 08-10-18 13:12:29, Amir Goldstein wrote: > > Use the 'reserved' u8 field in event metadata to describe event > > optional information. > > > > When event->pid is thread id, set the flag FAN_EVENT_INFO_TID in > > event->flags. > > > > Rename the init flag that is used to request reporting thread id > > in event to FAN_REPORT_TID. > > > > This change is semantic, because in the only case that user requests > > FAN_REPORT_TID and fanotify is not able to meet that request, > > event->pid will be zero anyway. > > > > However, for future event info requests, it is better to be explicit > > about whether fanotify was able to meet the request, similar to how > > statx() API behaves. > > > > Cc: <linux-api@vger.kernel.org> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > > > Jan, > > > > While working on reporting file handles, I realized that the API would > > be more solid if the event info flags are bi-directional similar to > > statx(). > > That makes some sense but I'd really like to see how you apply this to > other things because e.g. with PID vs TGID I don't really see the need for > any flags. It might be interesting to have PID vs TGID flag there for > consistency once we really start to send them for other things but I don't > see a need to rush it now. Plus we are at rc7 already so we are out of > time for changes going to the coming merge window. > I agree, I posted the event flags as a concept. There is no need to rush it now and frankly, just the single use case of FAN_REPORT_FID, I don't think the flags will be needed either. > > Even if you disapprove of the way this patch modifies the event foramt, > > or if you would like to take more time to consider that, I would still > > like to rename the fanotify_init flag to FAN_REPORT_TID, becasue I think > > the name is better describing. See example man page with new name [1]. > > I agree the name is somewhat better. I did the renaming. > Great! > > I realize that the reserved/flags union is a bit ugly, but I think it > > will be less painful than introducing event metadata format v4 at this > > time. > > > > What do you thing? > > Why would be a new metadata format problem? You will quickly run out of > bits in that u8 anyways... > Supporting and documenting two format versions is a burden. v3 was designed with several flexible design concept we still never used, so we can still squeeze some extra usability from it without maintaining and documenting a new format version. FAN_REPORT_FID is going to use meta->event_len > FAN_EVENT_METADATA_LEN. I thought that making use of the reserved bits for self descriptive info is harmless, but anyway its not a requirement for my work (yet). Thanks, Amir.
Jan Kara <jack@suse.cz>: > That makes some sense but I'd really like to see how you apply this to > other things because e.g. with PID vs TGID I don't really see the need > for any flags. It might be interesting to have PID vs TGID flag there > for consistency once we really start to send them for other things but > I don't see a need to rush it now. Plus we are at rc7 already so we > are out of time for changes going to the coming merge window. A general side note: the PID is often useless as the process can die before the fa-notification reaches the handler. I wish there were a CLOSE_PERM event that held the closing process in existence (as a zombie if nothing else) until the event has been processed. Marko
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index 2c57186caa2e..5769cf3ff035 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -171,7 +171,7 @@ struct fanotify_event_info *fanotify_alloc_event(struct fsnotify_group *group, goto out; init: __maybe_unused fsnotify_init_event(&event->fse, inode, mask); - if (FAN_GROUP_FLAG(group, FAN_EVENT_INFO_TID)) + if (FAN_GROUP_FLAG(group, FAN_REPORT_TID)) event->pid = get_pid(task_pid(current)); else event->pid = get_pid(task_tgid(current)); diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index e03be5071362..79c4ba349780 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -130,9 +130,11 @@ static int fill_event_metadata(struct fsnotify_group *group, metadata->event_len = FAN_EVENT_METADATA_LEN; metadata->metadata_len = FAN_EVENT_METADATA_LEN; metadata->vers = FANOTIFY_METADATA_VERSION; - metadata->reserved = 0; + metadata->flags = 0; metadata->mask = fsn_event->mask & FANOTIFY_OUTGOING_EVENTS; metadata->pid = pid_vnr(event->pid); + if (metadata->pid && FAN_GROUP_FLAG(group, FAN_REPORT_TID)) + metadata->flags |= FAN_EVENT_INFO_TID; if (unlikely(fsn_event->mask & FAN_Q_OVERFLOW)) metadata->fd = FAN_NOFD; else { diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h index 11c85bd3d82e..a5a60691e48b 100644 --- a/include/linux/fanotify.h +++ b/include/linux/fanotify.h @@ -19,7 +19,7 @@ FAN_CLASS_PRE_CONTENT) #define FANOTIFY_INIT_FLAGS (FANOTIFY_CLASS_BITS | \ - FAN_EVENT_INFO_TID | \ + FAN_REPORT_TID | \ FAN_CLOEXEC | FAN_NONBLOCK | \ FAN_UNLIMITED_QUEUE | FAN_UNLIMITED_MARKS) diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h index 00b2304ed124..3e2ca5c97892 100644 --- a/include/uapi/linux/fanotify.h +++ b/include/uapi/linux/fanotify.h @@ -41,7 +41,7 @@ #define FAN_ENABLE_AUDIT 0x00000040 /* Flags to determine fanotify event format */ -#define FAN_EVENT_INFO_TID 0x00000100 /* event->pid is thread id */ +#define FAN_REPORT_TID 0x00000100 /* Report thread id in event */ /* Deprecated - do not use this in programs and do not add new flags here! */ #define FAN_ALL_INIT_FLAGS (FAN_CLOEXEC | FAN_NONBLOCK | \ @@ -94,10 +94,16 @@ #define FANOTIFY_METADATA_VERSION 3 +/* Flags reported on event->flags */ +#define FAN_EVENT_INFO_TID 0x01 /* event->pid is thread id */ + struct fanotify_event_metadata { __u32 event_len; __u8 vers; - __u8 reserved; + union { + __u8 reserved; + __u8 flags; + }; __u16 metadata_len; __aligned_u64 mask; __s32 fd;
Use the 'reserved' u8 field in event metadata to describe event optional information. When event->pid is thread id, set the flag FAN_EVENT_INFO_TID in event->flags. Rename the init flag that is used to request reporting thread id in event to FAN_REPORT_TID. This change is semantic, because in the only case that user requests FAN_REPORT_TID and fanotify is not able to meet that request, event->pid will be zero anyway. However, for future event info requests, it is better to be explicit about whether fanotify was able to meet the request, similar to how statx() API behaves. Cc: <linux-api@vger.kernel.org> Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- Jan, While working on reporting file handles, I realized that the API would be more solid if the event info flags are bi-directional similar to statx(). Even if you disapprove of the way this patch modifies the event foramt, or if you would like to take more time to consider that, I would still like to rename the fanotify_init flag to FAN_REPORT_TID, becasue I think the name is better describing. See example man page with new name [1]. I realize that the reserved/flags union is a bit ugly, but I think it will be less painful than introducing event metadata format v4 at this time. What do you thing? Amir. [1] https://github.com/amir73il/man-pages/commits/fanotify_tid fs/notify/fanotify/fanotify.c | 2 +- fs/notify/fanotify/fanotify_user.c | 4 +++- include/linux/fanotify.h | 2 +- include/uapi/linux/fanotify.h | 10 ++++++++-- 4 files changed, 13 insertions(+), 5 deletions(-)