[LTP] [PATCH v2 0/4] fanotify ltp tests for v5.7-rc1

[LTP] [PATCH v2 0/4] fanotify ltp tests for v5.7-rc1

[Amir Goldstein]

Hi Petr,

Following patches test fanotify kernel code merged to v5.7-rc1.

Changes since v1:
- Patch to fanotify09 has already been merged
- Addressed comments by Petr, Cyril, Matthew and added reviewed-by
- Added patch to use new helper fanotify_save_fid by other tests

Patch 3 adds a new test for the new event type FAN_MODIFY_DIR, which
carries a new event format (parent fid + name).
The man page patches for FAN_MODIFY_DIR have already been merged.

Thanks all for the review,
Amir.

Amir Goldstein (4):
  syscalls/fanotify15: Minor corrections
  syscalls/fanotify15: Add a test case for inode marks
  syscalls/fanotify: New test for FAN_MODIFY_DIR
  syscalls/fanotify: Use fanotify_save_fid() helper

 runtest/syscalls                              |   1 +
 testcases/kernel/syscalls/fanotify/.gitignore |   1 +
 testcases/kernel/syscalls/fanotify/fanotify.h |  36 +-
 .../kernel/syscalls/fanotify/fanotify13.c     |  44 +-
 .../kernel/syscalls/fanotify/fanotify15.c     | 191 ++++++---
 .../kernel/syscalls/fanotify/fanotify16.c     | 403 ++++++++++++++++++
 6 files changed, 596 insertions(+), 80 deletions(-)
 create mode 100644 testcases/kernel/syscalls/fanotify/fanotify16.c

-- 
2.17.1

[LTP] [PATCH v2 1/4] syscalls/fanotify15: Minor corrections

[Amir Goldstein]

- Fix calculation of events buffer size
- Read file events and dir events in two batches
- Generate FAN_MODIFY event explicitly with truncate() operation
  instead of FAN_ATTRIB event implicitly with create() operation
- FAN_MODIFY and FAN_DELETE_SELF may or may not be merged
- Use macro FAN_MOVE for FAN_MOVED_FROM | FAN_MOVED_TO

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Reviewed-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>
---
 .../kernel/syscalls/fanotify/fanotify15.c     | 63 +++++++++++++------
 1 file changed, 43 insertions(+), 20 deletions(-)

diff --git a/testcases/kernel/syscalls/fanotify/fanotify15.c b/testcases/kernel/syscalls/fanotify/fanotify15.c
index e0d513025..2e860edb2 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify15.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify15.c
@@ -25,8 +25,14 @@
 #if defined(HAVE_SYS_FANOTIFY_H)
 #include <sys/fanotify.h>
 
-#define BUF_SIZE 256
-#define EVENT_MAX 256
+#define EVENT_MAX 10
+
+/* Size of the event structure, not including file handle */
+#define EVENT_SIZE (sizeof(struct fanotify_event_metadata) + \
+		    sizeof(struct fanotify_event_info_fid))
+/* Double events buffer size to account for file handles */
+#define EVENT_BUF_LEN (EVENT_MAX * EVENT_SIZE * 2)
+
 
 #define MOUNT_POINT "mntpoint"
 #define TEST_DIR MOUNT_POINT"/test_dir"
@@ -44,7 +50,7 @@ struct event_t {
 };
 
 static int fanotify_fd;
-static char events_buf[BUF_SIZE];
+static char events_buf[EVENT_BUF_LEN];
 static struct event_t event_set[EVENT_MAX];
 
 static void do_test(void)
@@ -56,24 +62,23 @@ static void do_test(void)
 	struct fanotify_event_info_fid *event_fid;
 
 	if (fanotify_mark(fanotify_fd, FAN_MARK_ADD | FAN_MARK_FILESYSTEM,
-				FAN_CREATE | FAN_DELETE | FAN_ATTRIB |
-				FAN_MOVED_FROM | FAN_MOVED_TO |
-				FAN_DELETE_SELF | FAN_ONDIR,
+				FAN_CREATE | FAN_DELETE | FAN_MOVE |
+				FAN_MODIFY | FAN_DELETE_SELF | FAN_ONDIR,
 				AT_FDCWD, TEST_DIR) == -1) {
 		if (errno == ENODEV)
 			tst_brk(TCONF,
 				"FAN_REPORT_FID not supported on %s "
 				"filesystem", tst_device->fs_type);
 		tst_brk(TBROK | TERRNO,
-			"fanotify_mark(%d, FAN_MARK_ADD, FAN_CREATE | "
-			"FAN_DELETE | FAN_MOVED_FROM | FAN_MOVED_TO | "
-			"FAN_DELETE_SELF | FAN_ONDIR, AT_FDCWD, %s) failed",
+			"fanotify_mark(%d, FAN_MARK_ADD | FAN_MARK_FILESYSTEM, "
+			"FAN_CREATE | FAN_DELETE | FAN_MOVE | "
+			"FAN_MODIFY | FAN_DELETE_SELF | FAN_ONDIR, "
+			"AT_FDCWD, %s) failed",
 			fanotify_fd, TEST_DIR);
 	}
 
-	/* Generate a sequence of events */
-	event_set[count].mask = FAN_CREATE | FAN_MOVED_FROM | FAN_MOVED_TO | \
-				FAN_DELETE;
+	/* All dirent events on testdir are merged */
+	event_set[count].mask = FAN_CREATE | FAN_MOVE | FAN_DELETE;
 	event_set[count].handle.handle_bytes = MAX_HANDLE_SZ;
 	fanotify_get_fid(TEST_DIR, &event_set[count].fsid,
 			 &event_set[count].handle);
@@ -82,9 +87,22 @@ static void do_test(void)
 	fd = SAFE_CREAT(FILE1, 0644);
 	SAFE_CLOSE(fd);
 
+	/*
+	 * Event on child file is not merged with dirent events.
+	 */
+	event_set[count].mask = FAN_MODIFY;
+	event_set[count].handle.handle_bytes = MAX_HANDLE_SZ;
+	fanotify_get_fid(FILE1, &event_set[count].fsid,
+			 &event_set[count].handle);
+	count++;
+
+	SAFE_TRUNCATE(FILE1, 1);
 	SAFE_RENAME(FILE1, FILE2);
 
-	event_set[count].mask = FAN_ATTRIB | FAN_DELETE_SELF;
+	/*
+	 * FAN_DELETE_SELF may be merged with FAN_MODIFY event above.
+	 */
+	event_set[count].mask = FAN_DELETE_SELF;
 	event_set[count].handle.handle_bytes = MAX_HANDLE_SZ;
 	fanotify_get_fid(FILE2, &event_set[count].fsid,
 			 &event_set[count].handle);
@@ -92,6 +110,9 @@ static void do_test(void)
 
 	SAFE_UNLINK(FILE2);
 
+	/* Read file events from the event queue */
+	len = SAFE_READ(0, fanotify_fd, events_buf, EVENT_BUF_LEN);
+
 	/*
 	 * Generate a sequence of events on a directory. Subsequent events
 	 * are merged, so it's required that we set FAN_ONDIR once in
@@ -99,8 +120,7 @@ static void do_test(void)
 	 * took place. Events on subdirectories are not merged with events
 	 * on non-subdirectories.
 	 */
-	event_set[count].mask = FAN_ONDIR | FAN_CREATE | FAN_MOVED_FROM | \
-				FAN_MOVED_TO | FAN_DELETE;
+	event_set[count].mask = FAN_ONDIR | FAN_CREATE | FAN_MOVE | FAN_DELETE;
 	event_set[count].handle.handle_bytes = MAX_HANDLE_SZ;
 	fanotify_get_fid(TEST_DIR, &event_set[count].fsid,
 			 &event_set[count].handle);
@@ -118,13 +138,12 @@ static void do_test(void)
 
 	SAFE_RMDIR(DIR2);
 
-	/* Read events from the event queue */
-	len = SAFE_READ(0, fanotify_fd, events_buf, BUF_SIZE);
+	/* Read dir events from the event queue */
+	len += SAFE_READ(0, fanotify_fd, events_buf + len, EVENT_BUF_LEN - len);
 
 	/* Process each event in buffer */
 	for (i = 0, metadata = (struct fanotify_event_metadata *) events_buf;
-		FAN_EVENT_OK(metadata, len);
-		metadata = FAN_EVENT_NEXT(metadata,len), i++) {
+		FAN_EVENT_OK(metadata, len); i++) {
 		event_fid = (struct fanotify_event_info_fid *) (metadata + 1);
 		event_file_handle = (struct file_handle *) event_fid->handle;
 
@@ -141,7 +160,7 @@ static void do_test(void)
 				"Received unexpected file descriptor %d in "
 				"event. Expected to get FAN_NOFD(%d)",
 				metadata->fd, FAN_NOFD);
-		} else if (metadata->mask != event_set[i].mask) {
+		} else if (!(metadata->mask & event_set[i].mask)) {
 			tst_res(TFAIL,
 				"Got event: mask=%llx (expected %llx) "
 				"pid=%u fd=%d",
@@ -197,6 +216,10 @@ static void do_test(void)
 				*(unsigned long *)
 				event_file_handle->f_handle);
 		}
+		metadata->mask  &= ~event_set[i].mask;
+		/* No events left in current mask? Go for next event */
+		if (metadata->mask == 0)
+			metadata = FAN_EVENT_NEXT(metadata, len);
 	}
 
 	for (; i < count; i++)
-- 
2.17.1

[LTP] [PATCH v2 2/4] syscalls/fanotify15: Add a test case for inode marks

[Amir Goldstein]

Test reporting events with fid also with recusrive inode marks:
- Test events "on self" (FAN_DELETE_SELF) on file and dir
- Test events "on child" (FAN_MODIFY) on file

With recursive inode marks, verify that the FAN_MODIFY event reported
to parent "on child" is merged with the FAN_MODIFY event reported to
child.

The new test case is a regression test for commit f367a62a7cad:

    fanotify: merge duplicate events on parent and child

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Reviewed-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>
---
 .../kernel/syscalls/fanotify/fanotify15.c     | 81 +++++++++++++++++--
 1 file changed, 73 insertions(+), 8 deletions(-)

diff --git a/testcases/kernel/syscalls/fanotify/fanotify15.c b/testcases/kernel/syscalls/fanotify/fanotify15.c
index 2e860edb2..a9ed2ec81 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify15.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify15.c
@@ -9,6 +9,10 @@
  *	Test file that has been purposely designed to verify
  *	FAN_REPORT_FID functionality while using newly defined dirent
  *	events.
+ *
+ * Test case #1 is a regression test for commit f367a62a7cad:
+ *
+ *      fanotify: merge duplicate events on parent and child
  */
 #define _GNU_SOURCE
 #include "config.h"
@@ -53,28 +57,49 @@ static int fanotify_fd;
 static char events_buf[EVENT_BUF_LEN];
 static struct event_t event_set[EVENT_MAX];
 
-static void do_test(void)
+static struct test_case_t {
+	const char *tname;
+	struct fanotify_mark_type mark;
+	unsigned long mask;
+} test_cases[] = {
+	{
+		"FAN_REPORT_FID on filesystem including FAN_DELETE_SELF",
+		INIT_FANOTIFY_MARK_TYPE(FILESYSTEM),
+		FAN_DELETE_SELF,
+	},
+	{
+		"FAN_REPORT_FID on directory with FAN_EVENT_ON_CHILD",
+		INIT_FANOTIFY_MARK_TYPE(INODE),
+		FAN_EVENT_ON_CHILD,
+	},
+};
+
+static void do_test(unsigned int number)
 {
 	int i, fd, len, count = 0;
 
 	struct file_handle *event_file_handle;
 	struct fanotify_event_metadata *metadata;
 	struct fanotify_event_info_fid *event_fid;
+	struct test_case_t *tc = &test_cases[number];
+	struct fanotify_mark_type *mark = &tc->mark;
+
+	tst_res(TINFO, "Test #%d: %s", number, tc->tname);
 
-	if (fanotify_mark(fanotify_fd, FAN_MARK_ADD | FAN_MARK_FILESYSTEM,
+	if (fanotify_mark(fanotify_fd, FAN_MARK_ADD | mark->flag, tc->mask |
 				FAN_CREATE | FAN_DELETE | FAN_MOVE |
-				FAN_MODIFY | FAN_DELETE_SELF | FAN_ONDIR,
+				FAN_MODIFY | FAN_ONDIR,
 				AT_FDCWD, TEST_DIR) == -1) {
 		if (errno == ENODEV)
 			tst_brk(TCONF,
 				"FAN_REPORT_FID not supported on %s "
 				"filesystem", tst_device->fs_type);
 		tst_brk(TBROK | TERRNO,
-			"fanotify_mark(%d, FAN_MARK_ADD | FAN_MARK_FILESYSTEM, "
+			"fanotify_mark(%d, FAN_MARK_ADD | %s, "
 			"FAN_CREATE | FAN_DELETE | FAN_MOVE | "
-			"FAN_MODIFY | FAN_DELETE_SELF | FAN_ONDIR, "
+			"FAN_MODIFY | FAN_ONDIR | 0x%lx, "
 			"AT_FDCWD, %s) failed",
-			fanotify_fd, TEST_DIR);
+			fanotify_fd, mark->name, tc->mask, TEST_DIR);
 	}
 
 	/* All dirent events on testdir are merged */
@@ -87,8 +112,21 @@ static void do_test(void)
 	fd = SAFE_CREAT(FILE1, 0644);
 	SAFE_CLOSE(fd);
 
+	/* Recursive watch file for events "on self" */
+	if (mark->flag == FAN_MARK_INODE &&
+	    fanotify_mark(fanotify_fd, FAN_MARK_ADD | mark->flag,
+			  FAN_MODIFY | FAN_DELETE_SELF,
+			  AT_FDCWD, FILE1) == -1) {
+		tst_brk(TBROK | TERRNO,
+			"fanotify_mark(%d, FAN_MARK_ADD | %s, "
+			"FAN_DELETE_SELF, AT_FDCWD, %s) failed",
+			fanotify_fd, mark->name, FILE1);
+	}
+
 	/*
 	 * Event on child file is not merged with dirent events.
+	 * FAN_MODIFY event reported on file mark should be merged with the
+	 * FAN_MODIFY event reported on parent directory watch.
 	 */
 	event_set[count].mask = FAN_MODIFY;
 	event_set[count].handle.handle_bytes = MAX_HANDLE_SZ;
@@ -128,6 +166,17 @@ static void do_test(void)
 
 	SAFE_MKDIR(DIR1, 0755);
 
+	/* Recursive watch subdir for events "on self" */
+	if (mark->flag == FAN_MARK_INODE &&
+	    fanotify_mark(fanotify_fd, FAN_MARK_ADD | mark->flag,
+			  FAN_DELETE_SELF | FAN_ONDIR,
+			  AT_FDCWD, DIR1) == -1) {
+		tst_brk(TBROK | TERRNO,
+			"fanotify_mark(%d, FAN_MARK_ADD | %s,"
+			"FAN_DELETE_SELF | FAN_ONDIR, AT_FDCWD, %s) failed",
+			fanotify_fd, mark->name, DIR1);
+	}
+
 	SAFE_RENAME(DIR1, DIR2);
 
 	event_set[count].mask = FAN_ONDIR | FAN_DELETE_SELF;
@@ -141,6 +190,17 @@ static void do_test(void)
 	/* Read dir events from the event queue */
 	len += SAFE_READ(0, fanotify_fd, events_buf + len, EVENT_BUF_LEN - len);
 
+	/*
+	 * Cleanup the mark
+	 */
+	if (fanotify_mark(fanotify_fd, FAN_MARK_FLUSH | mark->flag, 0,
+			  AT_FDCWD, TEST_DIR) < 0) {
+		tst_brk(TBROK | TERRNO,
+			"fanotify_mark (%d, FAN_MARK_FLUSH | %s, 0,"
+			"AT_FDCWD, '"TEST_DIR"') failed",
+			fanotify_fd, mark->name);
+	}
+
 	/* Process each event in buffer */
 	for (i = 0, metadata = (struct fanotify_event_metadata *) events_buf;
 		FAN_EVENT_OK(metadata, len); i++) {
@@ -259,9 +319,14 @@ static struct tst_test test = {
 	.mount_device = 1,
 	.mntpoint = MOUNT_POINT,
 	.all_filesystems = 1,
-	.test_all = do_test,
+	.test = do_test,
+	.tcnt = ARRAY_SIZE(test_cases),
 	.setup = do_setup,
-	.cleanup = do_cleanup
+	.cleanup = do_cleanup,
+	.tags = (const struct tst_tag[]) {
+		{"linux-git", "f367a62a7cad"},
+		{}
+	}
 };
 
 #else
-- 
2.17.1

[LTP] [PATCH v2 3/4] syscalls/fanotify: New test for FAN_MODIFY_DIR

[Amir Goldstein]

- Watch dir modify events with name info
- Watch self delete events w/o name info
- Test inode and filesystem marks
- Check getting events for all file and dir names

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
Reviewed-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>
---
 runtest/syscalls                              |   1 +
 testcases/kernel/syscalls/fanotify/.gitignore |   1 +
 testcases/kernel/syscalls/fanotify/fanotify.h |  36 +-
 .../kernel/syscalls/fanotify/fanotify16.c     | 403 ++++++++++++++++++
 4 files changed, 438 insertions(+), 3 deletions(-)
 create mode 100644 testcases/kernel/syscalls/fanotify/fanotify16.c

diff --git a/runtest/syscalls b/runtest/syscalls
index cbab5730c..edd3e8de7 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -575,6 +575,7 @@ fanotify12 fanotify12
 fanotify13 fanotify13
 fanotify14 fanotify14
 fanotify15 fanotify15
+fanotify16 fanotify16
 
 ioperm01 ioperm01
 ioperm02 ioperm02
diff --git a/testcases/kernel/syscalls/fanotify/.gitignore b/testcases/kernel/syscalls/fanotify/.gitignore
index 68e4cc7aa..e7cf224fd 100644
--- a/testcases/kernel/syscalls/fanotify/.gitignore
+++ b/testcases/kernel/syscalls/fanotify/.gitignore
@@ -13,4 +13,5 @@
 /fanotify13
 /fanotify14
 /fanotify15
+/fanotify16
 /fanotify_child
diff --git a/testcases/kernel/syscalls/fanotify/fanotify.h b/testcases/kernel/syscalls/fanotify/fanotify.h
index 6da7e765c..adb95c91a 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify.h
+++ b/testcases/kernel/syscalls/fanotify/fanotify.h
@@ -41,6 +41,9 @@ static long fanotify_mark(int fd, unsigned int flags, uint64_t mask,
 #ifndef FAN_REPORT_TID
 #define FAN_REPORT_TID		0x00000100
 #endif
+#ifndef FAN_REPORT_FID
+#define FAN_REPORT_FID		0x00000200
+#endif
 
 #ifndef FAN_MARK_INODE
 #define FAN_MARK_INODE		0
@@ -79,9 +82,8 @@ static long fanotify_mark(int fd, unsigned int flags, uint64_t mask,
 #ifndef FAN_OPEN_EXEC_PERM
 #define FAN_OPEN_EXEC_PERM	0x00040000
 #endif
-
-#ifndef FAN_REPORT_FID
-#define FAN_REPORT_FID		0x00000200
+#ifndef FAN_DIR_MODIFY
+#define FAN_DIR_MODIFY		0x00080000
 #endif
 
 /*
@@ -106,6 +108,13 @@ typedef struct {
 #define __kernel_fsid_t lapi_fsid_t
 #endif /* __kernel_fsid_t */
 
+#ifndef FAN_EVENT_INFO_TYPE_FID
+#define FAN_EVENT_INFO_TYPE_FID		1
+#endif
+#ifndef FAN_EVENT_INFO_TYPE_DFID_NAME
+#define FAN_EVENT_INFO_TYPE_DFID_NAME	2
+#endif
+
 #ifndef HAVE_STRUCT_FANOTIFY_EVENT_INFO_HEADER
 struct fanotify_event_info_header {
 	uint8_t info_type;
@@ -155,6 +164,27 @@ static inline void fanotify_get_fid(const char *path, __kernel_fsid_t *fsid,
 			"name_to_handle_at(AT_FDCWD, %s, ...) failed", path);
 	}
 }
+
+struct fanotify_fid_t {
+	__kernel_fsid_t fsid;
+	struct file_handle handle;
+	char buf[MAX_HANDLE_SZ];
+};
+
+static inline void fanotify_save_fid(const char *path,
+				     struct fanotify_fid_t *fid)
+{
+	int *fh = (int *)(fid->handle.f_handle);
+
+	fh[0] = fh[1] = fh[2] = 0;
+	fid->handle.handle_bytes = MAX_HANDLE_SZ;
+	fanotify_get_fid(path, &fid->fsid, &fid->handle);
+
+	tst_res(TINFO,
+		"fid(%s) = %x.%x.%x.%x.%x...", path,
+		FSID_VAL_MEMBER(fid->fsid, 0), FSID_VAL_MEMBER(fid->fsid, 1),
+		fh[0], fh[1], fh[2]);
+}
 #endif /* HAVE_NAME_TO_HANDLE_AT */
 
 #define INIT_FANOTIFY_MARK_TYPE(t) \
diff --git a/testcases/kernel/syscalls/fanotify/fanotify16.c b/testcases/kernel/syscalls/fanotify/fanotify16.c
new file mode 100644
index 000000000..4d6d1383b
--- /dev/null
+++ b/testcases/kernel/syscalls/fanotify/fanotify16.c
@@ -0,0 +1,403 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2020 CTERA Networks. All Rights Reserved.
+ *
+ * Started by Amir Goldstein <amir73il@gmail.com>
+ *
+ * DESCRIPTION
+ *     Check FAN_DIR_MODIFY events with name info
+ */
+#define _GNU_SOURCE
+#include "config.h"
+
+#include <stdio.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <string.h>
+#include <sys/mount.h>
+#include <sys/syscall.h>
+#include "tst_test.h"
+#include "fanotify.h"
+
+#if defined(HAVE_SYS_FANOTIFY_H)
+#include <sys/fanotify.h>
+#include <sys/inotify.h>
+
+#define EVENT_MAX 10
+
+/* Size of the event structure, not including file handle */
+#define EVENT_SIZE (sizeof(struct fanotify_event_metadata) + \
+		    sizeof(struct fanotify_event_info_fid))
+/* Tripple events buffer size to account for file handles and names */
+#define EVENT_BUF_LEN (EVENT_MAX * EVENT_SIZE * 3)
+
+
+#define BUF_SIZE 256
+
+static char fname1[BUF_SIZE], fname2[BUF_SIZE];
+static char dname1[BUF_SIZE], dname2[BUF_SIZE];
+static int fd_notify;
+
+struct event_t {
+	unsigned long long mask;
+	struct fanotify_fid_t *fid;
+	char name[BUF_SIZE];
+};
+
+static struct event_t event_set[EVENT_MAX];
+
+static char event_buf[EVENT_BUF_LEN];
+
+#define DIR_NAME1 "test_dir1"
+#define DIR_NAME2 "test_dir2"
+#define FILE_NAME1 "test_file1"
+#define FILE_NAME2 "test_file2"
+#define MOUNT_PATH "fs_mnt"
+
+static struct test_case_t {
+	const char *tname;
+	struct fanotify_mark_type mark;
+	unsigned long mask;
+	struct fanotify_mark_type sub_mark;
+	unsigned long sub_mask;
+} test_cases[] = {
+	{
+		/* Filesystem watch for dir modify and delete self events */
+		"FAN_REPORT_FID on filesystem with FAN_DIR_MODIFY",
+		INIT_FANOTIFY_MARK_TYPE(FILESYSTEM),
+		FAN_DIR_MODIFY | FAN_DELETE_SELF | FAN_ONDIR,
+		{},
+		0,
+	},
+	{
+		/* Recursive watches for dir modify events */
+		"FAN_REPORT_FID on directories with FAN_DIR_MODIFY",
+		INIT_FANOTIFY_MARK_TYPE(INODE),
+		FAN_DIR_MODIFY,
+		/* Watches for delete self event on subdir */
+		INIT_FANOTIFY_MARK_TYPE(INODE),
+		FAN_DIR_MODIFY | FAN_DELETE_SELF | FAN_ONDIR,
+	},
+};
+
+static void do_test(unsigned int number)
+{
+	int fd, len = 0, i = 0, test_num = 0, tst_count = 0;
+	struct test_case_t *tc = &test_cases[number];
+	struct fanotify_mark_type *mark = &tc->mark;
+	struct fanotify_mark_type *sub_mark = &tc->sub_mark;
+	struct fanotify_fid_t root_fid, dir_fid, file_fid;
+
+	tst_res(TINFO, "Test #%d: %s", number, tc->tname);
+
+	fd_notify = fanotify_init(FAN_REPORT_FID, 0);
+	if (fd_notify == -1) {
+		if (errno == EINVAL)
+			tst_brk(TCONF,
+				"FAN_REPORT_FID not supported by kernel");
+
+		tst_brk(TBROK | TERRNO,
+			"fanotify_init(FAN_REPORT_FID, 0) failed");
+	}
+
+	/*
+	 * Watch dir modify events with name in filesystem/dir
+	 */
+	if (fanotify_mark(fd_notify, FAN_MARK_ADD | mark->flag, tc->mask,
+			  AT_FDCWD, MOUNT_PATH) < 0) {
+		if (errno == EINVAL)
+			tst_brk(TCONF,
+				"FAN_DIR_MODIFY not supported by kernel");
+
+		tst_brk(TBROK | TERRNO,
+		    "fanotify_mark (%d, FAN_MARK_ADD | %s, 0x%lx, "
+		    "AT_FDCWD, '"MOUNT_PATH"') failed",
+		    fd_notify, mark->name, tc->mask);
+	}
+
+	/* Save the mount root fid */
+	fanotify_save_fid(MOUNT_PATH, &root_fid);
+
+	/*
+	 * Create subdir and watch open events "on children" with name.
+	 */
+	SAFE_MKDIR(dname1, 0755);
+
+	/* Save the subdir fid */
+	fanotify_save_fid(dname1, &dir_fid);
+
+	if (tc->sub_mask &&
+	    fanotify_mark(fd_notify, FAN_MARK_ADD | sub_mark->flag, tc->sub_mask,
+			  AT_FDCWD, dname1) < 0) {
+		tst_brk(TBROK | TERRNO,
+		    "fanotify_mark (%d, FAN_MARK_ADD | %s, 0x%lx, "
+		    "AT_FDCWD, '%s') failed",
+		    fd_notify, sub_mark->name, tc->sub_mask, dname1);
+	}
+
+	event_set[tst_count].mask = FAN_DIR_MODIFY;
+	event_set[tst_count].fid = &root_fid;
+	strcpy(event_set[tst_count].name, DIR_NAME1);
+	tst_count++;
+
+	/* Generate modify events "on child" */
+	fd = SAFE_CREAT(fname1, 0755);
+
+	/* Save the file fid */
+	fanotify_save_fid(fname1, &file_fid);
+
+	SAFE_WRITE(1, fd, "1", 1);
+	SAFE_RENAME(fname1, fname2);
+	SAFE_CLOSE(fd);
+
+	/* Generate delete events with fname2 */
+	SAFE_UNLINK(fname2);
+
+	/* Read events on files in subdir */
+	len += SAFE_READ(0, fd_notify, event_buf + len, EVENT_BUF_LEN - len);
+
+	/*
+	 * FAN_DIR_MODIFY events with the same name are merged.
+	 */
+	event_set[tst_count].mask = FAN_DIR_MODIFY;
+	event_set[tst_count].fid = &dir_fid;
+	strcpy(event_set[tst_count].name, FILE_NAME1);
+	tst_count++;
+	event_set[tst_count].mask = FAN_DIR_MODIFY;
+	event_set[tst_count].fid = &dir_fid;
+	strcpy(event_set[tst_count].name, FILE_NAME2);
+	tst_count++;
+
+	/*
+	 * Directory watch does not get self events on children.
+	 * Filesystem watch gets self event w/o name info.
+	 */
+	if (mark->flag == FAN_MARK_FILESYSTEM) {
+		event_set[tst_count].mask = FAN_DELETE_SELF;
+		event_set[tst_count].fid = &file_fid;
+		strcpy(event_set[tst_count].name, "");
+		tst_count++;
+	}
+
+	SAFE_RENAME(dname1, dname2);
+	SAFE_RMDIR(dname2);
+
+	/* Read more events on dirs */
+	len += SAFE_READ(0, fd_notify, event_buf + len, EVENT_BUF_LEN - len);
+
+	event_set[tst_count].mask = FAN_DIR_MODIFY;
+	event_set[tst_count].fid = &root_fid;
+	strcpy(event_set[tst_count].name, DIR_NAME1);
+	tst_count++;
+	event_set[tst_count].mask = FAN_DIR_MODIFY;
+	event_set[tst_count].fid = &root_fid;
+	strcpy(event_set[tst_count].name, DIR_NAME2);
+	tst_count++;
+	/*
+	 * Directory watch gets self event on itself w/o name info.
+	 */
+	event_set[tst_count].mask = FAN_DELETE_SELF | FAN_ONDIR;
+	strcpy(event_set[tst_count].name, "");
+	event_set[tst_count].fid = &dir_fid;
+	tst_count++;
+
+	/*
+	 * Cleanup the marks
+	 */
+	SAFE_CLOSE(fd_notify);
+	fd_notify = -1;
+
+	while (i < len) {
+		struct event_t *expected = &event_set[test_num];
+		struct fanotify_event_metadata *event;
+		struct fanotify_event_info_fid *event_fid;
+		struct file_handle *file_handle;
+		unsigned int fhlen;
+		const char *filename;
+		int namelen, info_type;
+
+		event = (struct fanotify_event_metadata *)&event_buf[i];
+		event_fid = (struct fanotify_event_info_fid *)(event + 1);
+		file_handle = (struct file_handle *)event_fid->handle;
+		fhlen = file_handle->handle_bytes;
+		filename = (char *)file_handle->f_handle + fhlen;
+		namelen = ((char *)event + event->event_len) - filename;
+		/* End of event could have name, zero padding, both or none */
+		if (namelen > 0) {
+			namelen = strlen(filename);
+		} else {
+			filename = "";
+			namelen = 0;
+		}
+
+		if (expected->name[0]) {
+			info_type = FAN_EVENT_INFO_TYPE_DFID_NAME;
+		} else {
+			info_type = FAN_EVENT_INFO_TYPE_FID;
+		}
+
+		if (test_num >= tst_count) {
+			tst_res(TFAIL,
+				"got unnecessary event: mask=%llx "
+				"pid=%u fd=%d name='%s' "
+				"len=%d info_type=%d info_len=%d fh_len=%d",
+				(unsigned long long)event->mask,
+				(unsigned)event->pid, event->fd, filename,
+				event->event_len, event_fid->hdr.info_type,
+				event_fid->hdr.len, fhlen);
+		} else if (!fhlen || namelen < 0) {
+			tst_res(TFAIL,
+				"got event without fid: mask=%llx pid=%u fd=%d, "
+				"len=%d info_type=%d info_len=%d fh_len=%d",
+				(unsigned long long)event->mask,
+				(unsigned)event->pid, event->fd,
+				event->event_len, event_fid->hdr.info_type,
+				event_fid->hdr.len, fhlen);
+		} else if (event->mask != expected->mask) {
+			tst_res(TFAIL,
+				"got event: mask=%llx (expected %llx) "
+				"pid=%u fd=%d name='%s' "
+				"len=%d info_type=%d info_len=%d fh_len=%d",
+				(unsigned long long)event->mask, expected->mask,
+				(unsigned)event->pid, event->fd, filename,
+				event->event_len, event_fid->hdr.info_type,
+				event_fid->hdr.len, fhlen);
+		} else if (info_type != event_fid->hdr.info_type) {
+			tst_res(TFAIL,
+				"got event: mask=%llx pid=%u fd=%d, "
+				"len=%d info_type=%d expected(%d) info_len=%d fh_len=%d",
+				(unsigned long long)event->mask,
+				(unsigned)event->pid, event->fd,
+				event->event_len, event_fid->hdr.info_type,
+				info_type, event_fid->hdr.len, fhlen);
+		} else if (fhlen != expected->fid->handle.handle_bytes) {
+			tst_res(TFAIL,
+				"got event: mask=%llx pid=%u fd=%d name='%s' "
+				"len=%d info_type=%d info_len=%d fh_len=%d expected(%d)"
+				"fh_type=%d",
+				(unsigned long long)event->mask,
+				(unsigned)event->pid, event->fd, filename,
+				event->event_len, info_type,
+				event_fid->hdr.len, fhlen,
+				expected->fid->handle.handle_bytes,
+				file_handle->handle_type);
+		} else if (file_handle->handle_type !=
+			   expected->fid->handle.handle_type) {
+			tst_res(TFAIL,
+				"got event: mask=%llx pid=%u fd=%d name='%s' "
+				"len=%d info_type=%d info_len=%d fh_len=%d "
+				"fh_type=%d expected(%x)",
+				(unsigned long long)event->mask,
+				(unsigned)event->pid, event->fd, filename,
+				event->event_len, info_type,
+				event_fid->hdr.len, fhlen,
+				file_handle->handle_type,
+				expected->fid->handle.handle_type);
+		} else if (memcmp(file_handle->f_handle,
+				  expected->fid->handle.f_handle, fhlen)) {
+			tst_res(TFAIL,
+				"got event: mask=%llx pid=%u fd=%d name='%s' "
+				"len=%d info_type=%d info_len=%d fh_len=%d "
+				"fh_type=%d unexpected file handle (%x...)",
+				(unsigned long long)event->mask,
+				(unsigned)event->pid, event->fd, filename,
+				event->event_len, info_type,
+				event_fid->hdr.len, fhlen,
+				file_handle->handle_type,
+				*(int *)(file_handle->f_handle));
+		} else if (memcmp(&event_fid->fsid, &expected->fid->fsid,
+				  sizeof(event_fid->fsid)) != 0) {
+			tst_res(TFAIL,
+				"got event: mask=%llx pid=%u fd=%d name='%s' "
+				"len=%d info_type=%d info_len=%d fh_len=%d "
+				"fsid=%x.%x (expected %x.%x)",
+				(unsigned long long)event->mask,
+				(unsigned)event->pid, event->fd, filename,
+				event->event_len, info_type,
+				event_fid->hdr.len, fhlen,
+				FSID_VAL_MEMBER(event_fid->fsid, 0),
+				FSID_VAL_MEMBER(event_fid->fsid, 1),
+				FSID_VAL_MEMBER(expected->fid->fsid, 0),
+				FSID_VAL_MEMBER(expected->fid->fsid, 1));
+		} else if (strcmp(expected->name, filename)) {
+			tst_res(TFAIL,
+				"got event: mask=%llx "
+				"pid=%u fd=%d name='%s' expected('%s') "
+				"len=%d info_type=%d info_len=%d fh_len=%d",
+				(unsigned long long)event->mask,
+				(unsigned)event->pid, event->fd,
+				filename, expected->name,
+				event->event_len, event_fid->hdr.info_type,
+				event_fid->hdr.len, fhlen);
+		} else if (event->pid != getpid()) {
+			tst_res(TFAIL,
+				"got event: mask=%llx pid=%u "
+				"(expected %u) fd=%d name='%s' "
+				"len=%d info_type=%d info_len=%d fh_len=%d",
+				(unsigned long long)event->mask,
+				(unsigned)event->pid,
+				(unsigned)getpid(),
+				event->fd, filename,
+				event->event_len, event_fid->hdr.info_type,
+				event_fid->hdr.len, fhlen);
+		} else {
+			tst_res(TPASS,
+				"got event #%d: mask=%llx pid=%u fd=%d name='%s' "
+				"len=%d info_type=%d info_len=%d fh_len=%d",
+				test_num, (unsigned long long)event->mask,
+				(unsigned)event->pid, event->fd, filename,
+				event->event_len, event_fid->hdr.info_type,
+				event_fid->hdr.len, fhlen);
+		}
+
+		i += event->event_len;
+		if (event->fd > 0)
+			SAFE_CLOSE(event->fd);
+		test_num++;
+	}
+
+	for (; test_num < tst_count; test_num++) {
+		tst_res(TFAIL, "didn't get event: mask=%llx, name='%s'",
+			 event_set[test_num].mask, event_set[test_num].name);
+
+	}
+}
+
+static void setup(void)
+{
+	int fd;
+
+	/* Check kernel for fanotify support */
+	fd = SAFE_FANOTIFY_INIT(FAN_CLASS_NOTIF, O_RDONLY);
+	SAFE_CLOSE(fd);
+
+	sprintf(dname1, "%s/%s", MOUNT_PATH, DIR_NAME1);
+	sprintf(dname2, "%s/%s", MOUNT_PATH, DIR_NAME2);
+	sprintf(fname1, "%s/%s", dname1, FILE_NAME1);
+	sprintf(fname2, "%s/%s", dname1, FILE_NAME2);
+}
+
+static void cleanup(void)
+{
+	if (fd_notify > 0)
+		SAFE_CLOSE(fd_notify);
+}
+
+static struct tst_test test = {
+	.test = do_test,
+	.tcnt = ARRAY_SIZE(test_cases),
+	.dev_fs_flags = TST_FS_SKIP_FUSE,
+	.setup = setup,
+	.cleanup = cleanup,
+	.mount_device = 1,
+	.mntpoint = MOUNT_PATH,
+	.all_filesystems = 1,
+	.needs_tmpdir = 1,
+	.needs_root = 1
+};
+
+#else
+	TST_TEST_TCONF("system doesn't have required fanotify support");
+#endif
-- 
2.17.1

[LTP] [PATCH v2 4/4] syscalls/fanotify: Use fanotify_save_fid() helper

[Amir Goldstein]

Reduce some boiler plate code in FAN_REPORT_FID tests and
save fid only once per object instead of once per expected event.

Suggested-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 .../kernel/syscalls/fanotify/fanotify13.c     | 44 ++++++-------
 .../kernel/syscalls/fanotify/fanotify15.c     | 63 +++++++++----------
 2 files changed, 50 insertions(+), 57 deletions(-)

diff --git a/testcases/kernel/syscalls/fanotify/fanotify13.c b/testcases/kernel/syscalls/fanotify/fanotify13.c
index 3d8de6009..51a498202 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify13.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify13.c
@@ -44,18 +44,16 @@
 #if defined(HAVE_NAME_TO_HANDLE_AT)
 struct event_t {
 	unsigned long long expected_mask;
-	__kernel_fsid_t fsid;
-	struct file_handle handle;
-	char buf[MAX_HANDLE_SZ];
 };
 
 static struct object_t {
 	const char *path;
 	int is_dir;
+	struct fanotify_fid_t fid;
 } objects[] = {
-	{FILE_PATH_ONE, 0},
-	{FILE_PATH_TWO, 0},
-	{DIR_PATH_ONE, 1}
+	{FILE_PATH_ONE, 0, {}},
+	{FILE_PATH_TWO, 0, {}},
+	{DIR_PATH_ONE, 1, {}}
 };
 
 static struct test_case_t {
@@ -108,11 +106,8 @@ static void create_objects(void)
 static void get_object_stats(void)
 {
 	unsigned int i;
-	for (i = 0; i < ARRAY_SIZE(objects); i++) {
-		event_set[i].handle.handle_bytes = MAX_HANDLE_SZ;
-		fanotify_get_fid(objects[i].path, &event_set[i].fsid,
-				&event_set[i].handle);
-	}
+	for (i = 0; i < ARRAY_SIZE(objects); i++)
+		fanotify_save_fid(objects[i].path, &objects[i].fid);
 }
 
 static int setup_marks(unsigned int fd, struct test_case_t *tc)
@@ -130,8 +125,8 @@ static int setup_marks(unsigned int fd, struct test_case_t *tc)
 					"kernel");
 				return 1;
 			} else if (errno == ENODEV &&
-					!event_set[i].fsid.val[0] &&
-					!event_set[i].fsid.val[1]) {
+				   !FSID_VAL_MEMBER(objects[i].fid.fsid, 0) &&
+				   !FSID_VAL_MEMBER(objects[i].fid.fsid, 1)) {
 				tst_res(TCONF,
 					"FAN_REPORT_FID not supported on "
 					"filesystem type %s",
@@ -207,6 +202,7 @@ static void do_test(unsigned int number)
 	for (i = 0, metadata = (struct fanotify_event_metadata *) events_buf;
 		FAN_EVENT_OK(metadata, len);
 		metadata = FAN_EVENT_NEXT(metadata, len), i++) {
+		struct fanotify_fid_t *expected_fid = &objects[i].fid;
 		event_fid = (struct fanotify_event_info_fid *) (metadata + 1);
 		event_file_handle = (struct file_handle *) event_fid->handle;
 
@@ -226,43 +222,43 @@ static void do_test(unsigned int number)
 				event_set[i].expected_mask);
 
 		/* Verify handle_bytes returned in event */
-		if (event_file_handle->handle_bytes
-				!= event_set[i].handle.handle_bytes) {
+		if (event_file_handle->handle_bytes !=
+		    expected_fid->handle.handle_bytes) {
 			tst_res(TFAIL,
 				"handle_bytes (%x) returned in event does not "
 				"equal to handle_bytes (%x) returned in "
 				"name_to_handle_at(2)",
 				event_file_handle->handle_bytes,
-				event_set[i].handle.handle_bytes);
+				expected_fid->handle.handle_bytes);
 			continue;
 		}
 
 		/* Verify handle_type returned in event */
 		if (event_file_handle->handle_type !=
-				event_set[i].handle.handle_type) {
+		    expected_fid->handle.handle_type) {
 			tst_res(TFAIL,
 				"handle_type (%x) returned in event does not "
 				"equal to handle_type (%x) returned in "
 				"name_to_handle_at(2)",
 				event_file_handle->handle_type,
-				event_set[i].handle.handle_type);
+				expected_fid->handle.handle_type);
 			continue;
 		}
 
 		/* Verify file identifier f_handle returned in event */
 		if (memcmp(event_file_handle->f_handle,
-				event_set[i].handle.f_handle,
-				event_set[i].handle.handle_bytes) != 0) {
+			   expected_fid->handle.f_handle,
+			   expected_fid->handle.handle_bytes) != 0) {
 			tst_res(TFAIL,
-				"event_file_handle->f_handle does not match "
-				"event_set[i].handle.f_handle returned in "
+				"file_handle returned in event does not match "
+				"the file_handle returned in "
 				"name_to_handle_at(2)");
 			continue;
 		}
 
 		/* Verify filesystem ID fsid  returned in event */
-		if (memcmp(&event_fid->fsid, &event_set[i].fsid,
-				sizeof(event_set[i].fsid)) != 0) {
+		if (memcmp(&event_fid->fsid, &expected_fid->fsid,
+			   sizeof(expected_fid->fsid)) != 0) {
 			tst_res(TFAIL,
 				"event_fid.fsid != stat.f_fsid that was "
 				"obtained via statfs(2)");
diff --git a/testcases/kernel/syscalls/fanotify/fanotify15.c b/testcases/kernel/syscalls/fanotify/fanotify15.c
index a9ed2ec81..cca6a5313 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify15.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify15.c
@@ -48,9 +48,7 @@
 #if defined(HAVE_NAME_TO_HANDLE_AT)
 struct event_t {
 	unsigned long long mask;
-	__kernel_fsid_t fsid;
-	struct file_handle handle;
-	char buf[MAX_HANDLE_SZ];
+	struct fanotify_fid_t *fid;
 };
 
 static int fanotify_fd;
@@ -83,6 +81,7 @@ static void do_test(unsigned int number)
 	struct fanotify_event_info_fid *event_fid;
 	struct test_case_t *tc = &test_cases[number];
 	struct fanotify_mark_type *mark = &tc->mark;
+	struct fanotify_fid_t root_fid, dir_fid, file_fid;
 
 	tst_res(TINFO, "Test #%d: %s", number, tc->tname);
 
@@ -102,16 +101,20 @@ static void do_test(unsigned int number)
 			fanotify_fd, mark->name, tc->mask, TEST_DIR);
 	}
 
+	/* Save the test root dir fid */
+	fanotify_save_fid(TEST_DIR, &root_fid);
+
 	/* All dirent events on testdir are merged */
 	event_set[count].mask = FAN_CREATE | FAN_MOVE | FAN_DELETE;
-	event_set[count].handle.handle_bytes = MAX_HANDLE_SZ;
-	fanotify_get_fid(TEST_DIR, &event_set[count].fsid,
-			 &event_set[count].handle);
+	event_set[count].fid = &root_fid;
 	count++;
 
 	fd = SAFE_CREAT(FILE1, 0644);
 	SAFE_CLOSE(fd);
 
+	/* Save the file fid */
+	fanotify_save_fid(FILE1, &file_fid);
+
 	/* Recursive watch file for events "on self" */
 	if (mark->flag == FAN_MARK_INODE &&
 	    fanotify_mark(fanotify_fd, FAN_MARK_ADD | mark->flag,
@@ -129,9 +132,7 @@ static void do_test(unsigned int number)
 	 * FAN_MODIFY event reported on parent directory watch.
 	 */
 	event_set[count].mask = FAN_MODIFY;
-	event_set[count].handle.handle_bytes = MAX_HANDLE_SZ;
-	fanotify_get_fid(FILE1, &event_set[count].fsid,
-			 &event_set[count].handle);
+	event_set[count].fid = &file_fid;
 	count++;
 
 	SAFE_TRUNCATE(FILE1, 1);
@@ -141,9 +142,7 @@ static void do_test(unsigned int number)
 	 * FAN_DELETE_SELF may be merged with FAN_MODIFY event above.
 	 */
 	event_set[count].mask = FAN_DELETE_SELF;
-	event_set[count].handle.handle_bytes = MAX_HANDLE_SZ;
-	fanotify_get_fid(FILE2, &event_set[count].fsid,
-			 &event_set[count].handle);
+	event_set[count].fid = &file_fid;
 	count++;
 
 	SAFE_UNLINK(FILE2);
@@ -159,13 +158,14 @@ static void do_test(unsigned int number)
 	 * on non-subdirectories.
 	 */
 	event_set[count].mask = FAN_ONDIR | FAN_CREATE | FAN_MOVE | FAN_DELETE;
-	event_set[count].handle.handle_bytes = MAX_HANDLE_SZ;
-	fanotify_get_fid(TEST_DIR, &event_set[count].fsid,
-			 &event_set[count].handle);
+	event_set[count].fid = &root_fid;
 	count++;
 
 	SAFE_MKDIR(DIR1, 0755);
 
+	/* Save the subdir fid */
+	fanotify_save_fid(DIR1, &dir_fid);
+
 	/* Recursive watch subdir for events "on self" */
 	if (mark->flag == FAN_MARK_INODE &&
 	    fanotify_mark(fanotify_fd, FAN_MARK_ADD | mark->flag,
@@ -180,9 +180,7 @@ static void do_test(unsigned int number)
 	SAFE_RENAME(DIR1, DIR2);
 
 	event_set[count].mask = FAN_ONDIR | FAN_DELETE_SELF;
-	event_set[count].handle.handle_bytes = MAX_HANDLE_SZ;
-	fanotify_get_fid(DIR2, &event_set[count].fsid,
-			 &event_set[count].handle);
+	event_set[count].fid = &dir_fid;
 	count++;
 
 	SAFE_RMDIR(DIR2);
@@ -204,6 +202,7 @@ static void do_test(unsigned int number)
 	/* Process each event in buffer */
 	for (i = 0, metadata = (struct fanotify_event_metadata *) events_buf;
 		FAN_EVENT_OK(metadata, len); i++) {
+		struct event_t *expected = &event_set[i];
 		event_fid = (struct fanotify_event_info_fid *) (metadata + 1);
 		event_file_handle = (struct file_handle *) event_fid->handle;
 
@@ -220,13 +219,12 @@ static void do_test(unsigned int number)
 				"Received unexpected file descriptor %d in "
 				"event. Expected to get FAN_NOFD(%d)",
 				metadata->fd, FAN_NOFD);
-		} else if (!(metadata->mask & event_set[i].mask)) {
+		} else if (!(metadata->mask & expected->mask)) {
 			tst_res(TFAIL,
 				"Got event: mask=%llx (expected %llx) "
 				"pid=%u fd=%d",
 				(unsigned long long) metadata->mask,
-				event_set[i].mask,
-				(unsigned) metadata->pid,
+				expected->mask, (unsigned) metadata->pid,
 				metadata->fd);
 		} else if (metadata->pid != getpid()) {
 			tst_res(TFAIL,
@@ -237,31 +235,30 @@ static void do_test(unsigned int number)
 				(unsigned) getpid(),
 				metadata->fd);
 		} else if (event_file_handle->handle_bytes !=
-				event_set[i].handle.handle_bytes) {
+			   expected->fid->handle.handle_bytes) {
 			tst_res(TFAIL,
 				"Got event: handle_bytes (%x) returned in "
 				"event does not equal handle_bytes (%x) "
 				"retunred in name_to_handle_at(2)",
 				event_file_handle->handle_bytes,
-				event_set[i].handle.handle_bytes);
+				expected->fid->handle.handle_bytes);
 		} else if (event_file_handle->handle_type !=
-				event_set[i].handle.handle_type) {
+			   expected->fid->handle.handle_type) {
 			tst_res(TFAIL,
 				"handle_type (%x) returned in event does not "
 				"equal to handle_type (%x) returned in "
 				"name_to_handle_at(2)",
 				event_file_handle->handle_type,
-				event_set[i].handle.handle_type);
+				expected->fid->handle.handle_type);
 		} else if (memcmp(event_file_handle->f_handle,
-					event_set[i].handle.f_handle,
-					event_set[i].handle.handle_bytes)
-					!= 0) {
+				  expected->fid->handle.f_handle,
+				  expected->fid->handle.handle_bytes) != 0) {
 			tst_res(TFAIL,
-				"event_file_handle->f_handle does not match "
-				"handle.f_handle returned in "
+				"file_handle returned in event does not match "
+				"the file_handle returned in "
 				"name_to_handle_at(2)");
-		} else if (memcmp(&event_fid->fsid, &event_set[i].fsid,
-					sizeof(event_set[i].fsid)) != 0) {
+		} else if (memcmp(&event_fid->fsid, &expected->fid->fsid,
+				  sizeof(event_fid->fsid)) != 0) {
 			tst_res(TFAIL,
 				"event_fid->fsid != stats.f_fsid that was "
 				"obtained via statfs(2)");
@@ -276,7 +273,7 @@ static void do_test(unsigned int number)
 				*(unsigned long *)
 				event_file_handle->f_handle);
 		}
-		metadata->mask  &= ~event_set[i].mask;
+		metadata->mask  &= ~expected->mask;
 		/* No events left in current mask? Go for next event */
 		if (metadata->mask == 0)
 			metadata = FAN_EVENT_NEXT(metadata, len);
-- 
2.17.1

[LTP] [PATCH v2 4/4] syscalls/fanotify: Use fanotify_save_fid() helper

[Matthew Bobrowski]

On Sat, May 02, 2020 at 07:27:44PM +0300, Amir Goldstein wrote:
> Reduce some boiler plate code in FAN_REPORT_FID tests and
> save fid only once per object instead of once per expected event.
> 
> Suggested-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---

Nice, thanks for cleaning up those other tests too. Changes look fine
to me.

Reviewed-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>

/M

[LTP] [PATCH v2 0/4] fanotify ltp tests for v5.7-rc1

[Petr Vorel]

Hi Amir,

> Hi Petr,

> Following patches test fanotify kernel code merged to v5.7-rc1.

> Changes since v1:
> - Patch to fanotify09 has already been merged
> - Addressed comments by Petr, Cyril, Matthew and added reviewed-by
> - Added patch to use new helper fanotify_save_fid by other tests

> Patch 3 adds a new test for the new event type FAN_MODIFY_DIR, which
> carries a new event format (parent fid + name).
> The man page patches for FAN_MODIFY_DIR have already been merged.

LGTM, I'll just fix failure on MUSl and then merge
https://travis-ci.org/github/pevik/ltp/jobs/682785812

Kind regards,
Petr

> Thanks all for the review,
> Amir.

> Amir Goldstein (4):
>   syscalls/fanotify15: Minor corrections
>   syscalls/fanotify15: Add a test case for inode marks
>   syscalls/fanotify: New test for FAN_MODIFY_DIR
>   syscalls/fanotify: Use fanotify_save_fid() helper

>  runtest/syscalls                              |   1 +
>  testcases/kernel/syscalls/fanotify/.gitignore |   1 +
>  testcases/kernel/syscalls/fanotify/fanotify.h |  36 +-
>  .../kernel/syscalls/fanotify/fanotify13.c     |  44 +-
>  .../kernel/syscalls/fanotify/fanotify15.c     | 191 ++++++---
>  .../kernel/syscalls/fanotify/fanotify16.c     | 403 ++++++++++++++++++
>  6 files changed, 596 insertions(+), 80 deletions(-)
>  create mode 100644 testcases/kernel/syscalls/fanotify/fanotify16.c

[LTP] [PATCH v2 2/4] syscalls/fanotify15: Add a test case for inode marks

[Jan Kara]

On Sat 02-05-20 19:27:42, Amir Goldstein wrote:
> Test reporting events with fid also with recusrive inode marks:
> - Test events "on self" (FAN_DELETE_SELF) on file and dir
> - Test events "on child" (FAN_MODIFY) on file
> 
> With recursive inode marks, verify that the FAN_MODIFY event reported
> to parent "on child" is merged with the FAN_MODIFY event reported to
> child.
> 
> The new test case is a regression test for commit f367a62a7cad:
> 
>     fanotify: merge duplicate events on parent and child

The test looks OK but do we want a test for this? I mean: A test like this
seems to imply we promise to merge identical events. Although that is a
good general guideline, I consider it rather an optimization that may or
may not happen but userspace should not rely on it. Thoughts?

								Honza


> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
> Reviewed-by: Matthew Bobrowski <mbobrowski@mbobrowski.org>
> ---
>  .../kernel/syscalls/fanotify/fanotify15.c     | 81 +++++++++++++++++--
>  1 file changed, 73 insertions(+), 8 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/fanotify/fanotify15.c b/testcases/kernel/syscalls/fanotify/fanotify15.c
> index 2e860edb2..a9ed2ec81 100644
> --- a/testcases/kernel/syscalls/fanotify/fanotify15.c
> +++ b/testcases/kernel/syscalls/fanotify/fanotify15.c
> @@ -9,6 +9,10 @@
>   *	Test file that has been purposely designed to verify
>   *	FAN_REPORT_FID functionality while using newly defined dirent
>   *	events.
> + *
> + * Test case #1 is a regression test for commit f367a62a7cad:
> + *
> + *      fanotify: merge duplicate events on parent and child
>   */
>  #define _GNU_SOURCE
>  #include "config.h"
> @@ -53,28 +57,49 @@ static int fanotify_fd;
>  static char events_buf[EVENT_BUF_LEN];
>  static struct event_t event_set[EVENT_MAX];
>  
> -static void do_test(void)
> +static struct test_case_t {
> +	const char *tname;
> +	struct fanotify_mark_type mark;
> +	unsigned long mask;
> +} test_cases[] = {
> +	{
> +		"FAN_REPORT_FID on filesystem including FAN_DELETE_SELF",
> +		INIT_FANOTIFY_MARK_TYPE(FILESYSTEM),
> +		FAN_DELETE_SELF,
> +	},
> +	{
> +		"FAN_REPORT_FID on directory with FAN_EVENT_ON_CHILD",
> +		INIT_FANOTIFY_MARK_TYPE(INODE),
> +		FAN_EVENT_ON_CHILD,
> +	},
> +};
> +
> +static void do_test(unsigned int number)
>  {
>  	int i, fd, len, count = 0;
>  
>  	struct file_handle *event_file_handle;
>  	struct fanotify_event_metadata *metadata;
>  	struct fanotify_event_info_fid *event_fid;
> +	struct test_case_t *tc = &test_cases[number];
> +	struct fanotify_mark_type *mark = &tc->mark;
> +
> +	tst_res(TINFO, "Test #%d: %s", number, tc->tname);
>  
> -	if (fanotify_mark(fanotify_fd, FAN_MARK_ADD | FAN_MARK_FILESYSTEM,
> +	if (fanotify_mark(fanotify_fd, FAN_MARK_ADD | mark->flag, tc->mask |
>  				FAN_CREATE | FAN_DELETE | FAN_MOVE |
> -				FAN_MODIFY | FAN_DELETE_SELF | FAN_ONDIR,
> +				FAN_MODIFY | FAN_ONDIR,
>  				AT_FDCWD, TEST_DIR) == -1) {
>  		if (errno == ENODEV)
>  			tst_brk(TCONF,
>  				"FAN_REPORT_FID not supported on %s "
>  				"filesystem", tst_device->fs_type);
>  		tst_brk(TBROK | TERRNO,
> -			"fanotify_mark(%d, FAN_MARK_ADD | FAN_MARK_FILESYSTEM, "
> +			"fanotify_mark(%d, FAN_MARK_ADD | %s, "
>  			"FAN_CREATE | FAN_DELETE | FAN_MOVE | "
> -			"FAN_MODIFY | FAN_DELETE_SELF | FAN_ONDIR, "
> +			"FAN_MODIFY | FAN_ONDIR | 0x%lx, "
>  			"AT_FDCWD, %s) failed",
> -			fanotify_fd, TEST_DIR);
> +			fanotify_fd, mark->name, tc->mask, TEST_DIR);
>  	}
>  
>  	/* All dirent events on testdir are merged */
> @@ -87,8 +112,21 @@ static void do_test(void)
>  	fd = SAFE_CREAT(FILE1, 0644);
>  	SAFE_CLOSE(fd);
>  
> +	/* Recursive watch file for events "on self" */
> +	if (mark->flag == FAN_MARK_INODE &&
> +	    fanotify_mark(fanotify_fd, FAN_MARK_ADD | mark->flag,
> +			  FAN_MODIFY | FAN_DELETE_SELF,
> +			  AT_FDCWD, FILE1) == -1) {
> +		tst_brk(TBROK | TERRNO,
> +			"fanotify_mark(%d, FAN_MARK_ADD | %s, "
> +			"FAN_DELETE_SELF, AT_FDCWD, %s) failed",
> +			fanotify_fd, mark->name, FILE1);
> +	}
> +
>  	/*
>  	 * Event on child file is not merged with dirent events.
> +	 * FAN_MODIFY event reported on file mark should be merged with the
> +	 * FAN_MODIFY event reported on parent directory watch.
>  	 */
>  	event_set[count].mask = FAN_MODIFY;
>  	event_set[count].handle.handle_bytes = MAX_HANDLE_SZ;
> @@ -128,6 +166,17 @@ static void do_test(void)
>  
>  	SAFE_MKDIR(DIR1, 0755);
>  
> +	/* Recursive watch subdir for events "on self" */
> +	if (mark->flag == FAN_MARK_INODE &&
> +	    fanotify_mark(fanotify_fd, FAN_MARK_ADD | mark->flag,
> +			  FAN_DELETE_SELF | FAN_ONDIR,
> +			  AT_FDCWD, DIR1) == -1) {
> +		tst_brk(TBROK | TERRNO,
> +			"fanotify_mark(%d, FAN_MARK_ADD | %s,"
> +			"FAN_DELETE_SELF | FAN_ONDIR, AT_FDCWD, %s) failed",
> +			fanotify_fd, mark->name, DIR1);
> +	}
> +
>  	SAFE_RENAME(DIR1, DIR2);
>  
>  	event_set[count].mask = FAN_ONDIR | FAN_DELETE_SELF;
> @@ -141,6 +190,17 @@ static void do_test(void)
>  	/* Read dir events from the event queue */
>  	len += SAFE_READ(0, fanotify_fd, events_buf + len, EVENT_BUF_LEN - len);
>  
> +	/*
> +	 * Cleanup the mark
> +	 */
> +	if (fanotify_mark(fanotify_fd, FAN_MARK_FLUSH | mark->flag, 0,
> +			  AT_FDCWD, TEST_DIR) < 0) {
> +		tst_brk(TBROK | TERRNO,
> +			"fanotify_mark (%d, FAN_MARK_FLUSH | %s, 0,"
> +			"AT_FDCWD, '"TEST_DIR"') failed",
> +			fanotify_fd, mark->name);
> +	}
> +
>  	/* Process each event in buffer */
>  	for (i = 0, metadata = (struct fanotify_event_metadata *) events_buf;
>  		FAN_EVENT_OK(metadata, len); i++) {
> @@ -259,9 +319,14 @@ static struct tst_test test = {
>  	.mount_device = 1,
>  	.mntpoint = MOUNT_POINT,
>  	.all_filesystems = 1,
> -	.test_all = do_test,
> +	.test = do_test,
> +	.tcnt = ARRAY_SIZE(test_cases),
>  	.setup = do_setup,
> -	.cleanup = do_cleanup
> +	.cleanup = do_cleanup,
> +	.tags = (const struct tst_tag[]) {
> +		{"linux-git", "f367a62a7cad"},
> +		{}
> +	}
>  };
>  
>  #else
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[LTP] [PATCH v2 2/4] syscalls/fanotify15: Add a test case for inode marks

[Amir Goldstein]

On Mon, May 4, 2020 at 11:07 AM Jan Kara <jack@suse.cz> wrote:
>
> On Sat 02-05-20 19:27:42, Amir Goldstein wrote:
> > Test reporting events with fid also with recusrive inode marks:
> > - Test events "on self" (FAN_DELETE_SELF) on file and dir
> > - Test events "on child" (FAN_MODIFY) on file
> >
> > With recursive inode marks, verify that the FAN_MODIFY event reported
> > to parent "on child" is merged with the FAN_MODIFY event reported to
> > child.
> >
> > The new test case is a regression test for commit f367a62a7cad:
> >
> >     fanotify: merge duplicate events on parent and child
>
> The test looks OK but do we want a test for this? I mean: A test like this
> seems to imply we promise to merge identical events. Although that is a
> good general guideline, I consider it rather an optimization that may or
> may not happen but userspace should not rely on it. Thoughts?

The thing is, those are not really two identical events.
This is in fact the same event (fsnotify_change() hook was called once).
The fact that listener process may have an inode watch, parent directory
watch and a filesystem watch should not affect the number of read events.

Now it's true that internally, fsnotify_dentry() emits two event flavors to
parent and to victim. For inotify it even made some sense, because listener
would read two different event flavors with two different formats.
With fanotify (either reporting fd or fid) receiving two events makes very
little sense.

I agree that the fix (merging those events) is best effort and we cannot
commit to merging the events, but this isolated regression test does
check the best effort fix reliably and this is the reason I think it
should stay.

Upcoming FAN_REPORT_NAME is about to change the picture a bit
towards the inotify behavior - victim watch gets event without name,
parent watch gets event with name, filesystem watch gets both event
flavors... that is, if you will agree to this behavior, but we shall continue
this discussion on the fanotiify_name patches....

Thanks,
Amir.

[LTP] [PATCH v2 3/4] syscalls/fanotify: New test for FAN_MODIFY_DIR

[Petr Vorel]

Hi Amir,

while waiting for you and Jan to agree whether whole patchset should be merged
I have 2 fixes for v2.

> +static inline void fanotify_save_fid(const char *path,
> +				     struct fanotify_fid_t *fid)
> +{
> +	int *fh = (int *)(fid->handle.f_handle);
> +
> +	fh[0] = fh[1] = fh[2] = 0;
> +	fid->handle.handle_bytes = MAX_HANDLE_SZ;
> +	fanotify_get_fid(path, &fid->fsid, &fid->handle);
> +
> +	tst_res(TINFO,
> +		"fid(%s) = %x.%x.%x.%x.%x...", path,
> +		FSID_VAL_MEMBER(fid->fsid, 0), FSID_VAL_MEMBER(fid->fsid, 1),
> +		fh[0], fh[1], fh[2]);

We're using __kernel_fsid_t, which has val member on both libc and musl,
thus it must be:
-		"fid(%s) = %x.%x.%x.%x.%x...", path,
-		FSID_VAL_MEMBER(fid->fsid, 0), FSID_VAL_MEMBER(fid->fsid, 1),
-		fh[0], fh[1], fh[2]);
+		"fid(%s) = %x.%x.%x.%x.%x...", path, fid->fsid.val[0],
+		fid->fsid.val[1], fh[0], fh[1], fh[2]);

...
> +		} else if (memcmp(&event_fid->fsid, &expected->fid->fsid,
> +				  sizeof(event_fid->fsid)) != 0) {
> +			tst_res(TFAIL,
> +				"got event: mask=%llx pid=%u fd=%d name='%s' "
> +				"len=%d info_type=%d info_len=%d fh_len=%d "
> +				"fsid=%x.%x (expected %x.%x)",
> +				(unsigned long long)event->mask,
> +				(unsigned)event->pid, event->fd, filename,
> +				event->event_len, info_type,
> +				event_fid->hdr.len, fhlen,
> +				FSID_VAL_MEMBER(event_fid->fsid, 0),
> +				FSID_VAL_MEMBER(event_fid->fsid, 1),
> +				FSID_VAL_MEMBER(expected->fid->fsid, 0),
> +				FSID_VAL_MEMBER(expected->fid->fsid, 1));

Also here 3rd and 4th access must be direct as it is event_t:
-				FSID_VAL_MEMBER(expected->fid->fsid, 0),
-				FSID_VAL_MEMBER(expected->fid->fsid, 1));
+				expected->fid->fsid.val[0],
+				expected->fid->fsid.val[1]);

FYI FSID_VAL_MEMBER is only for event_fid. I'm sorry, although there is a note
in fanotify.h, it's a bit confusing (see f37704d6c fanotify: Fix
FSID_VAL_MEMBER() usage).

There is also warning on array size on newer compilers:

  378 |  sprintf(fname1, "%s/%s", dname1, FILE_NAME1);
      |                      ^~
In file included from /usr/include/stdio.h:867,
                 from fanotify16.c:13:
/usr/include/bits/stdio2.h:36:10: note: ?__builtin___sprintf_chk? output between 12 and 267 bytes into a destination of size 256
   36 |   return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   37 |       __bos (__s), __fmt, __va_arg_pack ());
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
fanotify16.c:379:22: warning: ?%s? directive writing 10 bytes into a region of size between 0 and 255 [-Wformat-overflow=]
  379 |  sprintf(fname2, "%s/%s", dname1, FILE_NAME2);
      |                      ^~

It can be fixed by increasing the size of fname1 and fname2:
-static char fname1[BUF_SIZE], fname2[BUF_SIZE];
+static char fname1[BUF_SIZE + 11], fname2[BUF_SIZE + 11];

I don't like that code but on the other hand don't like introducing warnings
either.

Kind regards,
Petr

[LTP] [PATCH v2 4/4] syscalls/fanotify: Use fanotify_save_fid() helper

[Petr Vorel]

Hi Amir,

> Reduce some boiler plate code in FAN_REPORT_FID tests and
> save fid only once per object instead of once per expected event.

Reviewed-by: Petr Vorel <pvorel@suse.cz>
+1

>  static int setup_marks(unsigned int fd, struct test_case_t *tc)
> @@ -130,8 +125,8 @@ static int setup_marks(unsigned int fd, struct test_case_t *tc)
>  					"kernel");
>  				return 1;
>  			} else if (errno == ENODEV &&
> -					!event_set[i].fsid.val[0] &&
> -					!event_set[i].fsid.val[1]) {
> +				   !FSID_VAL_MEMBER(objects[i].fid.fsid, 0) &&
> +				   !FSID_VAL_MEMBER(objects[i].fid.fsid, 1)) {

For the same reasons as the previous commits this diff is needed:
-				   !FSID_VAL_MEMBER(objects[i].fid.fsid, 0) &&
-				   !FSID_VAL_MEMBER(objects[i].fid.fsid, 1)) {
+				   !objects[i].fid.fsid.val[0] &&
+				   !objects[i].fid.fsid.val[1]) {


Kind regards,
Petr

[LTP] [PATCH v2 2/4] syscalls/fanotify15: Add a test case for inode marks

[Jan Kara]

On Mon 04-05-20 11:51:27, Amir Goldstein wrote:
> On Mon, May 4, 2020 at 11:07 AM Jan Kara <jack@suse.cz> wrote:
> >
> > On Sat 02-05-20 19:27:42, Amir Goldstein wrote:
> > > Test reporting events with fid also with recusrive inode marks:
> > > - Test events "on self" (FAN_DELETE_SELF) on file and dir
> > > - Test events "on child" (FAN_MODIFY) on file
> > >
> > > With recursive inode marks, verify that the FAN_MODIFY event reported
> > > to parent "on child" is merged with the FAN_MODIFY event reported to
> > > child.
> > >
> > > The new test case is a regression test for commit f367a62a7cad:
> > >
> > >     fanotify: merge duplicate events on parent and child
> >
> > The test looks OK but do we want a test for this? I mean: A test like this
> > seems to imply we promise to merge identical events. Although that is a
> > good general guideline, I consider it rather an optimization that may or
> > may not happen but userspace should not rely on it. Thoughts?
> 
> The thing is, those are not really two identical events.
> This is in fact the same event (fsnotify_change() hook was called once).
> The fact that listener process may have an inode watch, parent directory
> watch and a filesystem watch should not affect the number of read events.

Yeah, I agree that in this case we should be merging the event if sanely
possible (which is why I've merged that patch).

> Now it's true that internally, fsnotify_dentry() emits two event flavors to
> parent and to victim. For inotify it even made some sense, because listener
> would read two different event flavors with two different formats.
> With fanotify (either reporting fd or fid) receiving two events makes very
> little sense.
> 
> I agree that the fix (merging those events) is best effort and we cannot
> commit to merging the events, but this isolated regression test does
> check the best effort fix reliably and this is the reason I think it
> should stay.

OK, I'm not too concerned about this test. But still the functionality is
more in the area of "nice to have" than "must have" so in future we may
break this if the implementation would get too hairy. But I guess we can
remove the test in that case.

> Upcoming FAN_REPORT_NAME is about to change the picture a bit
> towards the inotify behavior - victim watch gets event without name,
> parent watch gets event with name, filesystem watch gets both event
> flavors... that is, if you will agree to this behavior, but we shall continue
> this discussion on the fanotiify_name patches....

Yes.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[LTP] [PATCH v2 3/4] syscalls/fanotify: New test for FAN_MODIFY_DIR

[Amir Goldstein]

On Mon, May 4, 2020 at 3:07 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi Amir,
>
> while waiting for you and Jan to agree whether whole patchset should be merged
> I have 2 fixes for v2.
>
> > +static inline void fanotify_save_fid(const char *path,
> > +                                  struct fanotify_fid_t *fid)
> > +{
> > +     int *fh = (int *)(fid->handle.f_handle);
> > +
> > +     fh[0] = fh[1] = fh[2] = 0;
> > +     fid->handle.handle_bytes = MAX_HANDLE_SZ;
> > +     fanotify_get_fid(path, &fid->fsid, &fid->handle);
> > +
> > +     tst_res(TINFO,
> > +             "fid(%s) = %x.%x.%x.%x.%x...", path,
> > +             FSID_VAL_MEMBER(fid->fsid, 0), FSID_VAL_MEMBER(fid->fsid, 1),
> > +             fh[0], fh[1], fh[2]);
>
> We're using __kernel_fsid_t, which has val member on both libc and musl,
> thus it must be:
> -               "fid(%s) = %x.%x.%x.%x.%x...", path,
> -               FSID_VAL_MEMBER(fid->fsid, 0), FSID_VAL_MEMBER(fid->fsid, 1),
> -               fh[0], fh[1], fh[2]);
> +               "fid(%s) = %x.%x.%x.%x.%x...", path, fid->fsid.val[0],
> +               fid->fsid.val[1], fh[0], fh[1], fh[2]);
>

Sure. I didn't get the whole FSID_VAL_MEMBER thing correctly.
Whatever works on all platforms.
Its just an info message anyway.

> ...
> > +             } else if (memcmp(&event_fid->fsid, &expected->fid->fsid,
> > +                               sizeof(event_fid->fsid)) != 0) {
> > +                     tst_res(TFAIL,
> > +                             "got event: mask=%llx pid=%u fd=%d name='%s' "
> > +                             "len=%d info_type=%d info_len=%d fh_len=%d "
> > +                             "fsid=%x.%x (expected %x.%x)",
> > +                             (unsigned long long)event->mask,
> > +                             (unsigned)event->pid, event->fd, filename,
> > +                             event->event_len, info_type,
> > +                             event_fid->hdr.len, fhlen,
> > +                             FSID_VAL_MEMBER(event_fid->fsid, 0),
> > +                             FSID_VAL_MEMBER(event_fid->fsid, 1),
> > +                             FSID_VAL_MEMBER(expected->fid->fsid, 0),
> > +                             FSID_VAL_MEMBER(expected->fid->fsid, 1));
>
> Also here 3rd and 4th access must be direct as it is event_t:
> -                               FSID_VAL_MEMBER(expected->fid->fsid, 0),
> -                               FSID_VAL_MEMBER(expected->fid->fsid, 1));
> +                               expected->fid->fsid.val[0],
> +                               expected->fid->fsid.val[1]);
>
> FYI FSID_VAL_MEMBER is only for event_fid. I'm sorry, although there is a note
> in fanotify.h, it's a bit confusing (see f37704d6c fanotify: Fix
> FSID_VAL_MEMBER() usage).
>
> There is also warning on array size on newer compilers:
>
>   378 |  sprintf(fname1, "%s/%s", dname1, FILE_NAME1);
>       |                      ^~
> In file included from /usr/include/stdio.h:867,
>                  from fanotify16.c:13:
> /usr/include/bits/stdio2.h:36:10: note: ?__builtin___sprintf_chk? output between 12 and 267 bytes into a destination of size 256
>    36 |   return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
>       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    37 |       __bos (__s), __fmt, __va_arg_pack ());
>       |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> fanotify16.c:379:22: warning: ?%s? directive writing 10 bytes into a region of size between 0 and 255 [-Wformat-overflow=]
>   379 |  sprintf(fname2, "%s/%s", dname1, FILE_NAME2);
>       |                      ^~
>
> It can be fixed by increasing the size of fname1 and fname2:
> -static char fname1[BUF_SIZE], fname2[BUF_SIZE];
> +static char fname1[BUF_SIZE + 11], fname2[BUF_SIZE + 11];
>
> I don't like that code but on the other hand don't like introducing warnings
> either.

Whatever works for you is fine by me.

Thanks,
Amir.

[LTP] [PATCH v2 2/4] syscalls/fanotify15: Add a test case for inode marks

[Petr Vorel]

Hi Jan,

...
> > > > The new test case is a regression test for commit f367a62a7cad:

> > > >     fanotify: merge duplicate events on parent and child

> > > The test looks OK but do we want a test for this? I mean: A test like this
> > > seems to imply we promise to merge identical events. Although that is a
> > > good general guideline, I consider it rather an optimization that may or
> > > may not happen but userspace should not rely on it. Thoughts?

> > The thing is, those are not really two identical events.
> > This is in fact the same event (fsnotify_change() hook was called once).
> > The fact that listener process may have an inode watch, parent directory
> > watch and a filesystem watch should not affect the number of read events.

> Yeah, I agree that in this case we should be merging the event if sanely
> possible (which is why I've merged that patch).

> > Now it's true that internally, fsnotify_dentry() emits two event flavors to
> > parent and to victim. For inotify it even made some sense, because listener
> > would read two different event flavors with two different formats.
> > With fanotify (either reporting fd or fid) receiving two events makes very
> > little sense.

> > I agree that the fix (merging those events) is best effort and we cannot
> > commit to merging the events, but this isolated regression test does
> > check the best effort fix reliably and this is the reason I think it
> > should stay.

> OK, I'm not too concerned about this test. But still the functionality is
> more in the area of "nice to have" than "must have" so in future we may
> break this if the implementation would get too hairy. But I guess we can
> remove the test in that case.
Yes, nothing is set in stone.

> > Upcoming FAN_REPORT_NAME is about to change the picture a bit
> > towards the inotify behavior - victim watch gets event without name,
> > parent watch gets event with name, filesystem watch gets both event
> > flavors... that is, if you will agree to this behavior, but we shall continue
> > this discussion on the fanotiify_name patches....

> Yes.
Can I add your ack tag to the whole patchset?
Or do you still consider whether any of them should be merged?

Kind regards,
Petr

> 								Honza

[LTP] [PATCH v2 2/4] syscalls/fanotify15: Add a test case for inode marks

[Jan Kara]

On Mon 04-05-20 20:49:36, Petr Vorel wrote:
> Hi Jan,
> 
> ...
> > > > > The new test case is a regression test for commit f367a62a7cad:
> 
> > > > >     fanotify: merge duplicate events on parent and child
> 
> > > > The test looks OK but do we want a test for this? I mean: A test like this
> > > > seems to imply we promise to merge identical events. Although that is a
> > > > good general guideline, I consider it rather an optimization that may or
> > > > may not happen but userspace should not rely on it. Thoughts?
> 
> > > The thing is, those are not really two identical events.
> > > This is in fact the same event (fsnotify_change() hook was called once).
> > > The fact that listener process may have an inode watch, parent directory
> > > watch and a filesystem watch should not affect the number of read events.
> 
> > Yeah, I agree that in this case we should be merging the event if sanely
> > possible (which is why I've merged that patch).
> 
> > > Now it's true that internally, fsnotify_dentry() emits two event flavors to
> > > parent and to victim. For inotify it even made some sense, because listener
> > > would read two different event flavors with two different formats.
> > > With fanotify (either reporting fd or fid) receiving two events makes very
> > > little sense.
> 
> > > I agree that the fix (merging those events) is best effort and we cannot
> > > commit to merging the events, but this isolated regression test does
> > > check the best effort fix reliably and this is the reason I think it
> > > should stay.
> 
> > OK, I'm not too concerned about this test. But still the functionality is
> > more in the area of "nice to have" than "must have" so in future we may
> > break this if the implementation would get too hairy. But I guess we can
> > remove the test in that case.
> Yes, nothing is set in stone.
> 
> > > Upcoming FAN_REPORT_NAME is about to change the picture a bit
> > > towards the inotify behavior - victim watch gets event without name,
> > > parent watch gets event with name, filesystem watch gets both event
> > > flavors... that is, if you will agree to this behavior, but we shall continue
> > > this discussion on the fanotiify_name patches....
> 
> > Yes.
> Can I add your ack tag to the whole patchset?
> Or do you still consider whether any of them should be merged?

Feel free to add my Ack-by tag:

Acked-by: Jan Kara <jack@suse.cz>

I didn't check all the details but overall the patches look good to me.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[LTP] [PATCH v2 2/4] syscalls/fanotify15: Add a test case for inode marks

[Petr Vorel]

Hi all,

> > Can I add your ack tag to the whole patchset?
> > Or do you still consider whether any of them should be merged?

> Feel free to add my Ack-by tag:

> Acked-by: Jan Kara <jack@suse.cz>

> I didn't check all the details but overall the patches look good to me.
Merged with that few small fixes we discussed.
Thanks for your work!

> 								Honza

Kind regards,
Petr