Re: [PATCH 0/7] Report more information in fanotify dirent events

[Jan Kara <jack@suse.cz>]

Hi Amir!

On Sat 06-11-21 18:29:39, Amir Goldstein wrote:
> On Fri, Oct 29, 2021 at 2:40 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > This patch set follows up on the discussion on FAN_REPORT_TARGET_FID [1]
> > from 3 months ago.
> >
> > With FAN_REPORT_PIDFD in 5.15 and FAN_FS_ERROR on its way to 5.16,
> > I figured we could get an early (re)start of the discussion on
> > FAN_REPORT_TARGET_FID towards 5.17.
> >
> > The added information in dirent events solves problems for my use case -
> > It helps getting the following information in a race free manner:
> > 1. fid of a created directory on mkdir
> > 2. from/to path information on rename of non-dir
> >
> > I realize those are two different API traits, but they are close enough
> > so I preferred not to clutter the REPORT flags space any further than it
> > already is. The single added flag FAN_REPORT_TARGET_FID adds:
> > 1. child fid info to CREATE/DELETE/MOVED_* events
> > 2. new parent+name info to MOVED_FROM event
> >
> > Instead of going the "inotify way" and trying to join the MOVED_FROM/
> > MOVED_TO events using a cookie, I chose to incorporate the new
> > parent+name intomation only in the MOVED_FROM event.
> > I made this choice for several reasons:
> > 1. Availability of the moved dentry in the hook and event data
> > 2. First info record is the old parent+name, like FAN_REPORT_DFID_NAME
> > 3. Unlike, MOVED_TO, MOVED_FROM was useless for applications that use
> >    DFID_NAME info to statat(2) the object as we suggested
> >
> > I chose to reduce testing complexity and require all other FID
> > flags with FAN_REPORT_TARGET_FID and there is a convenience
> > macro FAN_REPORT_ALL_FIDS that application can use.
> 
> Self comment - Don't use ALL_ for macro names in uapi...
> There are 3 comment of "Deprecated ..."  for ALL flags in fanotify.h alone...

Yeah, probably the ALL_FIDS is not worth the possible confusion when we add
another FID flag later ;)

> BTW, I did not mention the FAN_RENAME event alternative proposal in this posting
> not because I object to FAN_RENAME, just because it was simpler to implement
> the MOVED_FROM alternative, so I thought I'll start with this proposal
> and see how
> it goes.

I've read through all the patches and I didn't find anything wrong.
Thinking about FAN_RENAME proposal - essentially fsnotify_move() would call
fsnotify_name() once more with FS_RENAME event and we'd gate addition of
second dir+name info just by FS_RENAME instead of FS_MOVED_FROM &&
FAN_REPORT_TARGET_FID. Otherwise everything would be the same as in the
current patch set, wouldn't it? IMHO it looks like a bit cleaner API so I'd
lean a bit more towards that.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR