On Thu, Jun 23, 2022 at 1:14 PM Jan Kara <jack@xxxxxxx> wrote: > > On Mon 20-06-22 16:45:51, Amir Goldstein wrote: > > This flag is a new way to configure ignore mask which allows adding and > > removing the event flags FAN_ONDIR and FAN_EVENT_ON_CHILD in ignore mask. > > > > The legacy FAN_MARK_IGNORED_MASK flag would always ignore events on > > directories and would ignore events on children depending on whether > > the FAN_EVENT_ON_CHILD flag was set in the (non ignored) mask. > > > > FAN_MARK_IGNORE can be used to ignore events on children without setting > > FAN_EVENT_ON_CHILD in the mark's mask and will not ignore events on > > directories unconditionally, only when FAN_ONDIR is set in ignore mask. > > > > The new behavior is sticky. After calling fanotify_mark() with > > FAN_MARK_IGNORE once, calling fanotify_mark() with FAN_MARK_IGNORED_MASK > > will update the ignore mask, but will not change the event flags in > > ignore mask nor how these flags are treated. > > IMHO this stickyness is not very obvious. Wouldn't it be less error-prone > for users to say that once FAN_MARK_IGNORE is used for a mark, all > subsequent modifications of ignore mask have to use FAN_MARK_IGNORE? I mean > if some program bothers with FAN_MARK_IGNORE, I'd expect it to use it for > all its calls as otherwise the mixup is kind of difficult to reason > about... I like that. > > Also it follows the behavior we have picked for FAN_MARK_EVICTABLE AFAIR > but that's not really important to me. It's kind of the opposite in the case of FAN_MARK_EVICTABLE. FAN_MARK_EVICTABLE can be "upgraded" no non-evictable but not the other way around. And also with FAN_MARK_EVICTABLE we do not deprecate the old API... See man page draft: https://github.com/amir73il/man-pages/commit/58851140bbc08b9ab9c7edd8830f37cf883d8d2a#diff-7a4387558a34e18ed6fb13d31933b2e4506496f8b3dd55df700f62b258e6f004R165 > > > @@ -1591,10 +1601,20 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, > > > > /* > > * Event flags (FAN_ONDIR, FAN_EVENT_ON_CHILD) have no effect with > > - * FAN_MARK_IGNORED_MASK. > > + * FAN_MARK_IGNORED_MASK. They can be updated in ignore mask with > > + * FAN_MARK_IGNORE and then they do take effect. > > */ > > - if (ignore) > > + switch (ignore) { > > + case 0: > > + case FAN_MARK_IGNORE: > > + break; > > + case FAN_MARK_IGNORED_MASK: > > mask &= ~FANOTIFY_EVENT_FLAGS; > > + umask = FANOTIFY_EVENT_FLAGS; > > + break; > > + default: > > + return -EINVAL; > > + } > > I think this would be easier to follow as two ifs: > > /* We don't allow FAN_MARK_IGNORE & FAN_MARK_IGNORED_MASK together */ > if (ignore == FAN_MARK_IGNORE | FAN_MARK_IGNORED_MASK) > return -EINVAL; > /* > * Event flags (FAN_ONDIR, FAN_EVENT_ON_CHILD) have no effect with > * FAN_MARK_IGNORED_MASK. > */ > if (ignore == FAN_MARK_IGNORED_MASK) { > mask &= ~FANOTIFY_EVENT_FLAGS; > umask = FANOTIFY_EVENT_FLAGS; > } > Yeh that looks better. Thanks, Amir.
