[LTP] [PATCH 0/3] fanotify{14,20}: cleanup

[LTP] [PATCH 0/3] fanotify{14,20}: cleanup

[Petr Vorel]

Hi,

just an example how to further cleanup fanotify tests by using test macros
from include/tst_test_macros.h. This can wait till Amir's FAN_MARK_IGNORE
patchset [1] is merged (unless there is going to be v2).

fanotify20 is an example what I'd address in the code, fanotify14 just
uses newly added TST_EXP_FD_ERRNO() (more cleanup here and actually in
other tests could be done).

I also admit code in include/tst_test_macros.h is a bit hard to read due
being macro. We should probably add some documentation to it.

Kind regards,
Petr

[1] https://lore.kernel.org/ltp/20220905154239.2652169-1-amir73il@gmail.com/

Petr Vorel (3):
  tst_test_macros: Add TST_EXP_FD_ERRNO
  fanotify20: Simplify code
  fanotify14: Use TST_EXP_FD_ERRNO()

 include/tst_test_macros.h                     |  10 ++
 .../kernel/syscalls/fanotify/fanotify14.c     | 118 +++++-------------
 .../kernel/syscalls/fanotify/fanotify20.c     |  81 +++---------
 3 files changed, 62 insertions(+), 147 deletions(-)

-- 
2.37.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 1/3] tst_test_macros: Add TST_EXP_FD_ERRNO

[Petr Vorel]

Combine TST_EXP_FD() and TST_EXP_FAIL().

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 include/tst_test_macros.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/tst_test_macros.h b/include/tst_test_macros.h
index 2e7b7871c..a2dfaac13 100644
--- a/include/tst_test_macros.h
+++ b/include/tst_test_macros.h
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * Copyright (c) 2015-2020 Cyril Hrubis <chrubis@suse.cz>
+ * Copyright (c) Linux Test Project, 2021-2022
  */
 
 #ifndef TST_TEST_MACROS_H__
@@ -93,6 +94,15 @@ extern void *TST_RET_PTR;
 				#SCALL, ##__VA_ARGS__);                        \
 	} while (0)
 
+#define TST_EXP_FD_ERRNO(SCALL, ERRNO, ...)                                    \
+	do {                                                                   \
+		if (ERRNO)                                                     \
+			TST_EXP_FAIL(SCALL, ERRNO, ##__VA_ARGS__);             \
+		else                                                           \
+			TST_EXP_FD(SCALL, ##__VA_ARGS__);                      \
+		                                                               \
+	} while (0)
+
 #define TST_EXP_PID_SILENT(SCALL, ...)	TST_EXP_POSITIVE_(SCALL, #SCALL, ##__VA_ARGS__)
 
 #define TST_EXP_PID(SCALL, ...)                                                \
-- 
2.37.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 2/3] fanotify20: Simplify code

[Petr Vorel]

* replace do_test() content with TST_EXP_FD_ERRNO() macro
* rename variables (shorten, use LTP common names)
* remove tc->want_err (not needed)
* add macro FLAGS_DESC (stringify)
* don't print number of tests (not needed for just 2 tests)

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 .../kernel/syscalls/fanotify/fanotify20.c     | 81 +++++--------------
 1 file changed, 19 insertions(+), 62 deletions(-)

diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
index de0fdb782..badc4c369 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify20.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * Copyright (c) 2021 Google. All Rights Reserved.
+ * Copyright (c) 2022 Petr Vorel <pvorel@suse.cz>
  *
  * Started by Matthew Bobrowski <repnop@google.com>
  */
@@ -25,26 +26,21 @@
 #include "fanotify.h"
 
 #define MOUNT_PATH	"fs_mnt"
+#define FLAGS_DESC(x) .flags = x, .desc = #x
 
-static int fanotify_fd;
+static int fd;
 
 static struct test_case_t {
-	char *name;
-	unsigned int init_flags;
-	int want_err;
-	int want_errno;
+	unsigned int flags;
+	char *desc;
+	int exp_errno;
 } test_cases[] = {
 	{
-		"fail on FAN_REPORT_PIDFD | FAN_REPORT_TID",
-		FAN_REPORT_PIDFD | FAN_REPORT_TID,
-		1,
-		EINVAL,
+		FLAGS_DESC(FAN_REPORT_PIDFD | FAN_REPORT_TID),
+		.exp_errno = EINVAL,
 	},
 	{
-		"pass on FAN_REPORT_PIDFD | FAN_REPORT_FID | FAN_REPORT_DFID_NAME",
-		FAN_REPORT_PIDFD | FAN_REPORT_FID | FAN_REPORT_DFID_NAME,
-		0,
-		0,
+		FLAGS_DESC(FAN_REPORT_PIDFD | FAN_REPORT_FID | FAN_REPORT_DFID_NAME),
 	},
 };
 
@@ -57,63 +53,24 @@ static void do_setup(void)
 	REQUIRE_FANOTIFY_INIT_FLAGS_SUPPORTED_BY_KERNEL(FAN_REPORT_PIDFD);
 }
 
-static void do_test(unsigned int num)
+static void do_test(unsigned int i)
 {
-	struct test_case_t *tc = &test_cases[num];
+	struct test_case_t *tc = &test_cases[i];
 
-	tst_res(TINFO, "Test #%d: %s", num, tc->name);
+	tst_res(TINFO, "Test %s on %s", tc->exp_errno ? "fail" : "pass",
+		tc->desc);
 
-	fanotify_fd = fanotify_init(tc->init_flags, O_RDONLY);
-	if (fanotify_fd < 0) {
-		if (!tc->want_err) {
-			tst_res(TFAIL,
-				"fanotify_fd=%d, fanotify_init(%x, O_RDONLY) "
-				"failed with error -%d but wanted success",
-				fanotify_fd, tc->init_flags, errno);
-			return;
-		}
+	TST_EXP_FD_ERRNO(fd = fanotify_init(tc->flags, O_RDONLY),
+			 tc->exp_errno);
 
-		if (errno != tc->want_errno) {
-			tst_res(TFAIL,
-				"fanotify_fd=%d, fanotify_init(%x, O_RDONLY) "
-				"failed with an unexpected error code -%d but "
-				"wanted -%d",
-				fanotify_fd, tc->init_flags,
-				errno, tc->want_errno);
-			return;
-		}
-
-		tst_res(TPASS,
-			"fanotify_fd=%d, fanotify_init(%x, O_RDONLY) "
-			"failed with error -%d as expected",
-			fanotify_fd, tc->init_flags, errno);
-		return;
-	}
-
-	/*
-	 * Catch test cases that had expected to receive an error upon calling
-	 * fanotify_init() but had unexpectedly resulted in a success.
-	 */
-	if (tc->want_err) {
-		tst_res(TFAIL,
-			"fanotify_fd=%d, fanotify_init(%x, O_RDONLY) "
-			"unexpectedly returned successfully, wanted error -%d",
-			fanotify_fd, tc->init_flags, tc->want_errno);
-		return;
-	}
-
-	tst_res(TPASS,
-		"fanotify_fd=%d, fanotify_init(%x, O_RDONLY) "
-		"successfully initialized notification group",
-		fanotify_fd, tc->init_flags);
-
-	SAFE_CLOSE(fanotify_fd);
+	if (fd > 0)
+		SAFE_CLOSE(fd);
 }
 
 static void do_cleanup(void)
 {
-	if (fanotify_fd >= 0)
-		SAFE_CLOSE(fanotify_fd);
+	if (fd > 0)
+		SAFE_CLOSE(fd);
 }
 
 static struct tst_test test = {
-- 
2.37.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/3] fanotify20: Simplify code

[Amir Goldstein]

On Tue, Sep 6, 2022 at 12:26 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> * replace do_test() content with TST_EXP_FD_ERRNO() macro
> * rename variables (shorten, use LTP common names)
> * remove tc->want_err (not needed)
> * add macro FLAGS_DESC (stringify)
> * don't print number of tests (not needed for just 2 tests)
>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>

Nice cleanup.
You may add
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

however...

> ---
>  .../kernel/syscalls/fanotify/fanotify20.c     | 81 +++++--------------
>  1 file changed, 19 insertions(+), 62 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
> index de0fdb782..badc4c369 100644
> --- a/testcases/kernel/syscalls/fanotify/fanotify20.c
> +++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  /*
>   * Copyright (c) 2021 Google. All Rights Reserved.
> + * Copyright (c) 2022 Petr Vorel <pvorel@suse.cz>
>   *
>   * Started by Matthew Bobrowski <repnop@google.com>
>   */
> @@ -25,26 +26,21 @@
>  #include "fanotify.h"
>
>  #define MOUNT_PATH     "fs_mnt"
> +#define FLAGS_DESC(x) .flags = x, .desc = #x
>
> -static int fanotify_fd;
> +static int fd;
>

What is this change for?
It makes the code less readable.
fd is quite an unspecific name for a global variable.

Thanks,
Amir.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/3] fanotify20: Simplify code

[Petr Vorel]

> On Tue, Sep 6, 2022 at 12:26 PM Petr Vorel <pvorel@suse.cz> wrote:

> > * replace do_test() content with TST_EXP_FD_ERRNO() macro
> > * rename variables (shorten, use LTP common names)
> > * remove tc->want_err (not needed)
> > * add macro FLAGS_DESC (stringify)
> > * don't print number of tests (not needed for just 2 tests)

> > Signed-off-by: Petr Vorel <pvorel@suse.cz>

> Nice cleanup.
> You may add
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>

Thanks a lot for your time to review.

> however...

> > ---
> >  .../kernel/syscalls/fanotify/fanotify20.c     | 81 +++++--------------
> >  1 file changed, 19 insertions(+), 62 deletions(-)

> > diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
> > index de0fdb782..badc4c369 100644
> > --- a/testcases/kernel/syscalls/fanotify/fanotify20.c
> > +++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0-or-later
> >  /*
> >   * Copyright (c) 2021 Google. All Rights Reserved.
> > + * Copyright (c) 2022 Petr Vorel <pvorel@suse.cz>
> >   *
> >   * Started by Matthew Bobrowski <repnop@google.com>
> >   */
> > @@ -25,26 +26,21 @@
> >  #include "fanotify.h"

> >  #define MOUNT_PATH     "fs_mnt"
> > +#define FLAGS_DESC(x) .flags = x, .desc = #x

> > -static int fanotify_fd;
> > +static int fd;


> What is this change for?
> It makes the code less readable.
> fd is quite an unspecific name for a global variable.
The motivation was: fanotify_fd is quite long and there are quite a lot of long
lines which needs to be split. I also thought that the only file descriptor in
fanotify tests does not have to have "fanotify_" prefix. But sure, No problem, I
merge it without this change.

Kind regards,
Petr

> Thanks,
> Amir.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/3] fanotify20: Simplify code

[Petr Vorel]

...
> > What is this change for?
> > It makes the code less readable.
> > fd is quite an unspecific name for a global variable.
> The motivation was: fanotify_fd is quite long and there are quite a lot of long
> lines which needs to be split. I also thought that the only file descriptor in
> fanotify tests does not have to have "fanotify_" prefix. But sure, No problem, I
> merge it without this change.

FYI what I would particularly prefer to change on fanotify code style are many
wrapped strings (hard to grep).

Kind regards,
Petr

> Kind regards,
> Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/3] fanotify20: Simplify code

[Matthew Bobrowski via ltp]

On Tue, Sep 06, 2022 at 11:26:14AM +0200, Petr Vorel wrote:
> * replace do_test() content with TST_EXP_FD_ERRNO() macro
> * rename variables (shorten, use LTP common names)
> * remove tc->want_err (not needed)
> * add macro FLAGS_DESC (stringify)
> * don't print number of tests (not needed for just 2 tests)
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>

Awesome simplification!

Reviewed-by: Matthew Bobrowski <repnop@google.com>

> ---
>  .../kernel/syscalls/fanotify/fanotify20.c     | 81 +++++--------------
>  1 file changed, 19 insertions(+), 62 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
> index de0fdb782..badc4c369 100644
> --- a/testcases/kernel/syscalls/fanotify/fanotify20.c
> +++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  /*
>   * Copyright (c) 2021 Google. All Rights Reserved.
> + * Copyright (c) 2022 Petr Vorel <pvorel@suse.cz>
>   *
>   * Started by Matthew Bobrowski <repnop@google.com>
>   */
> @@ -25,26 +26,21 @@
>  #include "fanotify.h"
>  
>  #define MOUNT_PATH	"fs_mnt"
> +#define FLAGS_DESC(x) .flags = x, .desc = #x

I'm wondering whether it makes sense to move this out into fanotify.h,
so that if the same test approach is ever used, we can simply recycle
this macro definition.

/M

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 2/3] fanotify20: Simplify code

[Petr Vorel]

> On Tue, Sep 06, 2022 at 11:26:14AM +0200, Petr Vorel wrote:
> > * replace do_test() content with TST_EXP_FD_ERRNO() macro
> > * rename variables (shorten, use LTP common names)
> > * remove tc->want_err (not needed)
> > * add macro FLAGS_DESC (stringify)
> > * don't print number of tests (not needed for just 2 tests)

> > Signed-off-by: Petr Vorel <pvorel@suse.cz>

> Awesome simplification!

> Reviewed-by: Matthew Bobrowski <repnop@google.com>

Thanks for your review!

> > ---
> >  .../kernel/syscalls/fanotify/fanotify20.c     | 81 +++++--------------
> >  1 file changed, 19 insertions(+), 62 deletions(-)

> > diff --git a/testcases/kernel/syscalls/fanotify/fanotify20.c b/testcases/kernel/syscalls/fanotify/fanotify20.c
> > index de0fdb782..badc4c369 100644
> > --- a/testcases/kernel/syscalls/fanotify/fanotify20.c
> > +++ b/testcases/kernel/syscalls/fanotify/fanotify20.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0-or-later
> >  /*
> >   * Copyright (c) 2021 Google. All Rights Reserved.
> > + * Copyright (c) 2022 Petr Vorel <pvorel@suse.cz>
> >   *
> >   * Started by Matthew Bobrowski <repnop@google.com>
> >   */
> > @@ -25,26 +26,21 @@
> >  #include "fanotify.h"

> >  #define MOUNT_PATH	"fs_mnt"
> > +#define FLAGS_DESC(x) .flags = x, .desc = #x

> I'm wondering whether it makes sense to move this out into fanotify.h,
> so that if the same test approach is ever used, we can simply recycle
> this macro definition.
I'd prefer to keep it now in fanotify20.c as that's the only file where is used.
It will be moved once other files starts to use it.

Kind regards,
Petr

> /M

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 3/3] fanotify14: Use TST_EXP_FD_ERRNO()

[Petr Vorel]

That greatly simplifies the code.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 .../kernel/syscalls/fanotify/fanotify14.c     | 118 +++++-------------
 1 file changed, 33 insertions(+), 85 deletions(-)

diff --git a/testcases/kernel/syscalls/fanotify/fanotify14.c b/testcases/kernel/syscalls/fanotify/fanotify14.c
index aa4db586e..47d013c9f 100644
--- a/testcases/kernel/syscalls/fanotify/fanotify14.c
+++ b/testcases/kernel/syscalls/fanotify/fanotify14.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (c) 2018 Matthew Bobrowski. All Rights Reserved.
+ * Copyright (c) Linux Test Project, 2020-2022
  *
  * Started by Matthew Bobrowski <mbobrowski@mbobrowski.org>
  */
@@ -48,6 +49,7 @@ static int fan_report_target_fid_unsupported;
 static struct test_case_t {
 	unsigned int init_flags;
 	unsigned int mark_flags;
+	/* zero mask expects to fail on fanotify_init() */
 	unsigned long long mask;
 	int expected_errno;
 } test_cases[] = {
@@ -111,7 +113,6 @@ static struct test_case_t {
 
 static void do_test(unsigned int number)
 {
-	int ret;
 	struct test_case_t *tc = &test_cases[number];
 
 	if (fan_report_target_fid_unsupported && tc->init_flags & FAN_REPORT_TARGET_FID) {
@@ -120,101 +121,48 @@ static void do_test(unsigned int number)
 		return;
 	}
 
-	fanotify_fd = fanotify_init(tc->init_flags, O_RDONLY);
-	if (fanotify_fd < 0) {
-		if (errno == tc->expected_errno) {
-			tst_res(TPASS,
-				"fanotify_fd=%d, fanotify_init(%x, O_RDONLY) "
-				"failed with error %d as expected",
-				fanotify_fd,
-				tc->init_flags, tc->expected_errno);
-			return;
-		}
-		tst_brk(TBROK | TERRNO,
-			"fanotify_fd=%d, fanotify_init(%x, O_RDONLY) failed",
-			fanotify_fd,
-			tc->init_flags);
-	}
+	TST_EXP_FD_ERRNO(fanotify_fd = fanotify_init(tc->init_flags, O_RDONLY),
+			 !tc->mask && tc->expected_errno ? tc->expected_errno : 0);
 
-	/*
-	 * A test case with a mask set to zero indicate that they've been
-	 * specifically designed to test and fail on the fanotify_init()
-	 * system call.
-	 */
-	if (tc->mask == 0) {
-		tst_res(TFAIL,
-			"fanotify_fd=%d fanotify_init(%x, O_RDONLY) "
-			"unexpectedly succeeded when tests with mask 0 are "
-			"expected to fail when calling fanotify_init()",
-			fanotify_fd,
-			tc->init_flags);
+	if (fanotify_fd < 0)
+		return;
+
+	if (!tc->mask)
 		goto out;
-	}
 
 	/* Set mark on non-dir only when expecting error ENOTDIR */
 	const char *path = tc->expected_errno == ENOTDIR ? FILE1 : MNTPOINT;
 
-	ret = fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark_flags,
-				tc->mask, AT_FDCWD, path);
-	if (ret < 0) {
-		if (errno == tc->expected_errno) {
-			tst_res(TPASS,
-				"ret=%d, fanotify_mark(%d, FAN_MARK_ADD | %x, "
-				"%llx, AT_FDCWD, %s) failed with error %d "
-				"as expected",
-				ret,
-				fanotify_fd,
-				tc->mark_flags,
-				tc->mask,
-				path, tc->expected_errno);
-			/*
-			 * ENOTDIR are errors for events/flags not allowed on a non-dir inode.
-			 * Try to set an inode mark on a directory and it should succeed.
-			 * Try to set directory events in filesystem mark mask on non-dir
-			 * and it should succeed.
-			 */
-			if (tc->expected_errno == ENOTDIR) {
-				SAFE_FANOTIFY_MARK(fanotify_fd, FAN_MARK_ADD | tc->mark_flags,
-						   tc->mask, AT_FDCWD, MNTPOINT);
-				tst_res(TPASS,
-					"Adding an inode mark on directory did not fail with "
-					"ENOTDIR error as on non-dir inode");
-			}
-			if (tc->expected_errno == ENOTDIR &&
-			    !(tc->mark_flags & FAN_MARK_ONLYDIR)) {
-				SAFE_FANOTIFY_MARK(fanotify_fd, FAN_MARK_ADD | tc->mark_flags |
-						   FAN_MARK_FILESYSTEM, tc->mask,
-						   AT_FDCWD, FILE1);
-				tst_res(TPASS,
-					"Adding a filesystem mark on non-dir did not fail with "
-					"ENOTDIR error as with an inode mark");
-			}
+	TST_EXP_FD_ERRNO(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark_flags,
+								   tc->mask, AT_FDCWD, path),
+					 tc->expected_errno);
 
-			goto out;
+	/*
+	 * ENOTDIR are errors for events/flags not allowed on a non-dir inode.
+	 * Try to set an inode mark on a directory and it should succeed.
+	 * Try to set directory events in filesystem mark mask on non-dir
+	 * and it should succeed.
+	 */
+	if (TST_PASS && tc->expected_errno == ENOTDIR) {
+		SAFE_FANOTIFY_MARK(fanotify_fd, FAN_MARK_ADD | tc->mark_flags,
+				   tc->mask, AT_FDCWD, MNTPOINT);
+		tst_res(TPASS,
+			"Adding an inode mark on directory did not fail with "
+			"ENOTDIR error as on non-dir inode");
+
+		if (!(tc->mark_flags & FAN_MARK_ONLYDIR)) {
+			SAFE_FANOTIFY_MARK(fanotify_fd, FAN_MARK_ADD | tc->mark_flags |
+					   FAN_MARK_FILESYSTEM, tc->mask,
+					   AT_FDCWD, FILE1);
+			tst_res(TPASS,
+				"Adding a filesystem mark on non-dir did not fail with "
+				"ENOTDIR error as with an inode mark");
 		}
-		tst_brk(TBROK | TERRNO,
-			"ret=%d, fanotify_mark(%d, FAN_MARK_ADD | %x, %llx, "
-			"AT_FDCWD, %s) failed",
-			ret,
-			fanotify_fd,
-			tc->mark_flags,
-			tc->mask,
-			FILE1);
 	}
 
-	tst_res(TFAIL,
-		"fanotify_fd=%d, ret=%d, fanotify_init(%x, O_RDONLY) and "
-		"fanotify_mark(%d, FAN_MARK_ADD | %x, %llx, AT_FDCWD, %s) did "
-		"not return any errors as expected",
-		fanotify_fd,
-		ret,
-		tc->init_flags,
-		fanotify_fd,
-		tc->mark_flags,
-		tc->mask,
-		FILE1);
 out:
-	SAFE_CLOSE(fanotify_fd);
+	if (fanotify_fd > 0)
+		SAFE_CLOSE(fanotify_fd);
 }
 
 static void do_setup(void)
-- 
2.37.3


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 3/3] fanotify14: Use TST_EXP_FD_ERRNO()

[Amir Goldstein]

On Tue, Sep 6, 2022 at 12:26 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> That greatly simplifies the code.
>
> Signed-off-by: Petr Vorel <pvorel@suse.cz>

Nice cleanup.
You may add:
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

I don't think that this patch conflicts with the patch in my FAN_MARK_IGNORE
series - if there are conflicts they should be trivial, so feel free to
merge these cleanups either before or after merging my series.

Thanks,
Amir.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 3/3] fanotify14: Use TST_EXP_FD_ERRNO()

[Petr Vorel]

> On Tue, Sep 6, 2022 at 12:26 PM Petr Vorel <pvorel@suse.cz> wrote:

> > That greatly simplifies the code.

> > Signed-off-by: Petr Vorel <pvorel@suse.cz>

> Nice cleanup.
> You may add:
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> I don't think that this patch conflicts with the patch in my FAN_MARK_IGNORE
> series - if there are conflicts they should be trivial, so feel free to
> merge these cleanups either before or after merging my series.

Hi Amir,

thanks! I'll wait little longer if there is some feedback from LTP community
before merging.

Kind regards,
Petr

> Thanks,
> Amir.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 3/3] fanotify14: Use TST_EXP_FD_ERRNO()

[Matthew Bobrowski via ltp]

On Tue, Sep 06, 2022 at 11:26:15AM +0200, Petr Vorel wrote:
> That greatly simplifies the code.

This looks OK to me, nice work!

Reviewed-by: Matthew Bobrowski <repnop@google.com>

> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  .../kernel/syscalls/fanotify/fanotify14.c     | 118 +++++-------------
>  1 file changed, 33 insertions(+), 85 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/fanotify/fanotify14.c b/testcases/kernel/syscalls/fanotify/fanotify14.c
> index aa4db586e..47d013c9f 100644
> --- a/testcases/kernel/syscalls/fanotify/fanotify14.c
> +++ b/testcases/kernel/syscalls/fanotify/fanotify14.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
>   * Copyright (c) 2018 Matthew Bobrowski. All Rights Reserved.
> + * Copyright (c) Linux Test Project, 2020-2022
>   *
>   * Started by Matthew Bobrowski <mbobrowski@mbobrowski.org>
>   */
> @@ -48,6 +49,7 @@ static int fan_report_target_fid_unsupported;
>  static struct test_case_t {
>  	unsigned int init_flags;
>  	unsigned int mark_flags;
> +	/* zero mask expects to fail on fanotify_init() */
>  	unsigned long long mask;
>  	int expected_errno;
>  } test_cases[] = {
> @@ -111,7 +113,6 @@ static struct test_case_t {
>  
>  static void do_test(unsigned int number)
>  {
> -	int ret;
>  	struct test_case_t *tc = &test_cases[number];
>  
>  	if (fan_report_target_fid_unsupported && tc->init_flags & FAN_REPORT_TARGET_FID) {
> @@ -120,101 +121,48 @@ static void do_test(unsigned int number)
>  		return;
>  	}
>  
> -	fanotify_fd = fanotify_init(tc->init_flags, O_RDONLY);
> -	if (fanotify_fd < 0) {
> -		if (errno == tc->expected_errno) {
> -			tst_res(TPASS,
> -				"fanotify_fd=%d, fanotify_init(%x, O_RDONLY) "
> -				"failed with error %d as expected",
> -				fanotify_fd,
> -				tc->init_flags, tc->expected_errno);
> -			return;
> -		}
> -		tst_brk(TBROK | TERRNO,
> -			"fanotify_fd=%d, fanotify_init(%x, O_RDONLY) failed",
> -			fanotify_fd,
> -			tc->init_flags);
> -	}
> +	TST_EXP_FD_ERRNO(fanotify_fd = fanotify_init(tc->init_flags, O_RDONLY),
> +			 !tc->mask && tc->expected_errno ? tc->expected_errno : 0);
>  
> -	/*
> -	 * A test case with a mask set to zero indicate that they've been
> -	 * specifically designed to test and fail on the fanotify_init()
> -	 * system call.
> -	 */
> -	if (tc->mask == 0) {
> -		tst_res(TFAIL,
> -			"fanotify_fd=%d fanotify_init(%x, O_RDONLY) "
> -			"unexpectedly succeeded when tests with mask 0 are "
> -			"expected to fail when calling fanotify_init()",
> -			fanotify_fd,
> -			tc->init_flags);
> +	if (fanotify_fd < 0)
> +		return;
> +
> +	if (!tc->mask)
>  		goto out;
> -	}
>  
>  	/* Set mark on non-dir only when expecting error ENOTDIR */
>  	const char *path = tc->expected_errno == ENOTDIR ? FILE1 : MNTPOINT;
>  
> -	ret = fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark_flags,
> -				tc->mask, AT_FDCWD, path);
> -	if (ret < 0) {
> -		if (errno == tc->expected_errno) {
> -			tst_res(TPASS,
> -				"ret=%d, fanotify_mark(%d, FAN_MARK_ADD | %x, "
> -				"%llx, AT_FDCWD, %s) failed with error %d "
> -				"as expected",
> -				ret,
> -				fanotify_fd,
> -				tc->mark_flags,
> -				tc->mask,
> -				path, tc->expected_errno);
> -			/*
> -			 * ENOTDIR are errors for events/flags not allowed on a non-dir inode.
> -			 * Try to set an inode mark on a directory and it should succeed.
> -			 * Try to set directory events in filesystem mark mask on non-dir
> -			 * and it should succeed.
> -			 */
> -			if (tc->expected_errno == ENOTDIR) {
> -				SAFE_FANOTIFY_MARK(fanotify_fd, FAN_MARK_ADD | tc->mark_flags,
> -						   tc->mask, AT_FDCWD, MNTPOINT);
> -				tst_res(TPASS,
> -					"Adding an inode mark on directory did not fail with "
> -					"ENOTDIR error as on non-dir inode");
> -			}
> -			if (tc->expected_errno == ENOTDIR &&
> -			    !(tc->mark_flags & FAN_MARK_ONLYDIR)) {
> -				SAFE_FANOTIFY_MARK(fanotify_fd, FAN_MARK_ADD | tc->mark_flags |
> -						   FAN_MARK_FILESYSTEM, tc->mask,
> -						   AT_FDCWD, FILE1);
> -				tst_res(TPASS,
> -					"Adding a filesystem mark on non-dir did not fail with "
> -					"ENOTDIR error as with an inode mark");
> -			}
> +	TST_EXP_FD_ERRNO(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark_flags,
> +								   tc->mask, AT_FDCWD, path),
> +					 tc->expected_errno);
>  
> -			goto out;
> +	/*
> +	 * ENOTDIR are errors for events/flags not allowed on a non-dir inode.
> +	 * Try to set an inode mark on a directory and it should succeed.
> +	 * Try to set directory events in filesystem mark mask on non-dir
> +	 * and it should succeed.
> +	 */
> +	if (TST_PASS && tc->expected_errno == ENOTDIR) {
> +		SAFE_FANOTIFY_MARK(fanotify_fd, FAN_MARK_ADD | tc->mark_flags,
> +				   tc->mask, AT_FDCWD, MNTPOINT);
> +		tst_res(TPASS,
> +			"Adding an inode mark on directory did not fail with "
> +			"ENOTDIR error as on non-dir inode");
> +
> +		if (!(tc->mark_flags & FAN_MARK_ONLYDIR)) {
> +			SAFE_FANOTIFY_MARK(fanotify_fd, FAN_MARK_ADD | tc->mark_flags |
> +					   FAN_MARK_FILESYSTEM, tc->mask,
> +					   AT_FDCWD, FILE1);
> +			tst_res(TPASS,
> +				"Adding a filesystem mark on non-dir did not fail with "
> +				"ENOTDIR error as with an inode mark");
>  		}
> -		tst_brk(TBROK | TERRNO,
> -			"ret=%d, fanotify_mark(%d, FAN_MARK_ADD | %x, %llx, "
> -			"AT_FDCWD, %s) failed",
> -			ret,
> -			fanotify_fd,
> -			tc->mark_flags,
> -			tc->mask,
> -			FILE1);
>  	}
>  
> -	tst_res(TFAIL,
> -		"fanotify_fd=%d, ret=%d, fanotify_init(%x, O_RDONLY) and "
> -		"fanotify_mark(%d, FAN_MARK_ADD | %x, %llx, AT_FDCWD, %s) did "
> -		"not return any errors as expected",
> -		fanotify_fd,
> -		ret,
> -		tc->init_flags,
> -		fanotify_fd,
> -		tc->mark_flags,
> -		tc->mask,
> -		FILE1);
>  out:
> -	SAFE_CLOSE(fanotify_fd);
> +	if (fanotify_fd > 0)
> +		SAFE_CLOSE(fanotify_fd);
>  }
>  
>  static void do_setup(void)
> -- 
> 2.37.3
> 
/M

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 0/3] fanotify{14,20}: cleanup

[Matthew Bobrowski via ltp]

On Tue, Sep 06, 2022 at 11:26:12AM +0200, Petr Vorel wrote:
> Hi,
> 
> just an example how to further cleanup fanotify tests by using test macros
> from include/tst_test_macros.h. This can wait till Amir's FAN_MARK_IGNORE
> patchset [1] is merged (unless there is going to be v2).
>
> fanotify20 is an example what I'd address in the code, fanotify14 just
> uses newly added TST_EXP_FD_ERRNO() (more cleanup here and actually in
> other tests could be done).

So, are you suggesting that we have a TODO list? ;)

> I also admit code in include/tst_test_macros.h is a bit hard to read due
> being macro. We should probably add some documentation to it.

Documentation is always nice. A lot of time could be saved as a result
of not having to decipher a given macro.

/M

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH 0/3] fanotify{14,20}: cleanup

[Petr Vorel]

> On Tue, Sep 06, 2022 at 11:26:12AM +0200, Petr Vorel wrote:
> > Hi,

> > just an example how to further cleanup fanotify tests by using test macros
> > from include/tst_test_macros.h. This can wait till Amir's FAN_MARK_IGNORE
> > patchset [1] is merged (unless there is going to be v2).

> > fanotify20 is an example what I'd address in the code, fanotify14 just
> > uses newly added TST_EXP_FD_ERRNO() (more cleanup here and actually in
> > other tests could be done).

> So, are you suggesting that we have a TODO list? ;)
Well, I would not dare this :). Amir wrote he's planning to do some cleanup,
but if you kernel maintainers are busy, I can do it. We really appreciate how
well you maintain tests for your kernel subsystem (I wish there were more kernel
maintainers as active as you).

> > I also admit code in include/tst_test_macros.h is a bit hard to read due
> > being macro. We should probably add some documentation to it.

> Documentation is always nice. A lot of time could be saved as a result
> of not having to decipher a given macro.
It's on my TODO list, hopefully I'll do it soon.

Kind regards,
Petr

> /M

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp