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

quoted

quoted

quoted

quoted

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.

I grew to like FAN_RENAME better myself as well.
To make sure we are talking about the same thing:
1. FAN_RENAME always reports 2*(dirfid+name)
2. FAN_REPORT_TARGET_FID adds optional child fid record to
    CREATE/DELETE/RENAME/MOVED_TO/FROM

Correct, that's what I meant.

quoted

Err, I tried the FAN_RENAME approach and hit a semantic issue:
Users can watch a directory inode and get only MOVED_FROM
when entries are moved from this directory. Same for MOVED_TO.
How would FAN_RENAME behave when setting FAN_RENAME on a directory inode?
Should listeners get events on files renamed in and out of that
directory?

I see several options:
1. Go back to FAN_MOVED_FROM as in this patch set, where semantics are clear

Well, semantics are clear but in principle user does not have access to
target dir either so the permission problems are the same as with option 2,
aren't they?

Correct.

quoted

2. Report FAN_RENAME if either old or new dir is watched (or mount/sb)
3. Report FAN_RENAME only if both old and new dirs are watched (or mount/sb)

For option 2, we may need to hide information records, For example,
because an unprivileged listener may not have access to old or new
directory.

Good spotting. That can indeed be a problem.

quoted

A variant of option 3, is that FAN_RENAME will be an event mask flag
that can be added to FAN_MOVE events, to request that if both FROM/TO events
are going to be reported, then a single joint event will be reported
instead, e.g:

#define FAN_MOVE (FAN_MOVED_FROM | FAN_MOVED_TO)
#define FAN_RENAME (FAN_MOVE | __FAN_MOVE_JOIN)

Instead of generating an extra FS_RENAME event in fsnotify_move(),
fsnotify() will search for matching marks on the moved->d_parent->d_inode
of MOVED_FROM event add the mark as the FSNOTIFY_OBJ_TYPE_PARENT
mark iterator type and then fanotify_group_event_mask() will be able
to tell if the
event should be reported as FAN_MOVED_FROM, FAN_MOVED_TO or a joint
FAN_RENAME.

If a group has the FAN_RENAME mask on the new parent dir, then
FS_MOVED_TO events can be dropped, because the event was already
reported as FAN_MOVED_TO or FAN_RENAME with the FS_MOVED_FROM
event.

Am I over complicating this?
Do you have a better and clearer semantics to propose?

So from API POV I like most keeping FAN_RENAME separate from FAN_MOVED_TO &
FAN_MOVED_FROM. It would be generated whenever source or target is tagged
with FAN_RENAME, source info is provided if source is tagged, target info
is provided when target is tagged (both are provides when both are tagged).
So it is kind of like FAN_MOVED_FROM | FAN_MOVED_TO but with guaranteed
merging. This looks like a clean enough and simple to explain API. Sure it
duplicates FAN_MOVED_FROM & FAN_MOVED_TO a lot but I think the simplicity
of the API outweights the duplication. Basically FAN_MOVED_FROM &
FAN_MOVED_TO could be deprecated with this semantics of FAN_RENAME although
I don't think we want to do it for compatibility reasons.

Well, not only for compatibility.
The ability to request events for files moved into directory ~/inbox/ and files
moved out of directory ~/outbox/ cannot be expressed with FAN_RENAME
alone...

Implementation-wise we have couple of options. Currently the simplest I can
see is that fsnotify() would iterate marks on both source & target dirs
(like we already do for inode & parent) when it handles FS_RENAME event. In

Yes. I already have a WIP branch (fan_reanme) using
FSNOTIFY_OBJ_TYPE_PARENT for the target dir mark.

Heads up: I intend to repurpose FS_DN_RENAME, by sending FS_RENAME
to ->handle_inode_event() backends only if (parent_mark == inode_mark).
Duplicating FS_MOVED_FROM I can cope with, but wasting 3 flags for
the same event is too much for me to bare ;-)

fanotify_handle_event() we will decide which info to report with FAN_RENAME
event based on which marks in iter_info have FS_RENAME set (luckily mount
marks are out of question for rename events so it will be relatively
simple). What do you think?

I like it. However,
If FAN_RENAME can have any combination of old,new,old+new info
we cannot get any with a single new into type
FAN_EVENT_INFO_TYPE_DFID_NAME2

(as in this posting)

We can go with:
#define FAN_EVENT_INFO_TYPE_OLD_DFID_NAME   6
#define FAN_EVENT_INFO_TYPE_NEW_DFID_NAME  7
#define FAN_EVENT_INFO_TYPE_OLD_DFID               8
#define FAN_EVENT_INFO_TYPE_NEW_DFID              9

Or we can go with:
/* Sub-types common to all three fid info types */
#define FAN_EVENT_INFO_FID_OF_OLD_DIR     1
#define FAN_EVENT_INFO_FID_OF_NEW_DIR    2

struct fanotify_event_info_header {
       __u8 info_type;
       __u8 sub_type;
       __u16 len;
};

(as in my wip branch fanotify_fid_of)

We could also have FAN_RENAME require FAN_REPORT_NAME
that would limit the number of info types, but I cannot find a good
justification for this requirement.

Any preference?

Thanks,
Amir.