Re: [PATCH v3 07/15] fsnotify: pass arguments of fsnotify() in struct fsnotify_event_info
From: Jan Kara <jack@suse.cz>
Date: 2021-07-08 10:43:12
Also in:
linux-fsdevel
On Tue 29-06-21 15:10:27, Gabriel Krisman Bertazi wrote:
From: Amir Goldstein <amir73il@gmail.com> There are a lot of arguments to fsnotify() and the handle_event() method. Pass them in a const struct instead of on the argument list. Apart from being more tidy, this helps with passing error reports to the backend. __fsnotify_parent() argument list was intentionally left untouched, because its argument list is still short enough and because most of the event info arguments are initialized inside __fsnotify_parent(). Signed-off-by: Amir Goldstein <amir73il@gmail.com> Signed-off-by: Gabriel Krisman Bertazi <redacted> --- fs/notify/fanotify/fanotify.c | 59 +++++++++++------------ fs/notify/fsnotify.c | 83 +++++++++++++++++--------------- include/linux/fsnotify.h | 15 ++++-- include/linux/fsnotify_backend.h | 73 +++++++++++++++++++++------- 4 files changed, 140 insertions(+), 90 deletions(-)
Besides the noop function issue Amir has already pointed out I have just a few nits:
quoted hunk ↗ jump to hunk
@@ -229,7 +229,11 @@ int __fsnotify_parent(struct dentry *dentry, __u32 mask, const void *data, } notify: - ret = fsnotify(mask, data, data_type, p_inode, file_name, inode, 0); + ret = __fsnotify(mask, &(struct fsnotify_event_info) { + .data = data, .data_type = data_type, + .dir = p_inode, .name = file_name, + .inode = inode, + });
What's the advantage of using __fsnotify() here instead of fsnotify()? In terms of readability the fewer places with these initializers the better I'd say...
static int fsnotify_handle_event(struct fsnotify_group *group, __u32 mask,
- const void *data, int data_type,
- struct inode *dir, const struct qstr *name,
- u32 cookie, struct fsnotify_iter_info *iter_info)
+ const struct fsnotify_event_info *event_info,
+ struct fsnotify_iter_info *iter_info)
{
struct fsnotify_mark *inode_mark = fsnotify_iter_inode_mark(iter_info);
struct fsnotify_mark *parent_mark = fsnotify_iter_parent_mark(iter_info);
+ struct fsnotify_event_info child_event_info = { };
int ret;No need to init child_event_info. It is fully rewritten if it gets used...
quoted hunk ↗ jump to hunk
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h index f8acddcf54fb..8c2c681b4495 100644 --- a/include/linux/fsnotify.h +++ b/include/linux/fsnotify.h@@ -30,7 +30,10 @@ static inline void fsnotify_name(struct inode *dir, __u32 mask, struct inode *child, const struct qstr *name, u32 cookie) { - fsnotify(mask, child, FSNOTIFY_EVENT_INODE, dir, name, NULL, cookie); + __fsnotify(mask, &(struct fsnotify_event_info) { + .data = child, .data_type = FSNOTIFY_EVENT_INODE, + .dir = dir, .name = name, .cookie = cookie, + }); }
Hmm, maybe we could have a macro initializer like:
#define FSNOTIFY_EVENT_INFO(data, data_type, dir, name, inode, cookie) \
(struct fsnotify_event_info) { \
.data = (data), .data_type = (data_type), .dir = (dir), \
.name = (name), .inode = (inode), .cookie = (cookie)}
Then we'd have:
__fsnotify(mask, &FSNOTIFY_EVENT_INFO(child, FSNOTIFY_EVENT_INODE,
dir, name, NULL, cookie));
Which looks a bit nicer to me. What do you think guys?
Honza
--
Jan Kara [off-list ref]
SUSE Labs, CR