On Tue, Feb 26, 2019 at 6:31 PM Michael Kerrisk (man-pages)
<mtk.manpages@xxxxxxxxx> wrote:
>
> Hello Amir,
>
> On 2/26/19 5:18 PM, Michael Kerrisk (man-pages) wrote:
> > Hello Amir,
> >
> > Thanks! I've applied this patch, but I have a question below.
> >
> > On 11/17/18 5:32 PM, Amir Goldstein wrote:
> >> Monitor fanotify events on the entire filesystem.
> >>
> >> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> >> ---
> >> man2/fanotify_mark.2 | 24 +++++++++++++++++++++---
> >> man7/fanotify.7 | 20 +++++++++++---------
> >> 2 files changed, 32 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/man2/fanotify_mark.2 b/man2/fanotify_mark.2
> >> index a9a482fe7..3862a8e34 100644
> >> --- a/man2/fanotify_mark.2
> >> +++ b/man2/fanotify_mark.2
> >> @@ -70,15 +70,23 @@ must be nonempty or the error
> >> will occur.
> >> .TP
> >> .B FAN_MARK_FLUSH
> >> -Remove either all mount or all non-mount marks from the fanotify group.
> >> +Remove either all marks for filesystems, all marks for mounts or all
> >> +marks for directories and files from the fanotify group.
> >> If
> >> .I flags
> >> contains
> >> .BR FAN_MARK_MOUNT ,
> >> all marks for mounts are removed from the group.
> >> +If
> >> +.I flags
> >> +contains
> >> +.BR FAN_MARK_FILESYSTEM ,
> >> +all marks for filesystems are removed from the group.
> >> Otherwise, all marks for directories and files are removed.
> >> -No flag other than
> >> +No flag other than and only one of the flags
> >
> > I think rather than "and only one of", the text should say
> > "and at most one of", since both of these flags could be omitted,
> > right?
Correct.
>
> Looking at the kernel code more closely, it seems there is no
> error generated if both FAN_MARK_MOUNT and FAN_MARK_FILESYSTEM
> are specified in the flags. That seems rather unkind to the user.
> How about adding an error return in this case, so that the user
> doesn't get surprised?
>
Hmm? That should return EINVAL.
Although I did not write a regression test for this flag validation...
#define FANOTIFY_MARK_TYPE_BITS (FAN_MARK_INODE | FAN_MARK_MOUNT | \
FAN_MARK_FILESYSTEM)
...
unsigned int mark_type = flags & FANOTIFY_MARK_TYPE_BITS;
...
switch (mark_type) {
case FAN_MARK_INODE:
case FAN_MARK_MOUNT:
case FAN_MARK_FILESYSTEM:
break;
default:
return -EINVAL;
}
Am I missing something?
FYI, we did actually also add:
#define FAN_MARK_INODE 0
to uapi header, but this is just a convenience macro.
I saw no reason to change the man page to include it.
You may have a different opinion about this.
Thanks,
Amir.
