| Message ID | 20240320102204.475230-4-pvorel@suse.cz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | fanotify14 on SELinux fix + lib source merge | expand |
Hi! > Reviewed-by: Amir Goldstein <amir73il@gmail.com> > Reviewed-by: Jan Kara <jack@suse.cz> > Co-developed-by: Petr Vorel <pvorel@suse.cz> > Signed-off-by: Mete Durlu <meted@linux.ibm.com> > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > changes v1->v2: > * Do not include library header in fanotify14 (not needed) > > .../kernel/syscalls/fanotify/fanotify14.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/testcases/kernel/syscalls/fanotify/fanotify14.c b/testcases/kernel/syscalls/fanotify/fanotify14.c > index d02d81495..82725f317 100644 > --- a/testcases/kernel/syscalls/fanotify/fanotify14.c > +++ b/testcases/kernel/syscalls/fanotify/fanotify14.c > @@ -47,6 +47,7 @@ static int pipes[2] = {-1, -1}; > static int fanotify_fd; > static int ignore_mark_unsupported; > static int filesystem_mark_unsupported; > +static int se_enforcing; > static unsigned int supported_init_flags; > > struct test_case_flags_t { > @@ -283,9 +284,18 @@ static void do_test(unsigned int number) > > tst_res(TINFO, "Testing %s with %s", > tc->mark.desc, tc->mask.desc); > - TST_EXP_FD_OR_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > - tc->mask.flags, dirfd, path), > - tc->expected_errno); > + > + if (tc->pfd && se_enforcing) { > + const int exp_errs[] = {tc->expected_errno, EACCES}; > + > + TST_EXP_FAIL_ARR(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > + tc->mask.flags, dirfd, path), > + exp_errs); > + } else { > + TST_EXP_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > + tc->mask.flags, dirfd, path), > + tc->expected_errno); > + } This looks like the test library is not flexible enough to make this simpler. Maybe having the ARRAY_SIZE() in the TST_EXP_FAIL_ARR wasn't a good idea after all. Or maybe we just need TST_EXP_FAIL_ARR2() that would take the array size explicitly, then we could do: const int exp_errs[] = {tc->expected_errno, EACESS} TST_EXP_FAIL_ARR2(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, tc->mask.flags, dirfd, path), exp_errs, se_enforcing ? 1 : 2); > /* > * ENOTDIR are errors for events/flags not allowed on a non-dir inode. > @@ -334,6 +344,8 @@ static void do_setup(void) > SAFE_FILE_PRINTF(FILE1, "0"); > /* Create anonymous pipes to place marks on */ > SAFE_PIPE2(pipes, O_CLOEXEC); > + > + se_enforcing = tst_selinux_enforcing(); > } > > static void do_cleanup(void) > -- > 2.43.0 >
> Hi! > > Reviewed-by: Amir Goldstein <amir73il@gmail.com> > > Reviewed-by: Jan Kara <jack@suse.cz> > > Co-developed-by: Petr Vorel <pvorel@suse.cz> > > Signed-off-by: Mete Durlu <meted@linux.ibm.com> > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > --- > > changes v1->v2: > > * Do not include library header in fanotify14 (not needed) > > .../kernel/syscalls/fanotify/fanotify14.c | 18 +++++++++++++++--- > > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/testcases/kernel/syscalls/fanotify/fanotify14.c b/testcases/kernel/syscalls/fanotify/fanotify14.c > > index d02d81495..82725f317 100644 > > --- a/testcases/kernel/syscalls/fanotify/fanotify14.c > > +++ b/testcases/kernel/syscalls/fanotify/fanotify14.c > > @@ -47,6 +47,7 @@ static int pipes[2] = {-1, -1}; > > static int fanotify_fd; > > static int ignore_mark_unsupported; > > static int filesystem_mark_unsupported; > > +static int se_enforcing; > > static unsigned int supported_init_flags; > > struct test_case_flags_t { > > @@ -283,9 +284,18 @@ static void do_test(unsigned int number) > > tst_res(TINFO, "Testing %s with %s", > > tc->mark.desc, tc->mask.desc); > > - TST_EXP_FD_OR_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > > - tc->mask.flags, dirfd, path), > > - tc->expected_errno); > > + > > + if (tc->pfd && se_enforcing) { > > + const int exp_errs[] = {tc->expected_errno, EACCES}; > > + > > + TST_EXP_FAIL_ARR(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > > + tc->mask.flags, dirfd, path), > > + exp_errs); > > + } else { > > + TST_EXP_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > > + tc->mask.flags, dirfd, path), > > + tc->expected_errno); > > + } > This looks like the test library is not flexible enough to make this > simpler. Maybe having the ARRAY_SIZE() in the TST_EXP_FAIL_ARR wasn't a > good idea after all. Or maybe we just need TST_EXP_FAIL_ARR2() that > would take the array size explicitly, then we could do: > const int exp_errs[] = {tc->expected_errno, EACESS} > TST_EXP_FAIL_ARR2(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > tc->mask.flags, dirfd, path), exp_errs, > se_enforcing ? 1 : 2); This LGTM, although it makes tst_test_macros.h even harder to read. I'll send another version. Kind regards, Petr > > /* > > * ENOTDIR are errors for events/flags not allowed on a non-dir inode. > > @@ -334,6 +344,8 @@ static void do_setup(void) > > SAFE_FILE_PRINTF(FILE1, "0"); > > /* Create anonymous pipes to place marks on */ > > SAFE_PIPE2(pipes, O_CLOEXEC); > > + > > + se_enforcing = tst_selinux_enforcing(); > > } > > static void do_cleanup(void) > > -- > > 2.43.0
H > Hi! > > Reviewed-by: Amir Goldstein <amir73il@gmail.com> > > Reviewed-by: Jan Kara <jack@suse.cz> > > Co-developed-by: Petr Vorel <pvorel@suse.cz> > > Signed-off-by: Mete Durlu <meted@linux.ibm.com> > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > --- > > changes v1->v2: > > * Do not include library header in fanotify14 (not needed) > > .../kernel/syscalls/fanotify/fanotify14.c | 18 +++++++++++++++--- > > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/testcases/kernel/syscalls/fanotify/fanotify14.c b/testcases/kernel/syscalls/fanotify/fanotify14.c > > index d02d81495..82725f317 100644 > > --- a/testcases/kernel/syscalls/fanotify/fanotify14.c > > +++ b/testcases/kernel/syscalls/fanotify/fanotify14.c > > @@ -47,6 +47,7 @@ static int pipes[2] = {-1, -1}; > > static int fanotify_fd; > > static int ignore_mark_unsupported; > > static int filesystem_mark_unsupported; > > +static int se_enforcing; > > static unsigned int supported_init_flags; > > struct test_case_flags_t { > > @@ -283,9 +284,18 @@ static void do_test(unsigned int number) > > tst_res(TINFO, "Testing %s with %s", > > tc->mark.desc, tc->mask.desc); > > - TST_EXP_FD_OR_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > > - tc->mask.flags, dirfd, path), > > - tc->expected_errno); > > + > > + if (tc->pfd && se_enforcing) { > > + const int exp_errs[] = {tc->expected_errno, EACCES}; > > + > > + TST_EXP_FAIL_ARR(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > > + tc->mask.flags, dirfd, path), > > + exp_errs); > > + } else { > > + TST_EXP_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > > + tc->mask.flags, dirfd, path), > > + tc->expected_errno); > > + } > This looks like the test library is not flexible enough to make this > simpler. Maybe having the ARRAY_SIZE() in the TST_EXP_FAIL_ARR wasn't a > good idea after all. Or maybe we just need TST_EXP_FAIL_ARR2() that > would take the array size explicitly, then we could do: [ Removing Jan and Amir from the LTP specific discussion ] @Cyril, @Li: How about add 2 macros with '_SIZE' in the name? (TST_EXP_FAIL_ARR_SIZE and TST_EXP_FAIL2_ARR_SIZE, see diff at the end). Other option would be to add '2' (TST_EXP_FAIL_ARR2 and TST_EXP_FAIL2_ARR2), not sure what is more readable. Kind regards, Petr > const int exp_errs[] = {tc->expected_errno, EACESS} > TST_EXP_FAIL_ARR2(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > tc->mask.flags, dirfd, path), exp_errs, > se_enforcing ? 1 : 2); +++ include/tst_test_macros.h @@ -246,6 +246,10 @@ const char *tst_errno_names(char *buf, const int *exp_errs, int exp_errs_cnt); TST_EXP_FAIL_ARR_(SCALL, #SCALL, EXP_ERRS, \ ARRAY_SIZE(EXP_ERRS), ##__VA_ARGS__); +#define TST_EXP_FAIL_ARR_SIZE(SCALL, EXP_ERRS, EXP_ERRS_CNT, ...) \ + TST_EXP_FAIL_ARR_(SCALL, #SCALL, EXP_ERRS, \ + EXP_ERRS_CNT, ##__VA_ARGS__); + #define TST_EXP_FAIL2_ARR_(SCALL, SSCALL, EXP_ERRS, EXP_ERRS_CNT, ...) \ do { \ TST_EXP_FAIL_SILENT_(TST_RET >= 0, SCALL, SSCALL, \ @@ -258,6 +262,10 @@ const char *tst_errno_names(char *buf, const int *exp_errs, int exp_errs_cnt); TST_EXP_FAIL2_ARR_(SCALL, #SCALL, EXP_ERRS, \ ARRAY_SIZE(EXP_ERRS), ##__VA_ARGS__); +#define TST_EXP_FAIL2_ARR_SIZE(SCALL, EXP_ERRS, EXP_ERRS_CNT...) \ + TST_EXP_FAIL2_ARR_(SCALL, #SCALL, EXP_ERRS, \ + EXP_ERRS_CNT, ##__VA_ARGS__); + #define TST_EXP_FAIL2(SCALL, EXP_ERR, ...) \ do { \ int tst_exp_err__ = EXP_ERR; \
Hi! > @Cyril, @Li: How about add 2 macros with '_SIZE' in the name? > (TST_EXP_FAIL_ARR_SIZE and TST_EXP_FAIL2_ARR_SIZE, see diff at the end). Maybe just _SZ instead of full _SIZE? Also the TST_EXP_FAIL_ARR() is rarely used so we may as well to change it to include the size argument explicitly, the worst case is that user would have to pass down ARRAY_SIZE(errnos), which is just one more argument. The macro is used by a single test at the moment so it's easy now. Doing that would at least slown down the combinatoric explosion of the test macros.
Hi Cyril, > Hi! > > @Cyril, @Li: How about add 2 macros with '_SIZE' in the name? > > (TST_EXP_FAIL_ARR_SIZE and TST_EXP_FAIL2_ARR_SIZE, see diff at the end). > Maybe just _SZ instead of full _SIZE? > Also the TST_EXP_FAIL_ARR() is rarely used so we may as well to change > it to include the size argument explicitly, the worst case is that user > would have to pass down ARRAY_SIZE(errnos), which is just one more > argument. The macro is used by a single test at the moment so it's easy > now. Doing that would at least slown down the combinatoric explosion of > the test macros. Good point, I just replace ARRAY_SIZE(errnos) with size parameter in the next version. Thanks a lot for your input. Kind regards, Petr
diff --git a/testcases/kernel/syscalls/fanotify/fanotify14.c b/testcases/kernel/syscalls/fanotify/fanotify14.c index d02d81495..82725f317 100644 --- a/testcases/kernel/syscalls/fanotify/fanotify14.c +++ b/testcases/kernel/syscalls/fanotify/fanotify14.c @@ -47,6 +47,7 @@ static int pipes[2] = {-1, -1}; static int fanotify_fd; static int ignore_mark_unsupported; static int filesystem_mark_unsupported; +static int se_enforcing; static unsigned int supported_init_flags; struct test_case_flags_t { @@ -283,9 +284,18 @@ static void do_test(unsigned int number) tst_res(TINFO, "Testing %s with %s", tc->mark.desc, tc->mask.desc); - TST_EXP_FD_OR_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, - tc->mask.flags, dirfd, path), - tc->expected_errno); + + if (tc->pfd && se_enforcing) { + const int exp_errs[] = {tc->expected_errno, EACCES}; + + TST_EXP_FAIL_ARR(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, + tc->mask.flags, dirfd, path), + exp_errs); + } else { + TST_EXP_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, + tc->mask.flags, dirfd, path), + tc->expected_errno); + } /* * ENOTDIR are errors for events/flags not allowed on a non-dir inode. @@ -334,6 +344,8 @@ static void do_setup(void) SAFE_FILE_PRINTF(FILE1, "0"); /* Create anonymous pipes to place marks on */ SAFE_PIPE2(pipes, O_CLOEXEC); + + se_enforcing = tst_selinux_enforcing(); } static void do_cleanup(void)