Re: [RFC][PATCH] fanotify_mark.2: Document FAN_MARK_IGNORE

[Matthew Bobrowski <repnop@google.com>]

On Wed, Sep 07, 2022 at 01:55:30PM +0300, Amir Goldstein wrote:
> On Wed, Sep 7, 2022 at 9:46 AM Matthew Bobrowski <repnop@google.com> wrote:
> >
> > Maybe some useful nits.
> 
> Definitely useful.
> Thanks!
> 
> >
> > On Sun, Sep 04, 2022 at 06:51:13PM +0300, Amir Goldstein wrote:
> > > diff --git a/man2/fanotify_mark.2 b/man2/fanotify_mark.2
> > > index 757ad9159..e9303827c 100644
> > > --- a/man2/fanotify_mark.2
> > > +++ b/man2/fanotify_mark.2
> > > @@ -146,12 +146,102 @@ capability.
> > >  The events in
> > >  .I mask
> > >  shall be added to or removed from the ignore mask.
> > > +Note that the flags
> > > +.B FAN_ONDIR ,
> > > +and
> > > +.B FAN_EVENT_ON_CHILD
> > > +have no effect in combination with this flag.
> > > +The effect of setting those flags in the mask
> >
> > Is there anything wrong with explicitly specifying the exact flags
> > you're referring to here? Like, "is it those flags there?", or "is it
> > those flags over there?". Oh wait, you mean "it is those flags
> > there?".
> >
> > I think it's best not describe flag usage and behavior of something
> > based on spatial proximity.
> >
> 
> ok.
> 
> > > +on ignoring events that are set in the ignore mask
> > > +is undefined and depend on the Linux kernel version.
> >
> > Reading this sentence made me a little confused at first. Just
> 
> Yeh, it's a badly phrased sentence.
> I tried to improve it a bit:
> 
>  .B FAN_MARK_IGNORED_MASK
>  The events in
>  .I mask
>  shall be added to or removed from the ignore mask.
> +Note that the flags
> +.B FAN_ONDIR ,
> +and
> +.B FAN_EVENT_ON_CHILD
> +have no effect in combination with this flag.
> +The effect of setting the flags
> +.B FAN_ONDIR ,
> +and
> +.B FAN_EVENT_ON_CHILD
> +in the mark mask
> +on the events that are set in the ignore mask
> +is undefined and depends on the Linux kernel version.
> +Specifically, prior to Linux 5.9,
> 
> I would love to get suggestions for improvement.
> 
> > checking, what I understood was that the use of FAN_ONDIR or
> > FAN_EVENT_ON_CHILD when setting the ignore mask via
> > FAN_MARK_IGNORED_MASK leads to undefined behavior. Is my understanding
> > correct?
> >
> 
> Not exactly.
> Setting those flags in the ignore mask has no effect
> (as mentioned above).
> Setting those flags in the mask has undefined behavior
> w.r.t whether the events on a directory or on a child will
> be ignored, when said events are in the ignore mask.
> 
> See the "Specifically" clause below.
> 
> > > +Specifically, prior to Linux 5.9,
> > > +.\" commit 497b0c5a7c0688c1b100a9c2e267337f677c198e
> > > +setting a mark mask on a file
> > > +and a mark with ignore mask on its parent directory
> > > +would not result in ignoring events on the file,
> > > +regardless of the
> > > +.B FAN_EVENT_ON_CHILD
> > > +flag in the parent directory's mark mask.
> > > +When the ignore mask is updated with the
> > > +.B FAN_MARK_IGNORED_MASK
> > > +flag,
> >
> > I'd just allow this sentence to flow without the use of comma after
> > flag.
> 
> ok.
> 
> >
> > > +on a mark that was previously updated with the
> > > +.B FAN_MARK_IGNORE
> > > +flag,
> > > +the update fails with
> > > +.B EEXIST
> > > +error.
> > > +.TP
> > > +.BR FAN_MARK_IGNORE " (since Linux 6.0)"
> > > +.\" commit e252f2ed1c8c6c3884ab5dd34e003ed21f1fe6e0
> > > +This flag has a similar effect as setting the
> > > +.B FAN_MARK_IGNORED_MASK
> > > +flag.
> > > +The events in
> > > +.I mask
> > > +shall be added to or removed from the ignore mask.
> > > +Unlike the
> > > +.B FAN_MARK_IGNORED_MASK
> > > +flag,
> > > +this flag also has the effect that the
> > > +.B FAN_ONDIR ,
> > > +and
> > > +.B FAN_EVENT_ON_CHILD
> > > +flags take effect on the ignore mask.
> > > +Specifically, unless
> >                         ^
> >                         the
> >
> 
> ok.
> 
> > > +.B FAN_ONDIR
> > > +flag is set with
> > > +.BR FAN_MARK_IGNORE ,
> > > +events on directories will not be ignored
> >                                             ^
> >                                             .
> >
> > > +and if the flag
> >
> > I'd start a new sentence here.
> 
> ok.
> 
> >
> > > +.B FAN_EVENT_ON_CHILD
> > > +is set,
> > > +events on children will be ignored.
> > > +For example,
> > > +a mark on a directory with combination of
> > > +a mask with
> > > +.B FAN_CREATE
> > > +event
> > > +and
> > > +.B FAN_ONDIR
> > > +flag
> > > +and an ignore mask with
> > > +.B FAN_CREATE
> > > +event
> > > +and without
> > > +.B FAN_ONDIR
> > > +flag,
> > > +will result in getting only events for creation of sub-directories.
> >                                          ^
> >                                          the
> 
> ok.
> 
> >
> > > +When using this flag to add to an ignore mask
> > > +of a mount, filesystem, or directory inode mark,
> > > +the
> > > +.B FAN_MARK_IGNORED_SURV_MODIFY
> > > +flag must be specified.
> > > +Failure to do so will results with
> > > +.B EINVAL
> > > +or
> > > +.B EISDIR
> > > +error.
> > >  .TP
> > >  .B FAN_MARK_IGNORED_SURV_MODIFY
> > >  The ignore mask shall survive modify events.
> > >  If this flag is not set,
> > >  the ignore mask is cleared when a modify event occurs
> > >  for the ignored file or directory.
> > > +This flag cannot be removed from a mark once set.
> > > +When the ignore mask is updated without this flag,
> >                                                     ^
> > Don't think the comma is needed here.
> 
> ok.
> 
> FYI, I pushed the fixes to:
> 
> https://github.com/amir73il/man-pages/commits/fan_mark_ignore

One optional suggestion, but apart from that the refactoring LGTM.

Reviewed-by: Matthew Bobrowski <repnop@google.com>

/M