linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Amir Goldstein <amir73il@gmail.com>
To: Jan Kara <jack@suse.cz>
Cc: Marko Rauhamaa <marko.rauhamaa@f-secure.com>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	linux-api@vger.kernel.org
Subject: Re: [PATCH v3 3/3] fanotify: add API to attach/detach super block mark
Date: Fri, 31 Aug 2018 18:30:32 +0300	[thread overview]
Message-ID: <CAOQ4uxhEmgHLdy4xkDnWsmruLUp24E+eKg8wQVA3+Q3PhUuVSg@mail.gmail.com> (raw)
In-Reply-To: <20180831140551.GK11622@quack2.suse.cz>

On Fri, Aug 31, 2018 at 5:05 PM Jan Kara <jack@suse.cz> 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@vger.kernel.org>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> 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.

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


Thanks,
Amir.

  reply	other threads:[~2018-08-31 19:38 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-30 15:15 [PATCH v3 0/3] fanotify super block marks Amir Goldstein
2018-08-30 15:15 ` [PATCH v3 1/3] fsnotify: add super block object type Amir Goldstein
2018-08-31 13:52   ` Jan Kara
2018-08-30 15:15 ` [PATCH v3 2/3] fsnotify: send path type events to group with super block marks Amir Goldstein
2018-08-31 13:50   ` Jan Kara
2018-08-31 15:07     ` Amir Goldstein
2018-09-03  8:36       ` Jan Kara
2018-08-30 15:15 ` [PATCH v3 3/3] fanotify: add API to attach/detach super block mark Amir Goldstein
2018-08-31 14:05   ` Jan Kara
2018-08-31 15:30     ` Amir Goldstein [this message]
2018-09-03  8:48       ` Jan Kara
2018-09-03  9:58         ` Amir Goldstein

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAOQ4uxhEmgHLdy4xkDnWsmruLUp24E+eKg8wQVA3+Q3PhUuVSg@mail.gmail.com \
    --to=amir73il@gmail.com \
    --cc=jack@suse.cz \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=marko.rauhamaa@f-secure.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).