| Message ID | 20220906092615.15116-3-pvorel@suse.cz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | fanotify{14,20}: cleanup | expand |
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.
> 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.
... > > 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
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
> 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
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 = {
* 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(-)