On Tue, Mar 24, 2020 at 7:50 PM Jan Kara <jack@xxxxxxx> wrote: > > On Thu 19-03-20 17:10:18, Amir Goldstein wrote: > > Breakup the union and make them both inherit from abstract fanotify_event. > > > > fanotify_path_event, fanotify_fid_event and fanotify_perm_event inherit > > from fanotify_event. > > > > type field in abstract fanotify_event determines the concrete event type. > > > > fanotify_path_event, fanotify_fid_event and fanotify_perm_event are > > allocated from separate memcache pools. > > > > The separation of struct fanotify_fid_hdr from the file handle that was > > done for efficient packing of fanotify_event is no longer needed, so > > re-group the file handle fields under struct fanotify_fh. > > > > The struct fanotify_fid, which served to group fsid and file handle for > > the union is no longer needed so break it up. > > > > Rename fanotify_perm_event casting macro to FANOTIFY_PERM(), so that > > FANOTIFY_PE() and FANOTIFY_FE() can be used as casting macros to > > fanotify_path_event and fanotify_fid_event. > > > > Suggested-by: Jan Kara <jack@xxxxxxx> > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx> > > So I was pondering about this commit. First I felt it should be split and Oh yeh. The split makes things much clearer! > second when splitting the commit I've realized I dislike how you rely on > 'struct fanotify_event' being the first in events that inherit it. That is > not well maintainable long term since over the time, hidden dependencies on > this tend to develop (you already had like four in this patch) and then > when you need to switch away from that in the future, you have a horrible > time untangling the mess... I also wanted helpers like FANOTIFY_PE() to be > inline functions to get type safety and realized you actually use > FANOTIFY_PE() both for fsnotify_event and fanotify_event which is hacky as Excellent! I avoided the FANOTIFY_E/fsn_event related cleanups, but now code looks much better and safe. > well. Finally, I've realized that fanotify was likely broken when > generating overflow events (create_fd() was returning -EOVERFLOW which > confused the caller - still need to write a testcase for that) and you > silently fix that so I wanted that as separate commit as well. I don't think you will find a test case. Before the divorce patch, the meaning of fanotify_event_has_path() is: event->fh_type == FILEID_ROOT; but overflow event with NULL path has: event->fh_type = FILEID_INVALID; So -EOVERFLOW code in was not reachable. Meaning that your patch "fanotify: Fix handling of overflow event" is correct, but its commit message is wrong. It also says: "by default fanotify event queues are unlimited", but FAN_UNLIMITED_QUEUE is opt-in??? > > All in all this commit ended up like three commits I'm attaching. I'd be > happy if you could have a look through them but the final code isn't that > different and LTP passes so I'm reasonably confident I didn't break > anything. The split and end result look very good. After rebasing my fanotify_name branch on top of your changes, it also fixed an error in FAN_REPORT_NAME test, which I was going to look at later, so your cleanup paid off real fast :-) Thanks, Amir.