From: Amir Goldstein <amir73il@gmail.com>
To: Jan Kara <jack@suse.cz>
Cc: Al Viro <viro@zeniv.linux.org.uk>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH v3 10/14] fanotify: divorce fanotify_path_event and fanotify_fid_event
Date: Wed, 25 Mar 2020 09:24:37 +0200 [thread overview]
Message-ID: <CAOQ4uxhh8DJC+5xPjGaph8yKXa_hSxi7ua0s3wUDaV7MPcaStw@mail.gmail.com> (raw)
In-Reply-To: <20200324175029.GD28951@quack2.suse.cz>
On Tue, Mar 24, 2020 at 7:50 PM Jan Kara <jack@suse.cz> 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@suse.cz>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> 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.
next prev parent reply other threads:[~2020-03-25 7:24 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-19 15:10 [PATCH v3 00/14] fanotify directory modify event Amir Goldstein
2020-03-19 15:10 ` [PATCH v3 01/14] fsnotify: tidy up FS_ and FAN_ constants Amir Goldstein
2020-03-19 15:10 ` [PATCH v3 02/14] fsnotify: factor helpers fsnotify_dentry() and fsnotify_file() Amir Goldstein
2020-03-19 15:10 ` [PATCH v3 03/14] fsnotify: funnel all dirent events through fsnotify_name() Amir Goldstein
2020-03-19 15:10 ` [PATCH v3 04/14] fsnotify: use helpers to access data by data_type Amir Goldstein
2020-03-19 15:10 ` [PATCH v3 05/14] fsnotify: simplify arguments passing to fsnotify_parent() Amir Goldstein
2020-03-19 15:10 ` [PATCH v3 06/14] fsnotify: pass dentry instead of inode for events possible on child Amir Goldstein
2020-03-25 10:22 ` Jan Kara
2020-03-25 11:20 ` Amir Goldstein
2020-03-19 15:10 ` [PATCH v3 07/14] fsnotify: replace inode pointer with an object id Amir Goldstein
2020-03-19 15:10 ` [PATCH v3 08/14] fanotify: merge duplicate events on parent and child Amir Goldstein
2020-03-19 15:10 ` [PATCH v3 09/14] fanotify: fix merging marks masks with FAN_ONDIR Amir Goldstein
2020-03-19 15:10 ` [PATCH v3 10/14] fanotify: divorce fanotify_path_event and fanotify_fid_event Amir Goldstein
2020-03-24 17:50 ` Jan Kara
2020-03-25 7:24 ` Amir Goldstein [this message]
2020-03-25 9:27 ` Jan Kara
2020-03-30 10:42 ` Amir Goldstein
2020-03-30 15:32 ` Jan Kara
2020-03-19 15:10 ` [PATCH v3 11/14] fanotify: send FAN_DIR_MODIFY event flavor with dir inode and name Amir Goldstein
2020-03-19 15:10 ` [PATCH v3 12/14] fanotify: prepare to report both parent and child fid's Amir Goldstein
2020-03-19 15:10 ` [PATCH v3 13/14] fanotify: record name info for FAN_DIR_MODIFY event Amir Goldstein
2020-03-19 15:10 ` [PATCH v3 14/14] fanotify: report " Amir Goldstein
2020-03-25 10:21 ` Jan Kara
2020-03-25 11:17 ` Amir Goldstein
2020-03-25 14:53 ` Jan Kara
2020-03-25 15:07 ` Amir Goldstein
2020-03-25 15:54 ` [PATCH v3 00/14] fanotify directory modify event Jan Kara
2020-03-25 16:55 ` Amir Goldstein
2020-03-25 17:01 ` Jan Kara
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAOQ4uxhh8DJC+5xPjGaph8yKXa_hSxi7ua0s3wUDaV7MPcaStw@mail.gmail.com \
--to=amir73il@gmail.com \
--cc=jack@suse.cz \
--cc=linux-fsdevel@vger.kernel.org \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).