Re: [PATCH v3 07/15] fsnotify: pass arguments of fsnotify() in struct fsnotify_event_info

On Thu, Jul 8, 2021 at 1:43 PM Jan Kara [off-list ref] wrote:

On Tue 29-06-21 15:10:27, Gabriel Krisman Bertazi wrote:

quoted

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

@@ -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...

quoted

 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

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?

Sure, looks good.
But I think it would be even better to have different "wrapper defines" like
FSNOTIFY_NAME_EVENT_INFO() will less irrelevant arguments.

Thanks,
Amir.