[LTP] [PATCH] syscalls/fanotify: misc cleanups

[LTP] [PATCH] syscalls/fanotify: misc cleanups

[Amir Goldstein]

* Cleanup backup file descriptor in fanotify03

* Fix whitespace and indentation

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Hi Cyril,

I am working on some tests for a new fanotify feature, so went on
a cleaning spree before forking some tests.

Thanks,
Amir.

 .../kernel/syscalls/fanotify/fanotify01.c     | 118 +++++++++---------
 .../kernel/syscalls/fanotify/fanotify02.c     |  61 +++++----
 .../kernel/syscalls/fanotify/fanotify03.c     |  56 +++++----
 .../kernel/syscalls/fanotify/fanotify04.c     |  54 ++++----
 .../kernel/syscalls/fanotify/fanotify05.c     |  38 +++---
 .../kernel/syscalls/fanotify/fanotify06.c     |  58 ++++-----
 .../kernel/syscalls/fanotify/fanotify09.c     |  46 +++----
 7 files changed, 216 insertions(+), 215 deletions(-)

diff --git a/testcases/kernel/syscalls/fanotify/fanotify01.c b/testcases/kernel/syscalls/fanotify/fanotify01.c
index ce08f3a58..cee9b34ea 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify01.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify01.c
@@ -65,11 +65,11 @@ void test01(void)
 	int tst_count = 0;
 
 	if (fanotify_mark(fd_notify, FAN_MARK_ADD, FAN_ACCESS | FAN_MODIFY |
-			    FAN_CLOSE | FAN_OPEN, AT_FDCWD, fname) < 0) {
+			  FAN_CLOSE | FAN_OPEN, AT_FDCWD, fname) < 0) {
 		tst_brk(TBROK | TERRNO,
-		    "fanotify_mark (%d, FAN_MARK_ADD, FAN_ACCESS | "
-		    "FAN_MODIFY | FAN_CLOSE | FAN_OPEN, AT_FDCWD, %s) "
-		    "failed", fd_notify, fname);
+			"fanotify_mark (%d, FAN_MARK_ADD, FAN_ACCESS | "
+			"FAN_MODIFY | FAN_CLOSE | FAN_OPEN, AT_FDCWD, %s) "
+			"failed", fd_notify, fname);
 	}
 
 	/*
@@ -119,12 +119,12 @@ void test01(void)
 
 	/* Ignore access events */
 	if (fanotify_mark(fd_notify,
-			    FAN_MARK_ADD | FAN_MARK_IGNORED_MASK,
-			    FAN_ACCESS, AT_FDCWD, fname) < 0) {
+			  FAN_MARK_ADD | FAN_MARK_IGNORED_MASK,
+			  FAN_ACCESS, AT_FDCWD, fname) < 0) {
 		tst_brk(TBROK | TERRNO,
-		     "fanotify_mark (%d, FAN_MARK_ADD | "
-		     "FAN_MARK_IGNORED_MASK, FAN_ACCESS, "
-		     "AT_FDCWD, %s) failed", fd_notify, fname);
+			"fanotify_mark (%d, FAN_MARK_ADD | "
+			"FAN_MARK_IGNORED_MASK, FAN_ACCESS, "
+			"AT_FDCWD, %s) failed", fd_notify, fname);
 	}
 
 	fd = SAFE_OPEN(fname, O_RDWR);
@@ -168,15 +168,15 @@ void test01(void)
 	 * Now ignore open & close events regardless of file
 	 * modifications
 	 */
-	if (fanotify_mark(fd_notify,
-			    FAN_MARK_ADD | FAN_MARK_IGNORED_MASK | FAN_MARK_IGNORED_SURV_MODIFY,
-			    FAN_OPEN | FAN_CLOSE, AT_FDCWD, fname) < 0) {
+	if (fanotify_mark(fd_notify, FAN_MARK_ADD |
+			  FAN_MARK_IGNORED_MASK | FAN_MARK_IGNORED_SURV_MODIFY,
+			  FAN_OPEN | FAN_CLOSE, AT_FDCWD, fname) < 0) {
 		tst_brk(TBROK | TERRNO,
-		     "fanotify_mark (%d, FAN_MARK_ADD | "
-		     "FAN_MARK_IGNORED_MASK | "
-		     "FAN_MARK_IGNORED_SURV_MODIFY, FAN_OPEN | "
-		     "FAN_CLOSE, AT_FDCWD, %s) failed", fd_notify,
-		     fname);
+			"fanotify_mark (%d, FAN_MARK_ADD | "
+			"FAN_MARK_IGNORED_MASK | "
+			"FAN_MARK_IGNORED_SURV_MODIFY, FAN_OPEN | "
+			"FAN_CLOSE, AT_FDCWD, %s) failed", fd_notify,
+			fname);
 	}
 
 	/* This event should be ignored */
@@ -199,13 +199,13 @@ void test01(void)
 
 	/* Now remove open and close from ignored mask */
 	if (fanotify_mark(fd_notify,
-			    FAN_MARK_REMOVE | FAN_MARK_IGNORED_MASK,
-			    FAN_OPEN | FAN_CLOSE, AT_FDCWD, fname) < 0) {
+			  FAN_MARK_REMOVE | FAN_MARK_IGNORED_MASK,
+			  FAN_OPEN | FAN_CLOSE, AT_FDCWD, fname) < 0) {
 		tst_brk(TBROK | TERRNO,
-		     "fanotify_mark (%d, FAN_MARK_REMOVE | "
-		     "FAN_MARK_IGNORED_MASK, FAN_OPEN | "
-		     "FAN_CLOSE, AT_FDCWD, %s) failed", fd_notify,
-		     fname);
+			"fanotify_mark (%d, FAN_MARK_REMOVE | "
+			"FAN_MARK_IGNORED_MASK, FAN_OPEN | "
+			"FAN_CLOSE, AT_FDCWD, %s) failed", fd_notify,
+			fname);
 	}
 
 	SAFE_CLOSE(fd);
@@ -219,8 +219,8 @@ void test01(void)
 
 	if (TST_TOTAL != tst_count) {
 		tst_brk(TBROK,
-			 "TST_TOTAL (%d) and tst_count (%d) are not "
-			 "equal", TST_TOTAL, tst_count);
+			"TST_TOTAL (%d) and tst_count (%d) are not "
+			"equal", TST_TOTAL, tst_count);
 	}
 	tst_count = 0;
 
@@ -233,51 +233,51 @@ void test01(void)
 		event = (struct fanotify_event_metadata *)&event_buf[i];
 		if (test_num >= TST_TOTAL) {
 			tst_res(TFAIL,
-				 "get unnecessary event: mask=%llx "
-				 "pid=%u fd=%u",
-				 (unsigned long long)event->mask,
-				 (unsigned)event->pid, event->fd);
+				"get unnecessary event: mask=%llx "
+				"pid=%u fd=%u",
+				(unsigned long long)event->mask,
+				(unsigned)event->pid, event->fd);
 		} else if (!(event->mask & event_set[test_num])) {
 			tst_res(TFAIL,
-				 "get event: mask=%llx (expected %llx) "
-				 "pid=%u fd=%u",
-				 (unsigned long long)event->mask,
-				 event_set[test_num],
-				 (unsigned)event->pid, event->fd);
+				"get event: mask=%llx (expected %llx) "
+				"pid=%u fd=%u",
+				(unsigned long long)event->mask,
+				event_set[test_num],
+				(unsigned)event->pid, event->fd);
 		} else if (event->pid != getpid()) {
 			tst_res(TFAIL,
-				 "get event: mask=%llx pid=%u "
-				 "(expected %u) fd=%u",
-				 (unsigned long long)event->mask,
-				 (unsigned)event->pid,
-				 (unsigned)getpid(),
-				 event->fd);
+				"get event: mask=%llx pid=%u "
+				"(expected %u) fd=%u",
+				(unsigned long long)event->mask,
+				(unsigned)event->pid,
+				(unsigned)getpid(),
+				event->fd);
 		} else {
 			if (event->fd == -2)
 				goto pass;
 			ret = read(event->fd, buf, BUF_SIZE);
 			if (ret != (int)strlen(fname)) {
 				tst_res(TFAIL,
-					 "cannot read from returned fd "
-					 "of event: mask=%llx pid=%u "
-					 "fd=%u ret=%d (errno=%d)",
-					 (unsigned long long)event->mask,
-					 (unsigned)event->pid,
-					 event->fd, ret, errno);
+					"cannot read from returned fd "
+					"of event: mask=%llx pid=%u "
+					"fd=%u ret=%d (errno=%d)",
+					(unsigned long long)event->mask,
+					(unsigned)event->pid,
+					event->fd, ret, errno);
 			} else if (memcmp(buf, fname, strlen(fname))) {
 				tst_res(TFAIL,
-					 "wrong data read from returned fd "
-					 "of event: mask=%llx pid=%u "
-					 "fd=%u",
-					 (unsigned long long)event->mask,
-					 (unsigned)event->pid,
-					 event->fd);
+					"wrong data read from returned fd "
+					"of event: mask=%llx pid=%u "
+					"fd=%u",
+					(unsigned long long)event->mask,
+					(unsigned)event->pid,
+					event->fd);
 			} else {
 pass:
 				tst_res(TPASS,
-				    "get event: mask=%llx pid=%u fd=%u",
-				    (unsigned long long)event->mask,
-				    (unsigned)event->pid, event->fd);
+					"get event: mask=%llx pid=%u fd=%u",
+					(unsigned long long)event->mask,
+					(unsigned)event->pid, event->fd);
 			}
 		}
 		/*
@@ -297,16 +297,16 @@ pass:
 	}
 	for (; test_num < TST_TOTAL; test_num++) {
 		tst_res(TFAIL, "didn't get event: mask=%llx",
-			 event_set[test_num]);
+			event_set[test_num]);
 
 	}
 	/* Remove mark to clear FAN_MARK_IGNORED_SURV_MODIFY */
 	if (fanotify_mark(fd_notify, FAN_MARK_REMOVE, FAN_ACCESS | FAN_MODIFY |
 			    FAN_CLOSE | FAN_OPEN, AT_FDCWD, fname) < 0) {
 		tst_brk(TBROK | TERRNO,
-		    "fanotify_mark (%d, FAN_MARK_REMOVE, FAN_ACCESS | "
-		    "FAN_MODIFY | FAN_CLOSE | FAN_OPEN, AT_FDCWD, %s) "
-		    "failed", fd_notify, fname);
+			"fanotify_mark (%d, FAN_MARK_REMOVE, FAN_ACCESS | "
+			"FAN_MODIFY | FAN_CLOSE | FAN_OPEN, AT_FDCWD, %s) "
+			"failed", fd_notify, fname);
 	}
 }
 
diff --git a/testcases/kernel/syscalls/fanotify/fanotify02.c b/testcases/kernel/syscalls/fanotify/fanotify02.c
index 215e33b9d..3c232739a 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify02.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify02.c
@@ -65,14 +65,14 @@ void test01(void)
 	int tst_count = 0;
 
 	if (fanotify_mark(fd_notify, FAN_MARK_ADD, FAN_ACCESS |
-			    FAN_MODIFY | FAN_CLOSE | FAN_OPEN |
-			    FAN_EVENT_ON_CHILD | FAN_ONDIR, AT_FDCWD,
+			  FAN_MODIFY | FAN_CLOSE | FAN_OPEN |
+			  FAN_EVENT_ON_CHILD | FAN_ONDIR, AT_FDCWD,
 			  ".") < 0) {
 		tst_brk(TBROK | TERRNO,
-		    "fanotify_mark (%d, FAN_MARK_ADD, FAN_ACCESS | "
-		    "FAN_MODIFY | FAN_CLOSE | FAN_OPEN | "
-		    "FAN_EVENT_ON_CHILD | FAN_ONDIR, AT_FDCWD, '.') "
-		    "failed", fd_notify);
+			"fanotify_mark (%d, FAN_MARK_ADD, FAN_ACCESS | "
+			"FAN_MODIFY | FAN_CLOSE | FAN_OPEN | "
+			"FAN_EVENT_ON_CHILD | FAN_ONDIR, AT_FDCWD, '.') "
+			"failed", fd_notify);
 	}
 
 	/*
@@ -121,11 +121,11 @@ void test01(void)
 	 * now remove child mark
 	 */
 	if (fanotify_mark(fd_notify, FAN_MARK_REMOVE,
-			    FAN_EVENT_ON_CHILD, AT_FDCWD, ".") < 0) {
+			  FAN_EVENT_ON_CHILD, AT_FDCWD, ".") < 0) {
 		tst_brk(TBROK | TERRNO,
-		    "fanotify_mark (%d, FAN_MARK REMOVE, "
-		    "FAN_EVENT_ON_CHILD, AT_FDCWD, '.') failed",
-		    fd_notify);
+			"fanotify_mark (%d, FAN_MARK REMOVE, "
+			"FAN_EVENT_ON_CHILD, AT_FDCWD, '.') failed",
+			fd_notify);
 	}
 
 	/*
@@ -151,8 +151,7 @@ void test01(void)
 	len += ret;
 
 	if (TST_TOTAL != tst_count) {
-		tst_brk(TBROK,
-			 "TST_TOTAL and tst_count are not equal");
+		tst_brk(TBROK, "TST_TOTAL and tst_count are not equal");
 	}
 	tst_count = 0;
 
@@ -165,30 +164,30 @@ void test01(void)
 		event = (struct fanotify_event_metadata *)&event_buf[i];
 		if (test_num >= TST_TOTAL) {
 			tst_res(TFAIL,
-				 "get unnecessary event: mask=%llx "
-				 "pid=%u fd=%u",
-				 (unsigned long long)event->mask,
-				 (unsigned)event->pid, event->fd);
+				"get unnecessary event: mask=%llx "
+				"pid=%u fd=%u",
+				(unsigned long long)event->mask,
+				(unsigned)event->pid, event->fd);
 		} else if (!(event->mask & event_set[test_num])) {
 			tst_res(TFAIL,
-				 "get event: mask=%llx (expected %llx) "
-				 "pid=%u fd=%u",
-				 (unsigned long long)event->mask,
-				 event_set[test_num],
-				 (unsigned)event->pid, event->fd);
+				"get event: mask=%llx (expected %llx) "
+				"pid=%u fd=%u",
+				(unsigned long long)event->mask,
+				event_set[test_num],
+				(unsigned)event->pid, event->fd);
 		} else if (event->pid != getpid()) {
 			tst_res(TFAIL,
-				 "get event: mask=%llx pid=%u "
-				 "(expected %u) fd=%u",
-				 (unsigned long long)event->mask,
-				 (unsigned)event->pid,
-				 (unsigned)getpid(),
-				 event->fd);
+				"get event: mask=%llx pid=%u "
+				"(expected %u) fd=%u",
+				(unsigned long long)event->mask,
+				(unsigned)event->pid,
+				(unsigned)getpid(),
+				event->fd);
 		} else {
 			tst_res(TPASS,
-				    "get event: mask=%llx pid=%u fd=%u",
-				    (unsigned long long)event->mask,
-				    (unsigned)event->pid, event->fd);
+				"get event: mask=%llx pid=%u fd=%u",
+				(unsigned long long)event->mask,
+				(unsigned)event->pid, event->fd);
 		}
 		event->mask &= ~event_set[test_num];
 		/* No events left in current mask? Go for next event */
@@ -201,7 +200,7 @@ void test01(void)
 	}
 	for (; test_num < TST_TOTAL; test_num++) {
 		tst_res(TFAIL, "didn't get event: mask=%llx",
-			 event_set[test_num]);
+			event_set[test_num]);
 
 	}
 }
diff --git a/testcases/kernel/syscalls/fanotify/fanotify03.c b/testcases/kernel/syscalls/fanotify/fanotify03.c
index 83cd26640..a37fb750b 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify03.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify03.c
@@ -57,6 +57,7 @@
 static char fname[BUF_SIZE];
 static char buf[BUF_SIZE];
 static volatile int fd_notify;
+static int fd_notify_backup = -1;
 
 static pid_t child_pid;
 
@@ -106,7 +107,7 @@ static void run_child(void)
 
 	if (sigaction(SIGCHLD, &child_action, NULL) < 0) {
 		tst_brk(TBROK | TERRNO,
-			 "sigaction(SIGCHLD, &child_action, NULL) failed");
+			"sigaction(SIGCHLD, &child_action, NULL) failed");
 	}
 
 	child_pid = SAFE_FORK();
@@ -128,7 +129,7 @@ static void check_child(void)
 	child_action.sa_flags = SA_NOCLDSTOP;
 	if (sigaction(SIGCHLD, &child_action, NULL) < 0) {
 		tst_brk(TBROK | TERRNO,
-			 "sigaction(SIGCHLD, &child_action, NULL) failed");
+			"sigaction(SIGCHLD, &child_action, NULL) failed");
 	}
 	SAFE_WAITPID(-1, &child_ret, 0);
 
@@ -140,7 +141,7 @@ static void check_child(void)
 
 void test01(void)
 {
-	int tst_count, fd_notify_backup = -1;
+	int tst_count;
 
 	int ret, len = 0, i = 0, test_num = 0;
 
@@ -158,8 +159,7 @@ void test01(void)
 
 	/* tst_count + 1 is for checking child return value */
 	if (TST_TOTAL != tst_count + 1) {
-		tst_brk(TBROK,
-			 "TST_TOTAL and tst_count do not match");
+		tst_brk(TBROK, "TST_TOTAL and tst_count do not match");
 	}
 	tst_count = 0;
 
@@ -177,8 +177,8 @@ void test01(void)
 				break;
 			if (ret < 0) {
 				tst_brk(TBROK,
-					 "read(%d, buf, %zu) failed",
-					 fd_notify, EVENT_BUF_LEN);
+					"read(%d, buf, %zu) failed",
+					fd_notify, EVENT_BUF_LEN);
 			}
 			len += ret;
 		}
@@ -186,24 +186,24 @@ void test01(void)
 		event = (struct fanotify_event_metadata *)&event_buf[i];
 		if (!(event->mask & event_set[test_num])) {
 			tst_res(TFAIL,
-				 "get event: mask=%llx (expected %llx) "
-				 "pid=%u fd=%u",
-				 (unsigned long long)event->mask,
-				 event_set[test_num],
-				 (unsigned)event->pid, event->fd);
+				"get event: mask=%llx (expected %llx) "
+				"pid=%u fd=%u",
+				(unsigned long long)event->mask,
+				event_set[test_num],
+				(unsigned)event->pid, event->fd);
 		} else if (event->pid != child_pid) {
 			tst_res(TFAIL,
-				 "get event: mask=%llx pid=%u "
-				 "(expected %u) fd=%u",
-				 (unsigned long long)event->mask,
-				 (unsigned)event->pid,
-				 (unsigned)child_pid,
-				 event->fd);
+				"get event: mask=%llx pid=%u "
+				"(expected %u) fd=%u",
+				(unsigned long long)event->mask,
+				(unsigned)event->pid,
+				(unsigned)child_pid,
+				event->fd);
 		} else {
 			tst_res(TPASS,
-				    "get event: mask=%llx pid=%u fd=%u",
-				    (unsigned long long)event->mask,
-				    (unsigned)event->pid, event->fd);
+				"get event: mask=%llx pid=%u fd=%u",
+				(unsigned long long)event->mask,
+				(unsigned)event->pid, event->fd);
 		}
 		/* Write response to permission event */
 		if (event_set[test_num] & FAN_ALL_PERM_EVENTS) {
@@ -225,7 +225,7 @@ void test01(void)
 	}
 	for (; test_num < TST_TOTAL - 1; test_num++) {
 		tst_res(TFAIL, "didn't get event: mask=%llx",
-			 event_set[test_num]);
+			event_set[test_num]);
 
 	}
 	check_child();
@@ -244,15 +244,15 @@ static void setup(void)
 	fd_notify = SAFE_FANOTIFY_INIT(FAN_CLASS_CONTENT, O_RDONLY);
 
 	if (fanotify_mark(fd_notify, FAN_MARK_ADD, FAN_ACCESS_PERM |
-			    FAN_OPEN_PERM, AT_FDCWD, fname) < 0) {
+			  FAN_OPEN_PERM, AT_FDCWD, fname) < 0) {
 		if (errno == EINVAL) {
 			tst_brk(TCONF | TERRNO,
-				 "CONFIG_FANOTIFY_ACCESS_PERMISSIONS not "
-				 "configured in kernel?");
+				"CONFIG_FANOTIFY_ACCESS_PERMISSIONS not "
+				"configured in kernel?");
 		} else {
 			tst_brk(TBROK | TERRNO,
-				 "fanotify_mark (%d, FAN_MARK_ADD, FAN_ACCESS_PERM | "
-				 "FAN_OPEN_PERM, AT_FDCWD, %s) failed.", fd_notify, fname);
+				"fanotify_mark (%d, FAN_MARK_ADD, FAN_ACCESS_PERM | "
+				"FAN_OPEN_PERM, AT_FDCWD, %s) failed.", fd_notify, fname);
 		}
 	}
 
@@ -262,6 +262,8 @@ static void cleanup(void)
 {
 	if (fd_notify > 0)
 		SAFE_CLOSE(fd_notify);
+	if (fd_notify_backup > 0)
+		SAFE_CLOSE(fd_notify_backup);
 }
 
 static struct tst_test test = {
diff --git a/testcases/kernel/syscalls/fanotify/fanotify04.c b/testcases/kernel/syscalls/fanotify/fanotify04.c
index 6713ff609..fb932aeb1 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify04.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify04.c
@@ -77,14 +77,14 @@ static void check_mark(char *file, unsigned long long flag, char *flagstr,
 		       int expect, void (*test_event)(char *))
 {
 	if (fanotify_mark(fd_notify, FAN_MARK_ADD | flag, FAN_OPEN, AT_FDCWD,
-			    file) != expect) {
-		tst_res(TFAIL,
-		    "fanotify_mark (%d, FAN_MARK_ADD | %s, FAN_OPEN, AT_FDCWD, "
-		    "'%s') %s", fd_notify, flagstr, file, expect_str_fail(expect));
+			  file) != expect) {
+		tst_res(TFAIL, "fanotify_mark (%d, FAN_MARK_ADD | %s, "
+			"FAN_OPEN, AT_FDCWD, '%s') %s",
+			fd_notify, flagstr, file, expect_str_fail(expect));
 	} else {
-		tst_res(TPASS,
-		    "fanotify_mark (%d, FAN_MARK_ADD | %s, FAN_OPEN, AT_FDCWD, "
-		    "'%s') %s", fd_notify, flagstr, file, expect_str_pass(expect));
+		tst_res(TPASS, "fanotify_mark (%d, FAN_MARK_ADD | %s, "
+			"FAN_OPEN, AT_FDCWD, '%s') %s",
+			fd_notify, flagstr, file, expect_str_pass(expect));
 
 		/* If we expected failure there's nothing to clean up */
 		if (expect == -1)
@@ -94,11 +94,11 @@ static void check_mark(char *file, unsigned long long flag, char *flagstr,
 			test_event(file);
 
 		if (fanotify_mark(fd_notify, FAN_MARK_REMOVE | flag,
-				    FAN_OPEN, AT_FDCWD, file) < 0) {
+				  FAN_OPEN, AT_FDCWD, file) < 0) {
 			tst_brk(TBROK | TERRNO,
-			    "fanotify_mark (%d, FAN_MARK_REMOVE | %s, "
-			    "FAN_OPEN, AT_FDCWD, '%s') failed",
-			    fd_notify, flagstr, file);
+				"fanotify_mark (%d, FAN_MARK_REMOVE | %s, "
+				"FAN_OPEN, AT_FDCWD, '%s') failed",
+				fd_notify, flagstr, file);
 		}
 	}
 }
@@ -137,13 +137,13 @@ static void verify_event(int mask)
 
 	if (event->mask != FAN_OPEN) {
 		tst_res(TFAIL, "got unexpected event %llx",
-			 (unsigned long long)event->mask);
+			(unsigned long long)event->mask);
 	} else if (fstat(event->fd, &st) < 0) {
 		tst_res(TFAIL, "failed to stat event->fd (%s)",
-			 strerror(errno));
+			strerror(errno));
 	} else if ((int)(st.st_mode & S_IFMT) != mask) {
 		tst_res(TFAIL, "event->fd points to object of different type "
-			 "(%o != %o)", st.st_mode & S_IFMT, mask);
+			"(%o != %o)", st.st_mode & S_IFMT, mask);
 	} else {
 		tst_res(TPASS, "event generated properly for type %o", mask);
 	}
@@ -173,13 +173,13 @@ static void verify_no_event(void)
 
 		event = (struct fanotify_event_metadata *)&event_buf[len];
 		tst_res(TFAIL, "seen unexpected event (mask %llx)",
-			 (unsigned long long)event->mask);
+			(unsigned long long)event->mask);
 		/* Cleanup fd from the event */
 		if (event->fd != FAN_NOFD)
 			SAFE_CLOSE(event->fd);
 	} else if (errno != EAGAIN) {
 		tst_res(TFAIL | TERRNO, "read(%d, buf, %zu) failed", fd_notify,
-			 EVENT_BUF_LEN);
+			EVENT_BUF_LEN);
 	} else {
 		tst_res(TPASS, "No event as expected");
 	}
@@ -208,27 +208,27 @@ void test01(void)
 
 	/* Verify FAN_MARK_FLUSH destroys all inode marks */
 	if (fanotify_mark(fd_notify, FAN_MARK_ADD,
-			    FAN_OPEN, AT_FDCWD, fname) < 0) {
+			  FAN_OPEN, AT_FDCWD, fname) < 0) {
 		tst_brk(TBROK | TERRNO,
-		    "fanotify_mark (%d, FAN_MARK_ADD, FAN_OPEN, "
-		    "AT_FDCWD, '%s') failed", fd_notify, fname);
+			"fanotify_mark (%d, FAN_MARK_ADD, FAN_OPEN, "
+			"AT_FDCWD, '%s') failed", fd_notify, fname);
 	}
 	if (fanotify_mark(fd_notify, FAN_MARK_ADD,
-			    FAN_OPEN | FAN_ONDIR, AT_FDCWD, dir) < 0) {
+			  FAN_OPEN | FAN_ONDIR, AT_FDCWD, dir) < 0) {
 		tst_brk(TBROK | TERRNO,
-		    "fanotify_mark (%d, FAN_MARK_ADD, FAN_OPEN | "
-		    "FAN_ONDIR, AT_FDCWD, '%s') failed", fd_notify,
-		    dir);
+			"fanotify_mark (%d, FAN_MARK_ADD, FAN_OPEN | "
+			"FAN_ONDIR, AT_FDCWD, '%s') failed", fd_notify,
+			dir);
 	}
 	open_file(fname);
 	verify_event(S_IFREG);
 	open_dir(dir);
 	verify_event(S_IFDIR);
 	if (fanotify_mark(fd_notify, FAN_MARK_FLUSH,
-			    0, AT_FDCWD, ".") < 0) {
+			  0, AT_FDCWD, ".") < 0) {
 		tst_brk(TBROK | TERRNO,
-		    "fanotify_mark (%d, FAN_MARK_FLUSH, 0, "
-		    "AT_FDCWD, '.') failed", fd_notify);
+			"fanotify_mark (%d, FAN_MARK_FLUSH, 0, "
+			"AT_FDCWD, '.') failed", fd_notify);
 	}
 
 	open_dir(dir);
@@ -250,7 +250,7 @@ static void setup(void)
 	SAFE_MKDIR(dir, 0755);
 
 	fd_notify = SAFE_FANOTIFY_INIT(FAN_CLASS_NOTIF | FAN_NONBLOCK,
-			O_RDONLY);
+					O_RDONLY);
 }
 
 static void cleanup(void)
diff --git a/testcases/kernel/syscalls/fanotify/fanotify05.c b/testcases/kernel/syscalls/fanotify/fanotify05.c
index ec615e6d8..a2296cc5e 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify05.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify05.c
@@ -71,11 +71,11 @@ void test01(void)
 		if (len < 0) {
 			if (errno == -EAGAIN) {
 				tst_res(TFAIL, "Overflow event not "
-					 "generated!\n");
+					"generated!\n");
 				break;
 			}
 			tst_brk(TBROK | TERRNO,
-				 "read of notification event failed");
+				"read of notification event failed");
 			break;
 		}
 		if (event.fd != FAN_NOFD)
@@ -87,27 +87,27 @@ void test01(void)
 		if (event.mask != FAN_OPEN &&
 		    event.mask != FAN_Q_OVERFLOW) {
 			tst_res(TFAIL,
-				 "get event: mask=%llx (expected %llx)"
-				 "pid=%u fd=%d",
-				 (unsigned long long)event.mask,
-				 (unsigned long long)FAN_OPEN,
-				 (unsigned)event.pid, event.fd);
+				"get event: mask=%llx (expected %llx)"
+				"pid=%u fd=%d",
+				(unsigned long long)event.mask,
+				(unsigned long long)FAN_OPEN,
+				(unsigned)event.pid, event.fd);
 			break;
 		}
 		if (event.mask == FAN_Q_OVERFLOW) {
 			if (event.fd != FAN_NOFD) {
 				tst_res(TFAIL,
-					 "invalid overflow event: "
-					 "mask=%llx pid=%u fd=%d",
-					 (unsigned long long)event.mask,
-					 (unsigned)event.pid,
-					 event.fd);
+					"invalid overflow event: "
+					"mask=%llx pid=%u fd=%d",
+					(unsigned long long)event.mask,
+					(unsigned)event.pid,
+					event.fd);
 				break;
 			}
 			tst_res(TPASS,
-				 "get event: mask=%llx pid=%u fd=%d",
-				 (unsigned long long)event.mask,
-				 (unsigned)event.pid, event.fd);
+				"get event: mask=%llx pid=%u fd=%d",
+				(unsigned long long)event.mask,
+				(unsigned)event.pid, event.fd);
 				break;
 		}
 	}
@@ -119,11 +119,11 @@ static void setup(void)
 			O_RDONLY);
 
 	if (fanotify_mark(fd_notify, FAN_MARK_MOUNT | FAN_MARK_ADD, FAN_OPEN,
-			    AT_FDCWD, ".") < 0) {
+			  AT_FDCWD, ".") < 0) {
 		tst_brk(TBROK | TERRNO,
-			 "fanotify_mark (%d, FAN_MARK_MOUNT | FAN_MARK_ADD, "
-			 "FAN_OPEN, AT_FDCWD, \".\") failed",
-			 fd_notify);
+			"fanotify_mark (%d, FAN_MARK_MOUNT | FAN_MARK_ADD, "
+			"FAN_OPEN, AT_FDCWD, \".\") failed",
+			fd_notify);
 	}
 }
 
diff --git a/testcases/kernel/syscalls/fanotify/fanotify06.c b/testcases/kernel/syscalls/fanotify/fanotify06.c
index 988296a7e..bf95b2d7c 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify06.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify06.c
@@ -83,8 +83,8 @@ static void create_fanotify_groups(void)
 	for (p = 0; p < FANOTIFY_PRIORITIES; p++) {
 		for (i = 0; i < GROUPS_PER_PRIO; i++) {
 			fd_notify[p][i] = SAFE_FANOTIFY_INIT(fanotify_prio[p] |
-							FAN_NONBLOCK,
-							O_RDONLY);
+							     FAN_NONBLOCK,
+							     O_RDONLY);
 
 			/* Add mount mark for each group */
 			ret = fanotify_mark(fd_notify[p][i],
@@ -93,9 +93,9 @@ static void create_fanotify_groups(void)
 					    AT_FDCWD, ".");
 			if (ret < 0) {
 				tst_brk(TBROK | TERRNO,
-					 "fanotify_mark(%d, FAN_MARK_ADD | "
-					 "FAN_MARK_MOUNT, FAN_MODIFY, AT_FDCWD,"
-					 " '.') failed", fd_notify[p][i]);
+					"fanotify_mark(%d, FAN_MARK_ADD | "
+					"FAN_MARK_MOUNT, FAN_MODIFY, AT_FDCWD,"
+					" '.') failed", fd_notify[p][i]);
 			}
 			/* Add ignore mark for groups with higher priority */
 			if (p == 0)
@@ -107,11 +107,11 @@ static void create_fanotify_groups(void)
 					    FAN_MODIFY, AT_FDCWD, fname);
 			if (ret < 0) {
 				tst_brk(TBROK | TERRNO,
-					 "fanotify_mark(%d, FAN_MARK_ADD | "
-					 "FAN_MARK_IGNORED_MASK | "
-					 "FAN_MARK_IGNORED_SURV_MODIFY, "
-					 "FAN_MODIFY, AT_FDCWD, %s) failed",
-					 fd_notify[p][i], fname);
+					"fanotify_mark(%d, FAN_MARK_ADD | "
+					"FAN_MARK_IGNORED_MASK | "
+					"FAN_MARK_IGNORED_SURV_MODIFY, "
+					"FAN_MODIFY, AT_FDCWD, %s) failed",
+					fd_notify[p][i], fname);
 			}
 		}
 	}
@@ -133,18 +133,18 @@ static void verify_event(int group, struct fanotify_event_metadata *event)
 {
 	if (event->mask != FAN_MODIFY) {
 		tst_res(TFAIL, "group %d get event: mask %llx (expected %llx) "
-			 "pid=%u fd=%u", group, (unsigned long long)event->mask,
-			 (unsigned long long)FAN_MODIFY,
-			 (unsigned)event->pid, event->fd);
+			"pid=%u fd=%u", group, (unsigned long long)event->mask,
+			(unsigned long long)FAN_MODIFY,
+			(unsigned)event->pid, event->fd);
 	} else if (event->pid != getpid()) {
 		tst_res(TFAIL, "group %d get event: mask %llx pid=%u "
-			 "(expected %u) fd=%u", group,
-			 (unsigned long long)event->mask, (unsigned)event->pid,
-			 (unsigned)getpid(), event->fd);
+			"(expected %u) fd=%u", group,
+			(unsigned long long)event->mask, (unsigned)event->pid,
+			(unsigned)getpid(), event->fd);
 	} else {
 		tst_res(TPASS, "group %d get event: mask %llx pid=%u fd=%u",
-			 group, (unsigned long long)event->mask,
-			 (unsigned)event->pid, event->fd);
+			group, (unsigned long long)event->mask,
+			(unsigned)event->pid, event->fd);
 	}
 }
 
@@ -167,22 +167,22 @@ void test01(void)
 		if (ret < 0) {
 			if (errno == EAGAIN) {
 				tst_res(TFAIL, "group %d did not get "
-					 "event", i);
+					"event", i);
 			}
 			tst_brk(TBROK | TERRNO,
-				 "reading fanotify events failed");
+				"reading fanotify events failed");
 		}
 		if (ret < (int)FAN_EVENT_METADATA_LEN) {
 			tst_brk(TBROK,
-				 "short read when reading fanotify "
-				 "events (%d < %d)", ret,
-				 (int)EVENT_BUF_LEN);
+				"short read when reading fanotify "
+				"events (%d < %d)", ret,
+				(int)EVENT_BUF_LEN);
 		}
 		event = (struct fanotify_event_metadata *)event_buf;
 		if (ret > (int)event->event_len) {
 			tst_res(TFAIL, "group %d got more than one "
-				 "event (%d > %d)", i, ret,
-				 event->event_len);
+				"event (%d > %d)", i, ret,
+				event->event_len);
 		} else {
 			verify_event(i, event);
 		}
@@ -194,18 +194,18 @@ void test01(void)
 			ret = read(fd_notify[p][i], event_buf, EVENT_BUF_LEN);
 			if (ret > 0) {
 				tst_res(TFAIL, "group %d got event",
-					 p*GROUPS_PER_PRIO + i);
+					p*GROUPS_PER_PRIO + i);
 				if (event->fd != FAN_NOFD)
 					SAFE_CLOSE(event->fd);
 			} else if (ret == 0) {
 				tst_brk(TBROK, "zero length "
-					 "read from fanotify fd");
+					"read from fanotify fd");
 			} else if (errno != EAGAIN) {
 				tst_brk(TBROK | TERRNO,
-					 "reading fanotify events failed");
+					"reading fanotify events failed");
 			} else {
 				tst_res(TPASS, "group %d got no event",
-					 p*GROUPS_PER_PRIO + i);
+					p*GROUPS_PER_PRIO + i);
 			}
 		}
 	}
diff --git a/testcases/kernel/syscalls/fanotify/fanotify09.c b/testcases/kernel/syscalls/fanotify/fanotify09.c
index 56be851ab..ebfbcb090 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify09.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify09.c
@@ -71,8 +71,8 @@ static void create_fanotify_groups(void)
 
 	for (i = 0; i < NUM_GROUPS; i++) {
 		fd_notify[i] = SAFE_FANOTIFY_INIT(FAN_CLASS_NOTIF |
-						FAN_NONBLOCK,
-						O_RDONLY);
+						  FAN_NONBLOCK,
+						  O_RDONLY);
 
 		/* Add mount mark for each group without MODIFY event */
 		ret = fanotify_mark(fd_notify[i],
@@ -81,9 +81,9 @@ static void create_fanotify_groups(void)
 				    AT_FDCWD, ".");
 		if (ret < 0) {
 			tst_brk(TBROK | TERRNO,
-				 "fanotify_mark(%d, FAN_MARK_ADD | "
-				 "FAN_MARK_MOUNT, FAN_MODIFY, AT_FDCWD,"
-				 " '.') failed", fd_notify[i]);
+				"fanotify_mark(%d, FAN_MARK_ADD | "
+				"FAN_MARK_MOUNT, FAN_MODIFY, AT_FDCWD,"
+				" '.') failed", fd_notify[i]);
 		}
 		/*
 		 * Add inode mark on parent for each group with MODIFY
@@ -98,10 +98,10 @@ static void create_fanotify_groups(void)
 				    FAN_MODIFY | onchild, AT_FDCWD, ".");
 		if (ret < 0) {
 			tst_brk(TBROK | TERRNO,
-				 "fanotify_mark(%d, FAN_MARK_ADD, "
-				 "FAN_MODIFY%s, AT_FDCWD, '.') failed",
-				 fd_notify[i],
-				 onchild ? " | FAN_EVENT_ON_CHILD" : "");
+				"fanotify_mark(%d, FAN_MARK_ADD, "
+				"FAN_MODIFY%s, AT_FDCWD, '.') failed",
+				fd_notify[i],
+				onchild ? " | FAN_EVENT_ON_CHILD" : "");
 		}
 	}
 }
@@ -120,18 +120,18 @@ static void verify_event(int group, struct fanotify_event_metadata *event)
 {
 	if (event->mask != FAN_MODIFY) {
 		tst_res(TFAIL, "group %d get event: mask %llx (expected %llx) "
-			 "pid=%u fd=%u", group, (unsigned long long)event->mask,
-			 (unsigned long long)FAN_MODIFY,
-			 (unsigned)event->pid, event->fd);
+			"pid=%u fd=%u", group, (unsigned long long)event->mask,
+			(unsigned long long)FAN_MODIFY,
+			(unsigned)event->pid, event->fd);
 	} else if (event->pid != getpid()) {
 		tst_res(TFAIL, "group %d get event: mask %llx pid=%u "
-			 "(expected %u) fd=%u", group,
-			 (unsigned long long)event->mask, (unsigned)event->pid,
-			 (unsigned)getpid(), event->fd);
+			"(expected %u) fd=%u", group,
+			(unsigned long long)event->mask, (unsigned)event->pid,
+			(unsigned)getpid(), event->fd);
 	} else {
 		tst_res(TPASS, "group %d get event: mask %llx pid=%u fd=%u",
-			 group, (unsigned long long)event->mask,
-			 (unsigned)event->pid, event->fd);
+			group, (unsigned long long)event->mask,
+			(unsigned)event->pid, event->fd);
 	}
 }
 
@@ -155,20 +155,20 @@ void test01(void)
 			tst_res(TFAIL, "first group did not get event");
 		} else {
 			tst_brk(TBROK | TERRNO,
-				 "reading fanotify events failed");
+				"reading fanotify events failed");
 		}
 	}
 	if (ret < (int)FAN_EVENT_METADATA_LEN) {
 		tst_brk(TBROK,
-			 "short read when reading fanotify "
-			 "events (%d < %d)", ret,
-			 (int)EVENT_BUF_LEN);
+			"short read when reading fanotify "
+			"events (%d < %d)", ret,
+			(int)EVENT_BUF_LEN);
 	}
 	event = (struct fanotify_event_metadata *)event_buf;
 	if (ret > (int)event->event_len) {
 		tst_res(TFAIL, "first group got more than one "
-			 "event (%d > %d)", ret,
-			 event->event_len);
+			"event (%d > %d)", ret,
+			event->event_len);
 	} else {
 		verify_event(0, event);
 	}
-- 
2.17.1

[LTP] [PATCH] syscalls/fanotify: new test for mount ignore mask

[Amir Goldstein]

This is a regression test for commit:
    9bdda4e9cf2d fsnotify: fix ignore mask logic in fsnotify()

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 runtest/syscalls                              |   1 +
 testcases/kernel/syscalls/fanotify/.gitignore |   1 +
 .../kernel/syscalls/fanotify/fanotify10.c     | 248 ++++++++++++++++++
 3 files changed, 250 insertions(+)
 create mode 100644 testcases/kernel/syscalls/fanotify/fanotify10.c

diff --git a/runtest/syscalls b/runtest/syscalls
index 62fb890cd..33b8d8625 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -497,6 +497,7 @@ fanotify06 fanotify06
 fanotify07 fanotify07
 fanotify08 fanotify08
 fanotify09 fanotify09
+fanotify10 fanotify10
 
 ioperm01 ioperm01
 ioperm02 ioperm02
diff --git a/testcases/kernel/syscalls/fanotify/.gitignore b/testcases/kernel/syscalls/fanotify/.gitignore
index ec7553995..c26f2bd27 100644
--- a/testcases/kernel/syscalls/fanotify/.gitignore
+++ b/testcases/kernel/syscalls/fanotify/.gitignore
@@ -7,3 +7,4 @@
 /fanotify07
 /fanotify08
 /fanotify09
+/fanotify10
diff --git a/testcases/kernel/syscalls/fanotify/fanotify10.c b/testcases/kernel/syscalls/fanotify/fanotify10.c
new file mode 100644
index 000000000..5245eb6f8
--- /dev/null
+++ b/testcases/kernel/syscalls/fanotify/fanotify10.c
@@ -0,0 +1,248 @@
+/*
+ * Copyright (c) 2014 SUSE.  All Rights Reserved.
+ * Copyright (c) 2018 CTERA Networks.  All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ *
+ * Further, this software is distributed without any warranty that it is
+ * free of the rightful claim of any third person regarding infringement
+ * or the like.  Any license provided herein, whether implied or
+ * otherwise, applies only to this software file.  Patent licenses, if
+ * any, provided herein do not apply to combinations of this program with
+ * other software, or any other product whatsoever.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Started by Jan Kara <jack@suse.cz>
+ * Forked from fanotify06.c by Amir Goldstein <amir73il@gmail.com>
+ *
+ * DESCRIPTION
+ *     Check that fanotify properly merges ignore mask of a mount mark
+ *     with a mask of an inode mark on the same group.  Unlike the
+ *     prototype test fanotify06, do not use FAN_MODIFY event for the
+ *     test mask, because it hides the bug.
+ *
+ * This is a regression test for commit:
+ *
+ *     9bdda4e9cf2d fsnotify: fix ignore mask logic in fsnotify()
+ */
+#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>
+
+#define EVENT_MAX 1024
+/* size of the event structure, not counting name */
+#define EVENT_SIZE  (sizeof (struct fanotify_event_metadata))
+/* reasonable guess as to size of 1024 events */
+#define EVENT_BUF_LEN        (EVENT_MAX * EVENT_SIZE)
+
+unsigned int fanotify_prio[] = {
+	FAN_CLASS_PRE_CONTENT,
+	FAN_CLASS_CONTENT,
+	FAN_CLASS_NOTIF
+};
+#define FANOTIFY_PRIORITIES ARRAY_SIZE(fanotify_prio)
+
+#define GROUPS_PER_PRIO 3
+
+#define BUF_SIZE 256
+static char fname[BUF_SIZE];
+static int fd_notify[FANOTIFY_PRIORITIES][GROUPS_PER_PRIO];
+
+static char event_buf[EVENT_BUF_LEN];
+
+#define MOUNT_NAME "mntpoint"
+static int mount_created;
+
+static void create_fanotify_groups(void)
+{
+	unsigned int p, i;
+	int ret;
+
+	for (p = 0; p < FANOTIFY_PRIORITIES; p++) {
+		for (i = 0; i < GROUPS_PER_PRIO; i++) {
+			fd_notify[p][i] = SAFE_FANOTIFY_INIT(fanotify_prio[p] |
+							     FAN_NONBLOCK,
+							     O_RDONLY);
+
+			/* Add mount mark for each group */
+			ret = fanotify_mark(fd_notify[p][i],
+					    FAN_MARK_ADD,
+					    FAN_OPEN,
+					    AT_FDCWD, fname);
+			if (ret < 0) {
+				tst_brk(TBROK | TERRNO,
+					"fanotify_mark(%d, FAN_MARK_ADD | "
+					"FAN_MARK_MOUNT, FAN_OPEN, AT_FDCWD,"
+					" %s) failed", fd_notify[p][i],
+					fname);
+			}
+			/* Add ignore mark for groups with higher priority */
+			if (p == 0)
+				continue;
+			ret = fanotify_mark(fd_notify[p][i],
+					    FAN_MARK_ADD | FAN_MARK_MOUNT |
+					    FAN_MARK_IGNORED_MASK |
+					    FAN_MARK_IGNORED_SURV_MODIFY,
+					    FAN_OPEN, AT_FDCWD, fname);
+			if (ret < 0) {
+				tst_brk(TBROK | TERRNO,
+					"fanotify_mark(%d, FAN_MARK_ADD | "
+					"FAN_MARK_IGNORED_MASK | "
+					"FAN_MARK_IGNORED_SURV_MODIFY, "
+					"FAN_OPEN, AT_FDCWD, %s) failed",
+					fd_notify[p][i], fname);
+			}
+		}
+	}
+}
+
+static void cleanup_fanotify_groups(void)
+{
+	unsigned int i, p;
+
+	for (p = 0; p < FANOTIFY_PRIORITIES; p++) {
+		for (i = 0; i < GROUPS_PER_PRIO; i++) {
+			if (fd_notify[p][i] > 0)
+				SAFE_CLOSE(fd_notify[p][i]);
+		}
+	}
+}
+
+static void verify_event(int group, struct fanotify_event_metadata *event)
+{
+	if (event->mask != FAN_OPEN) {
+		tst_res(TFAIL, "group %d get event: mask %llx (expected %llx) "
+			"pid=%u fd=%u", group, (unsigned long long)event->mask,
+			(unsigned long long)FAN_OPEN,
+			(unsigned)event->pid, event->fd);
+	} else if (event->pid != getpid()) {
+		tst_res(TFAIL, "group %d get event: mask %llx pid=%u "
+			"(expected %u) fd=%u", group,
+			(unsigned long long)event->mask, (unsigned)event->pid,
+			(unsigned)getpid(), event->fd);
+	} else {
+		tst_res(TPASS, "group %d get event: mask %llx pid=%u fd=%u",
+			group, (unsigned long long)event->mask,
+			(unsigned)event->pid, event->fd);
+	}
+}
+
+void test01(void)
+{
+	int ret, fd;
+	unsigned int p, i;
+	struct fanotify_event_metadata *event;
+
+	create_fanotify_groups();
+
+	/*
+	 * generate sequence of events
+	 */
+	fd = SAFE_OPEN(fname, O_RDONLY);
+	SAFE_CLOSE(fd);
+
+	/* First verify all groups without ignore mask got the event */
+	for (i = 0; i < GROUPS_PER_PRIO; i++) {
+		ret = read(fd_notify[0][i], event_buf, EVENT_BUF_LEN);
+		if (ret < 0) {
+			if (errno == EAGAIN) {
+				tst_res(TFAIL, "group %d did not get "
+					"event", i);
+			}
+			tst_brk(TBROK | TERRNO,
+				"reading fanotify events failed");
+		}
+		if (ret < (int)FAN_EVENT_METADATA_LEN) {
+			tst_brk(TBROK,
+				"short read when reading fanotify "
+				"events (%d < %d)", ret,
+				(int)EVENT_BUF_LEN);
+		}
+		event = (struct fanotify_event_metadata *)event_buf;
+		if (ret > (int)event->event_len) {
+			tst_res(TFAIL, "group %d got more than one "
+				"event (%d > %d)", i, ret,
+				event->event_len);
+		} else {
+			verify_event(i, event);
+		}
+		if (event->fd != FAN_NOFD)
+			SAFE_CLOSE(event->fd);
+	}
+	for (p = 1; p < FANOTIFY_PRIORITIES; p++) {
+		for (i = 0; i < GROUPS_PER_PRIO; i++) {
+			ret = read(fd_notify[p][i], event_buf, EVENT_BUF_LEN);
+			if (ret > 0) {
+				tst_res(TFAIL, "group %d got event",
+					p*GROUPS_PER_PRIO + i);
+				if (event->fd != FAN_NOFD)
+					SAFE_CLOSE(event->fd);
+			} else if (ret == 0) {
+				tst_brk(TBROK, "zero length "
+					"read from fanotify fd");
+			} else if (errno != EAGAIN) {
+				tst_brk(TBROK | TERRNO,
+					"reading fanotify events failed");
+			} else {
+				tst_res(TPASS, "group %d got no event",
+					p*GROUPS_PER_PRIO + i);
+			}
+		}
+	}
+	cleanup_fanotify_groups();
+}
+
+static void setup(void)
+{
+	SAFE_MKDIR(MOUNT_NAME, 0755);
+	SAFE_MOUNT(MOUNT_NAME, MOUNT_NAME, "none", MS_BIND, NULL);
+	mount_created = 1;
+	SAFE_CHDIR(MOUNT_NAME);
+
+	sprintf(fname, "tfile_%d", getpid());
+	SAFE_FILE_PRINTF(fname, "1");
+}
+
+static void cleanup(void)
+{
+	cleanup_fanotify_groups();
+
+	SAFE_CHDIR("../");
+
+	if (mount_created && tst_umount(MOUNT_NAME) < 0)
+		tst_brk(TBROK | TERRNO, "umount failed");
+}
+
+static struct tst_test test = {
+	.test_all = test01,
+	.setup = setup,
+	.cleanup = cleanup,
+	.needs_tmpdir = 1,
+	.needs_root = 1
+};
+
+#else
+	TST_TEST_TCONF("system doesn't have required fanotify support");
+#endif
-- 
2.17.1

[LTP] [PATCH] syscalls/fanotify: misc cleanups

[Richard Palethorpe]

Hello Amir,

Amir Goldstein <amir73il@gmail.com> writes:

> * Cleanup backup file descriptor in fanotify03

Please send the fanotify03 changes on their own. It is generally best to
send the smallest "reasonable" patch set possible to make the feeback and
correction cycle as fast as possible.

>
> * Fix whitespace and indentation

Although it is nice to clean up the whitespace, it doesn't justify
overwritting the git history (because it makes git-blame less usable)
and we have to verify that there are no logic changes hidden in the
whitespace changes. It is better to mix whitespace corrections in with
actual logic changes where we have to read every line anyway.

Also use "checkpatch.pl --no-tree -f" from the kernel when checking
whitespace and style.

> @@ -262,6 +262,8 @@ static void cleanup(void)
>  {
>  	if (fd_notify > 0)
>  		SAFE_CLOSE(fd_notify);
> +	if (fd_notify_backup > 0)
> +		SAFE_CLOSE(fd_notify_backup);
>  }
>

Unless I am mistaken the call to SAFE_DUP may fail leaving fd_notify
with a valid fd and nothing in fd_notify_backup. So it is probably best
to check and close both of them.

--
Thank you,
Richard.

[LTP] [PATCH] syscalls/fanotify: misc cleanups

[Amir Goldstein]

On Mon, Sep 10, 2018 at 5:35 PM Richard Palethorpe <rpalethorpe@suse.de> wrote:
>
> Hello Amir,
>
> Amir Goldstein <amir73il@gmail.com> writes:
>
> > * Cleanup backup file descriptor in fanotify03
>
> Please send the fanotify03 changes on their own. It is generally best to
> send the smallest "reasonable" patch set possible to make the feeback and
> correction cycle as fast as possible.
>

ok.

> >
> > * Fix whitespace and indentation
>
> Although it is nice to clean up the whitespace, it doesn't justify
> overwritting the git history (because it makes git-blame less usable)
> and we have to verify that there are no logic changes hidden in the
> whitespace changes. It is better to mix whitespace corrections in with
> actual logic changes where we have to read every line anyway.
>

It's fine by me to drop the whitespace cleanup, although about the
argument of verifying no logic changes, git diff -w is quite useful.

> Also use "checkpatch.pl --no-tree -f" from the kernel when checking
> whitespace and style.
>
> > @@ -262,6 +262,8 @@ static void cleanup(void)
> >  {
> >       if (fd_notify > 0)
> >               SAFE_CLOSE(fd_notify);
> > +     if (fd_notify_backup > 0)
> > +             SAFE_CLOSE(fd_notify_backup);
> >  }
> >
>
> Unless I am mistaken the call to SAFE_DUP may fail leaving fd_notify
> with a valid fd and nothing in fd_notify_backup. So it is probably best
> to check and close both of them.
>

I'm not following.
Isn't "checking and closing both of them" what my fix does??

Thanks,
Amir.

[LTP] [PATCH] syscalls/fanotify: misc cleanups

[Richard Palethorpe]

Hello,

Amir Goldstein <amir73il@gmail.com> writes:

> On Mon, Sep 10, 2018 at 5:35 PM Richard Palethorpe <rpalethorpe@suse.de> wrote:
>>
>> Hello Amir,
>>
>> Amir Goldstein <amir73il@gmail.com> writes:
>>
>> > * Cleanup backup file descriptor in fanotify03
>>
>> Please send the fanotify03 changes on their own. It is generally best to
>> send the smallest "reasonable" patch set possible to make the feeback and
>> correction cycle as fast as possible.
>>
>
> ok.
>
>> >
>> > * Fix whitespace and indentation
>>
>> Although it is nice to clean up the whitespace, it doesn't justify
>> overwritting the git history (because it makes git-blame less usable)
>> and we have to verify that there are no logic changes hidden in the
>> whitespace changes. It is better to mix whitespace corrections in with
>> actual logic changes where we have to read every line anyway.
>>
>
> It's fine by me to drop the whitespace cleanup, although about the
> argument of verifying no logic changes, git diff -w is quite useful.
>
>> Also use "checkpatch.pl --no-tree -f" from the kernel when checking
>> whitespace and style.
>>
>> > @@ -262,6 +262,8 @@ static void cleanup(void)
>> >  {
>> >       if (fd_notify > 0)
>> >               SAFE_CLOSE(fd_notify);
>> > +     if (fd_notify_backup > 0)
>> > +             SAFE_CLOSE(fd_notify_backup);
>> >  }
>> >
>>
>> Unless I am mistaken the call to SAFE_DUP may fail leaving fd_notify
>> with a valid fd and nothing in fd_notify_backup. So it is probably best
>> to check and close both of them.
>>
>
> I'm not following.
> Isn't "checking and closing both of them" what my fix does??
>
> Thanks,
> Amir.

Sorry, of course you are right.

-- 
Thank you,
Richard.

[LTP] [PATCH] syscalls/fanotify: new test for mount ignore mask

[Richard Palethorpe]

Hello,

If a test can handle two different regressions with only slight
modifications then we should use the test index feature:
https://github.com/linux-test-project/ltp/wiki/C-Test-Case-Tutorial#6-split-the-test

In more complex cases common code can be put into a shared header file
or library. Whatever is simpler and cleaner overall. A lot of older
tests have been copied and pasted which leads to big problems in the
long term.

Amir Goldstein <amir73il@gmail.com> writes:

> This is a regression test for commit:
>     9bdda4e9cf2d fsnotify: fix ignore mask logic in fsnotify()
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  runtest/syscalls                              |   1 +
>  testcases/kernel/syscalls/fanotify/.gitignore |   1 +
>  .../kernel/syscalls/fanotify/fanotify10.c     | 248 ++++++++++++++++++
>  3 files changed, 250 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/fanotify/fanotify10.c
>
> diff --git a/runtest/syscalls b/runtest/syscalls
> index 62fb890cd..33b8d8625 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -497,6 +497,7 @@ fanotify06 fanotify06
>  fanotify07 fanotify07
>  fanotify08 fanotify08
>  fanotify09 fanotify09
> +fanotify10 fanotify10
>
>  ioperm01 ioperm01
>  ioperm02 ioperm02
> diff --git a/testcases/kernel/syscalls/fanotify/.gitignore b/testcases/kernel/syscalls/fanotify/.gitignore
> index ec7553995..c26f2bd27 100644
> --- a/testcases/kernel/syscalls/fanotify/.gitignore
> +++ b/testcases/kernel/syscalls/fanotify/.gitignore
> @@ -7,3 +7,4 @@
>  /fanotify07
>  /fanotify08
>  /fanotify09
> +/fanotify10
> diff --git a/testcases/kernel/syscalls/fanotify/fanotify10.c b/testcases/kernel/syscalls/fanotify/fanotify10.c
> new file mode 100644
> index 000000000..5245eb6f8
> --- /dev/null
> +++ b/testcases/kernel/syscalls/fanotify/fanotify10.c
> @@ -0,0 +1,248 @@
> +/*
> + * Copyright (c) 2014 SUSE.  All Rights Reserved.
> + * Copyright (c) 2018 CTERA Networks.  All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of version 2 of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it would be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> + *
> + * Further, this software is distributed without any warranty that it is
> + * free of the rightful claim of any third person regarding infringement
> + * or the like.  Any license provided herein, whether implied or
> + * otherwise, applies only to this software file.  Patent licenses, if
> + * any, provided herein do not apply to combinations of this program with
> + * other software, or any other product whatsoever.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + *
> + * Started by Jan Kara <jack@suse.cz>
> + * Forked from fanotify06.c by Amir Goldstein <amir73il@gmail.com>
> + *
> + * DESCRIPTION
> + *     Check that fanotify properly merges ignore mask of a mount mark
> + *     with a mask of an inode mark on the same group.  Unlike the
> + *     prototype test fanotify06, do not use FAN_MODIFY event for the
> + *     test mask, because it hides the bug.
> + *
> + * This is a regression test for commit:
> + *
> + *     9bdda4e9cf2d fsnotify: fix ignore mask logic in fsnotify()
> + */
> +#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>
> +
> +#define EVENT_MAX 1024
> +/* size of the event structure, not counting name */
> +#define EVENT_SIZE  (sizeof (struct fanotify_event_metadata))
> +/* reasonable guess as to size of 1024 events */
> +#define EVENT_BUF_LEN        (EVENT_MAX * EVENT_SIZE)
> +
> +unsigned int fanotify_prio[] = {
> +	FAN_CLASS_PRE_CONTENT,
> +	FAN_CLASS_CONTENT,
> +	FAN_CLASS_NOTIF
> +};
> +#define FANOTIFY_PRIORITIES ARRAY_SIZE(fanotify_prio)
> +
> +#define GROUPS_PER_PRIO 3
> +
> +#define BUF_SIZE 256
> +static char fname[BUF_SIZE];
> +static int fd_notify[FANOTIFY_PRIORITIES][GROUPS_PER_PRIO];
> +
> +static char event_buf[EVENT_BUF_LEN];
> +
> +#define MOUNT_NAME "mntpoint"
> +static int mount_created;
> +
> +static void create_fanotify_groups(void)
> +{
> +	unsigned int p, i;
> +	int ret;
> +
> +	for (p = 0; p < FANOTIFY_PRIORITIES; p++) {
> +		for (i = 0; i < GROUPS_PER_PRIO; i++) {
> +			fd_notify[p][i] = SAFE_FANOTIFY_INIT(fanotify_prio[p] |
> +							     FAN_NONBLOCK,
> +							     O_RDONLY);
> +
> +			/* Add mount mark for each group */
> +			ret = fanotify_mark(fd_notify[p][i],
> +					    FAN_MARK_ADD,
> +					    FAN_OPEN,
> +					    AT_FDCWD, fname);
> +			if (ret < 0) {
> +				tst_brk(TBROK | TERRNO,
> +					"fanotify_mark(%d, FAN_MARK_ADD | "
> +					"FAN_MARK_MOUNT, FAN_OPEN, AT_FDCWD,"
> +					" %s) failed", fd_notify[p][i],
> +					fname);
> +			}
> +			/* Add ignore mark for groups with higher priority */
> +			if (p == 0)
> +				continue;
> +			ret = fanotify_mark(fd_notify[p][i],
> +					    FAN_MARK_ADD | FAN_MARK_MOUNT |
> +					    FAN_MARK_IGNORED_MASK |
> +					    FAN_MARK_IGNORED_SURV_MODIFY,
> +					    FAN_OPEN, AT_FDCWD, fname);
> +			if (ret < 0) {
> +				tst_brk(TBROK | TERRNO,
> +					"fanotify_mark(%d, FAN_MARK_ADD | "
> +					"FAN_MARK_IGNORED_MASK | "
> +					"FAN_MARK_IGNORED_SURV_MODIFY, "
> +					"FAN_OPEN, AT_FDCWD, %s) failed",
> +					fd_notify[p][i], fname);
> +			}
> +		}
> +	}
> +}
> +
> +static void cleanup_fanotify_groups(void)
> +{
> +	unsigned int i, p;
> +
> +	for (p = 0; p < FANOTIFY_PRIORITIES; p++) {
> +		for (i = 0; i < GROUPS_PER_PRIO; i++) {
> +			if (fd_notify[p][i] > 0)
> +				SAFE_CLOSE(fd_notify[p][i]);
> +		}
> +	}
> +}
> +
> +static void verify_event(int group, struct fanotify_event_metadata *event)
> +{
> +	if (event->mask != FAN_OPEN) {
> +		tst_res(TFAIL, "group %d get event: mask %llx (expected %llx) "
> +			"pid=%u fd=%u", group, (unsigned long long)event->mask,
> +			(unsigned long long)FAN_OPEN,
> +			(unsigned)event->pid, event->fd);
> +	} else if (event->pid != getpid()) {
> +		tst_res(TFAIL, "group %d get event: mask %llx pid=%u "
> +			"(expected %u) fd=%u", group,
> +			(unsigned long long)event->mask, (unsigned)event->pid,
> +			(unsigned)getpid(), event->fd);
> +	} else {
> +		tst_res(TPASS, "group %d get event: mask %llx pid=%u fd=%u",
> +			group, (unsigned long long)event->mask,
> +			(unsigned)event->pid, event->fd);
> +	}
> +}
> +
> +void test01(void)
> +{
> +	int ret, fd;
> +	unsigned int p, i;
> +	struct fanotify_event_metadata *event;
> +
> +	create_fanotify_groups();
> +
> +	/*
> +	 * generate sequence of events
> +	 */
> +	fd = SAFE_OPEN(fname, O_RDONLY);
> +	SAFE_CLOSE(fd);
> +
> +	/* First verify all groups without ignore mask got the event */
> +	for (i = 0; i < GROUPS_PER_PRIO; i++) {
> +		ret = read(fd_notify[0][i], event_buf, EVENT_BUF_LEN);
> +		if (ret < 0) {
> +			if (errno == EAGAIN) {
> +				tst_res(TFAIL, "group %d did not get "
> +					"event", i);
> +			}
> +			tst_brk(TBROK | TERRNO,
> +				"reading fanotify events failed");
> +		}
> +		if (ret < (int)FAN_EVENT_METADATA_LEN) {
> +			tst_brk(TBROK,
> +				"short read when reading fanotify "
> +				"events (%d < %d)", ret,
> +				(int)EVENT_BUF_LEN);
> +		}
> +		event = (struct fanotify_event_metadata *)event_buf;
> +		if (ret > (int)event->event_len) {
> +			tst_res(TFAIL, "group %d got more than one "
> +				"event (%d > %d)", i, ret,
> +				event->event_len);
> +		} else {
> +			verify_event(i, event);
> +		}
> +		if (event->fd != FAN_NOFD)
> +			SAFE_CLOSE(event->fd);
> +	}
> +	for (p = 1; p < FANOTIFY_PRIORITIES; p++) {
> +		for (i = 0; i < GROUPS_PER_PRIO; i++) {
> +			ret = read(fd_notify[p][i], event_buf, EVENT_BUF_LEN);
> +			if (ret > 0) {
> +				tst_res(TFAIL, "group %d got event",
> +					p*GROUPS_PER_PRIO + i);
> +				if (event->fd != FAN_NOFD)
> +					SAFE_CLOSE(event->fd);
> +			} else if (ret == 0) {
> +				tst_brk(TBROK, "zero length "
> +					"read from fanotify fd");
> +			} else if (errno != EAGAIN) {
> +				tst_brk(TBROK | TERRNO,
> +					"reading fanotify events failed");
> +			} else {
> +				tst_res(TPASS, "group %d got no event",
> +					p*GROUPS_PER_PRIO + i);
> +			}
> +		}
> +	}
> +	cleanup_fanotify_groups();
> +}
> +
> +static void setup(void)
> +{
> +	SAFE_MKDIR(MOUNT_NAME, 0755);
> +	SAFE_MOUNT(MOUNT_NAME, MOUNT_NAME, "none", MS_BIND, NULL);
> +	mount_created = 1;
> +	SAFE_CHDIR(MOUNT_NAME);
> +
> +	sprintf(fname, "tfile_%d", getpid());
> +	SAFE_FILE_PRINTF(fname, "1");
> +}
> +
> +static void cleanup(void)
> +{
> +	cleanup_fanotify_groups();
> +
> +	SAFE_CHDIR("../");
> +
> +	if (mount_created && tst_umount(MOUNT_NAME) < 0)
> +		tst_brk(TBROK | TERRNO, "umount failed");
> +}
> +
> +static struct tst_test test = {
> +	.test_all = test01,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.needs_tmpdir = 1,
> +	.needs_root = 1
> +};
> +
> +#else
> +	TST_TEST_TCONF("system doesn't have required fanotify support");
> +#endif
> --
> 2.17.1


--
Thank you,
Richard.

[LTP] [PATCH] syscalls/fanotify: new test for mount ignore mask

[Amir Goldstein]

On Tue, Sep 11, 2018 at 12:07 PM Richard Palethorpe <rpalethorpe@suse.de> wrote:
>
> Hello,
>
> If a test can handle two different regressions with only slight
> modifications then we should use the test index feature:
> https://github.com/linux-test-project/ltp/wiki/C-Test-Case-Tutorial#6-split-the-test
>
> In more complex cases common code can be put into a shared header file
> or library. Whatever is simpler and cleaner overall. A lot of older
> tests have been copied and pasted which leads to big problems in the
> long term.
>

All right. I have already written this test with a test index to cover
all possible
mixes of mark types include and exclude masks:
https://github.com/amir73il/ltp/blob/fanotify_sb/testcases/kernel/syscalls/fanotify/fanotify13.c
This gives better coverage than fanotify06 and fanotify10 combined.

However, if I modify test fanotify06 instead of forking test fanotify10, the
test (fanotify06) is going to start failing on non-master kernels.
Is that acceptable for LTP? I am asking because in fstests project, we have
the practice not to change an existing test to failing. When we find a
new regression
we write a new variant of the test for it.

If changing an existing test to cover more cases is appropriate than I am
going to generalize fanotify06 (as fanotify13 linked above) and then
when FAN_MARK_FILESYSTEM mark type support is added to kernel
all I will need to do is change the test again to add another mark type
to  fanotify_mark_types array.

Thoughts?

Thanks,
Amir.

[LTP] [PATCH] syscalls/fanotify: new test for mount ignore mask

[Cyril Hrubis]

Hi!
> All right. I have already written this test with a test index to cover
> all possible
> mixes of mark types include and exclude masks:
> https://github.com/amir73il/ltp/blob/fanotify_sb/testcases/kernel/syscalls/fanotify/fanotify13.c
> This gives better coverage than fanotify06 and fanotify10 combined.
> 
> However, if I modify test fanotify06 instead of forking test fanotify10, the
> test (fanotify06) is going to start failing on non-master kernels.
> Is that acceptable for LTP? I am asking because in fstests project, we have
> the practice not to change an existing test to failing. When we find a
> new regression we write a new variant of the test for it.

We do not have a rule lik this but it sounds like a reasonable rule to
me, since when existing test starts to fail it does look like a
regression in the tests itself.

> If changing an existing test to cover more cases is appropriate than I am
> going to generalize fanotify06 (as fanotify13 linked above) and then
> when FAN_MARK_FILESYSTEM mark type support is added to kernel
> all I will need to do is change the test again to add another mark type
> to  fanotify_mark_types array.

I guess that either would be fine. In the end someone has to look
closely at failing tests anyway to say what exactly happened.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] syscalls/fanotify: new test for mount ignore mask

[Amir Goldstein]

On Thu, Sep 13, 2018 at 2:45 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > All right. I have already written this test with a test index to cover
> > all possible
> > mixes of mark types include and exclude masks:
> > https://github.com/amir73il/ltp/blob/fanotify_sb/testcases/kernel/syscalls/fanotify/fanotify13.c
> > This gives better coverage than fanotify06 and fanotify10 combined.
> >
> > However, if I modify test fanotify06 instead of forking test fanotify10, the
> > test (fanotify06) is going to start failing on non-master kernels.
> > Is that acceptable for LTP? I am asking because in fstests project, we have
> > the practice not to change an existing test to failing. When we find a
> > new regression we write a new variant of the test for it.
>
> We do not have a rule lik this but it sounds like a reasonable rule to
> me, since when existing test starts to fail it does look like a
> regression in the tests itself.
>
> > If changing an existing test to cover more cases is appropriate than I am
> > going to generalize fanotify06 (as fanotify13 linked above) and then
> > when FAN_MARK_FILESYSTEM mark type support is added to kernel
> > all I will need to do is change the test again to add another mark type
> > to  fanotify_mark_types array.
>
> I guess that either would be fine. In the end someone has to look
> closely at failing tests anyway to say what exactly happened.
>

So here is what I suggest, which seems most reasonable w.r.t code reuse
and friendliness to testers:
- Leave fanotify06 untouched because it checks the common case
  (ignore on inode mark)
- Add new test fanotify10 that checks all combinations of marks types
  (like referenced fanotify13)
- After FAN_MARK_FILESYSTEM patches are merged, add the new mark
  type to fanotify10 test matrix, but disable it on runtime if new mark type
  is not supported by kernel.

Thanks,
Amir.

[LTP] [PATCH] syscalls/fanotify: new test for mount ignore mask

[Richard Palethorpe]

Hello,

Amir Goldstein <amir73il@gmail.com> writes:

> On Thu, Sep 13, 2018 at 2:45 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>>
>> Hi!
>> > All right. I have already written this test with a test index to cover
>> > all possible
>> > mixes of mark types include and exclude masks:
>> > https://github.com/amir73il/ltp/blob/fanotify_sb/testcases/kernel/syscalls/fanotify/fanotify13.c
>> > This gives better coverage than fanotify06 and fanotify10 combined.
>> >
>> > However, if I modify test fanotify06 instead of forking test fanotify10, the
>> > test (fanotify06) is going to start failing on non-master kernels.
>> > Is that acceptable for LTP? I am asking because in fstests project, we have
>> > the practice not to change an existing test to failing. When we find a
>> > new regression we write a new variant of the test for it.
>>
>> We do not have a rule lik this but it sounds like a reasonable rule to
>> me, since when existing test starts to fail it does look like a
>> regression in the tests itself.
>>
>> > If changing an existing test to cover more cases is appropriate than I am
>> > going to generalize fanotify06 (as fanotify13 linked above) and then
>> > when FAN_MARK_FILESYSTEM mark type support is added to kernel
>> > all I will need to do is change the test again to add another mark type
>> > to  fanotify_mark_types array.
>>
>> I guess that either would be fine. In the end someone has to look
>> closely at failing tests anyway to say what exactly happened.
>>
>
> So here is what I suggest, which seems most reasonable w.r.t code reuse
> and friendliness to testers:
> - Leave fanotify06 untouched because it checks the common case
>   (ignore on inode mark)
> - Add new test fanotify10 that checks all combinations of marks types
>   (like referenced fanotify13)
> - After FAN_MARK_FILESYSTEM patches are merged, add the new mark
>   type to fanotify10 test matrix, but disable it on runtime if new mark type
>   is not supported by kernel.
>
> Thanks,
> Amir.

Maybe it does make sense to introduce a new test if you are testing a
new feature (even if it is only a minor change). But to clarify, I also
think that for long term maintainability, it makes sense to refactor
common code into a header file.

There are instances in the LTP where a test has been copied, then
various fixes and improvements applied to one copy, but not the others
(sometimes a fix is not needed straight away in a particular test copy,
but then something else changes and it starts failing). Eventually
someone has to combine all the copies in one big painful refactoring,
instead of appying incremental changes along the way.

Obviously if you touch fanotify06 in any way you are increasing the risk
of breaking the test, but I think it is worth the risk over time.

--
Thank you,
Richard.