On Fri 31-08-18 18:30:32, Amir Goldstein wrote: > On Fri, Aug 31, 2018 at 5:05 PM Jan Kara <jack@xxxxxxx> wrote: > > > > On Thu 30-08-18 18:15:51, Amir Goldstein wrote: > > > Add another mark type flag FAN_MARK_FILESYSTEM for add/remove/flush > > > of super block mark type. > > > > > > A super block watch gets all events on the filesystem, regardless of > > > the mount from which the mark was added, unless an ignore mask exists > > > on either the inode or the mount where the event was generated. > > > > > > Only one of FAN_MARK_MOUNT and FAN_MARK_FILESYSTEM mark type flags > > > may be provided to fanotify_mark() or no mark type flag for inode mark. > > > > > > Cc: <linux-api@xxxxxxxxxxxxxxx> > > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx> > > > > Just one nit below, otherwise the patch look good to me. > > > > > diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h > > > index 74247917de04..7345b9a57f66 100644 > > > --- a/include/uapi/linux/fanotify.h > > > +++ b/include/uapi/linux/fanotify.h > > > @@ -43,6 +43,7 @@ > > > FAN_UNLIMITED_MARKS) > > > > > > /* flags used for fanotify_modify_mark() */ > > > +#define FAN_MARK_INODE 0x00000000 > > > #define FAN_MARK_ADD 0x00000001 > > > #define FAN_MARK_REMOVE 0x00000002 > > > #define FAN_MARK_DONT_FOLLOW 0x00000004 > > > @@ -51,6 +52,7 @@ > > > #define FAN_MARK_IGNORED_MASK 0x00000020 > > > #define FAN_MARK_IGNORED_SURV_MODIFY 0x00000040 > > > #define FAN_MARK_FLUSH 0x00000080 > > > +#define FAN_MARK_FILESYSTEM 0x00000100 > > > > > > #define FAN_ALL_MARK_FLAGS (FAN_MARK_ADD |\ > > > FAN_MARK_REMOVE |\ > > > @@ -59,7 +61,10 @@ > > > FAN_MARK_MOUNT |\ > > > FAN_MARK_IGNORED_MASK |\ > > > FAN_MARK_IGNORED_SURV_MODIFY |\ > > > - FAN_MARK_FLUSH) > > > + FAN_MARK_FLUSH|\ > > > + FAN_MARK_FILESYSTEM) > > > + > > > +#define FAN_ALL_MARK_TYPE_FLAGS (FAN_MARK_MOUNT | FAN_MARK_FILESYSTEM) > > > > > > /* > > > * All of the events - we build the list by hand so that we can add flags in > > > > So for completeness I'd add FAN_MARK_INODE to FAN_ALL_MARK_FLAGS and > > FAN_ALL_MARK_TYPE_FLAGS. I know it doesn't change the actual value but > > logically it belongs there when you defined it... > > > > Ok. > > > Also one more thing to consider: Different mark types cannot be combined. > > So it could save some bits in 'flags' in future if we had something like > > FAN_MARK_TYPE_MASK and (flags & FAN_MARK_TYPE_MASK) would enumerate > > different mark types - 0 for inode mark, FAN_MARK_MOUNT for mount mark, > > FAN_MARK_FILESYSTEM for superblock mark etc. Again, currently there's no > > practical difference in the values, just the names and tests for validity > > would be slightly different. > > > > So do you prefer that I replace the test (mark_type && > !is_power_of_2(mark_type)) > with a switch cases statement for supported types? > Makes sense. Yes. > Shall I go as far as: > #define FAN_MARK_TYPE_BIT1 0x00000010 > #define FAN_MARK_TYPE_BIT2 0x00000100 > #define FAN_MARK_TYPE_MASK (FAN_MARK_TYPE_BIT1 | FAN_MARK_TYPE_BIT2) > > /* mark type can be a combination of mark type bits */ > #define FAN_MARK_INODE 0 > #define FAN_MARK_MOUNT FAN_MARK_TYPE_BIT1 > #define FAN_MARK_FILESYSTEM FAN_MARK_TYPE_BIT2 Probably I would not go as far as defining FAN_MARK_TYPE_BIT?. That looks a bit confusing and it's in userspace-visible headers. I'd just define the mask and add it into FAN_ALL_MARK_FLAGS instead of FAN_MARK_MOUNT. That should protect us (together with flags & supported-type checks in do_fanotify_mark()) against messing up the definitions (at least I hope ;). Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
