Re: FAN_REPORT_CHILD_FID

[Amir Goldstein <amir73il@gmail.com>]

+linux-api

On Tue, Jul 27, 2021 at 1:44 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Fri, Jul 16, 2021 at 2:24 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Fri, Jul 16, 2021 at 12:47 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Fri 16-07-21 09:53:15, Matthew Bobrowski wrote:
> > > > On Wed, Jul 14, 2021 at 03:09:56PM +0300, Amir Goldstein wrote:
> > > > > I am still debating with myself between adding a new event type
> > > > > (FAN_RENAME), adding a new report flag (FAN_REPORT_TARGET_FID)
> > > > > that adds info records to existing MOVE_ events or some combination.
> > > >
> > > > Well, if we went with adding a new event FAN_RENAME and specifying that
> > > > resulted in the generation of additional
> > > > FAN_EVENT_INFO_TYPE_DFID_NAME_{FROM,TO} information record types for an
> > > > event, wouldn't it be weird as it doesn't follow the conventional mechanism
> > > > of a listener asking for additional information records? As in,
> > > > traditionally we'd intialize the notification group with a flag and then
> > > > that flag controls whether or not one is permitted to receive events of a
> > > > particular type that may or may not include information records?
> > > >
> > > > Maybe a combination approach is needed in this instance, but this doesn't
> > > > necessarily simplify things when attempting to document the API semantics
> > > > IMO.
> > >
> > > So there are couple of ways how to approach this I guess. One is that we
> > > add a flag like FAN_REPORT_SECONDARY_DFID and FAN_REPORT_SECONDARY_NAME
> > > which would add another DFID(+NAME) record of new type to rename event. In
> > > principle these could be added to MOVED_FROM and/or MOVED_TO events
> > > (probably both are useful). But I'd find the naming somewhat confusing and
> > > difficult to sensibly describe.
> > >
> > > That's why I think it may be clearer to go with new FAN_RENAME event that
> > > will be triggered when the directory is on either end of rename(2) (source
> > > or target). If DFID(+NAME) is enabled for the group, the event would report
> > > both source and target DFIDs (and names if enabled) instead of one. I don't
> > > think special FAN_REPORT_? flag to enable the second DFID would be useful
> > > in this case (either for clarity or enabling some functionality).
> > >
> >
> > I agree that FAN_RENAME without any new REPORT flag is possible.
> > Still, I would like to at least try to come up with some UAPI that is more
> > compatible with existing semantics and simlifies them rather than creating
> > more special cases.
> >
> > For example, if FAN_REPORT_ALL_FIDS would start reporting all the
> > relevant fid records for all events, then nothing would change for
> > FAN_OPEN etc, but FAN_CREATE etc would start reporting the
> > target fid and FAN_MOVED_* would start reporting source, target and self
> > fids or dfid/name records and then FAN_MOVED_* pair can be "merged"
> > because they carry the exact same information.
> > (I suppose FAN_MOVE_SELF could be "merged" with them as well)
> >
> > When I write "merged" I mean queued as a single recorded event
> > depending on backend flags, like we do with event ON_CHILD and
> > event on self for inotify vs. fanotify.
> >
> > With this scheme, listeners that only set FAN_MOVED_FROM in
> > mask would get all relevant information and users that only set
> > FAN_MOVED_TO would get all relevant information and we avoid
> > the spam of different event formats for the same event in case
> > users set FAN_MOVED|FAN_RENAME|FAN_MOVE_SELF in the mask.
> >
> > That just leaves the question of HOW to describe the info records
> > in a consistent way.
> >
> > I was thinking about:
> >
> > #define FAN_EVENT_INFO_OF_SELF           1
> > #define FAN_EVENT_INFO_OF_SOURCE    2
> > #define FAN_EVENT_INFO_OF_TARGET     3
> >
> > struct fanotify_event_info_header {
> >         __u8 info_type; /* The type of object of the info record */
> >         __u8 info_of;     /* The subject of the info record */
> >         __u16 len;
> > };
> >
> > The existing info_type values determine HOW that info can be used.
> > For example, FAN_EVENT_INFO_TYPE_DFID can always be resolved
> > to a linked path if directory is not dead and reachable, while
> > FAN_EVENT_INFO_TYPE_FID is similar, but it could resolve to an
> > unknown path even if the inode is linked.
> >
> > Most events will only report records OF_SELF or OF_TARGET or both.
> > FAN_MOVED_* will also report records OF_SOURCE.
> >
> > One way to think about it is that we are encoding all the information
> > found in man page about what the info record is describing, depending
> > on the event type and making it available in the event itself, so application
> > does not need to code the semantics of the man page w.r.t info records
> > meaning for different event types.
> >
> > w.r.t backward compatibility of event parsers, naturally, the REPORT_ALL
> > flag protects us from regressions, but also adapting to the new scheme is
> > pretty easy. In particular, if application ignored the info_of field the only
> > event where things could go wrong is FAN_MOVED_TO because the
> > first info record is not what it used to be (assuming that we report the
> > info of source dirent first).
> >
>
> > Thoughts?
>
> Jan,
>
> I know you are catching up on emails and this is a very low priority.
> Whenever you get to it, here is a POC branch I prepared for
> FAN_REPORT_FID_OF:
>
> https://github.com/amir73il/linux/commits/fanotify_fid_of
>

Slight change of plans. I pushed new branches:
https://github.com/amir73il/linux/commits/fanotify_target_fid
https://github.com/amir73il/ltp/commits/fanotify_target_fid

With the following changes:
1. Uses FAN_REPORT_TARGET_FID as you suggested
2. Requires FAN_REPORT_NAME (to reduce test matrix)
3. Implements 3rd record only for MOVED_FROM event

> This does not include the unified move event with 3 info records
> only the basic extension of the UAPI with the sub_type info
> concept, which is used to report child fid records for dirent events.
>

So I dropped the plan for a "unified rename event".
Adding one extra record only to MOVED_FROM was really easy
because we already have that information available in the "moved"
dentry passed to the backend.

I kept the info record subtype concept and second info record
I created with type DFID_NAME and subtype FID_OF_PARENT2,
but now this UAPI detail is up for debate.

We can easily throw away the subtype and use info type DFID2_NAME
instead.

The thing is, in an earlier version I did not require FAN_REPORT_NAME
and it was useful to reuse the subtype without having to create two new
types (i.e.  DFID2 and DFID2_NAME).

All-in-all it's a very minor difference in the UAPI and I could go either way.
I am leaning towards dropping the subtype, but I am not sure, so I kept
it to hear what other people have to say (CC: linux-api).

There is also the possibility to use the same info record type for the
second info record (DFID_NAME) because this record can only be
reported after a first valid DFID_NAME record, but I do not like this option.

Thoughts?

Amir.