| Message ID | 20200623001547.22255-5-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | kselftest: fix TAP output for skipped test and ksft_set_plan misuse | expand |
On Mon, Jun 22, 2020 at 08:15:45PM -0400, Paolo Bonzini wrote: > Calling ksft_exit_skip after ksft_set_plan results in executing fewer tests > than planned. Use ksft_test_result_skip instead. > > The plan passed to ksft_set_plan was wrong, too, so fix it while at it. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- Thanks for the patch! Hm, this series misses a bunch of Cces for the maintainers of these files... (Also note that Kees has a/some series with most of us Cced that might conflict with some of these changes. But not sure rn.) A comment below. > tools/testing/selftests/pidfd/pidfd_test.c | 39 ++++++++++++++++++---- > 1 file changed, 33 insertions(+), 6 deletions(-) > > diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c > index 7aff2d3b42c0..380c6314e6a2 100644 > --- a/tools/testing/selftests/pidfd/pidfd_test.c > +++ b/tools/testing/selftests/pidfd/pidfd_test.c > @@ -8,6 +8,7 @@ > #include <sched.h> > #include <signal.h> > #include <stdio.h> > +#include <stdbool.h> > #include <stdlib.h> > #include <string.h> > #include <syscall.h> > @@ -27,6 +28,8 @@ > > #define MAX_EVENTS 5 > > +static bool have_pidfd_send_signal = false; > + > static pid_t pidfd_clone(int flags, int *pidfd, int (*fn)(void *)) > { > size_t stack_size = 1024; > @@ -56,6 +59,13 @@ static int test_pidfd_send_signal_simple_success(void) > int pidfd, ret; > const char *test_name = "pidfd_send_signal send SIGUSR1"; > > + if (!have_pidfd_send_signal) { > + ksft_test_result_skip( > + "%s test: pidfd_send_signal() syscall not supported\n", > + test_name); > + return 0; > + } > + > pidfd = open("/proc/self", O_DIRECTORY | O_CLOEXEC); > if (pidfd < 0) > ksft_exit_fail_msg( > @@ -86,6 +96,13 @@ static int test_pidfd_send_signal_exited_fail(void) > pid_t pid; > const char *test_name = "pidfd_send_signal signal exited process"; > > + if (!have_pidfd_send_signal) { > + ksft_test_result_skip( > + "%s test: pidfd_send_signal() syscall not supported\n", > + test_name); > + return 0; > + } > + > pid = fork(); > if (pid < 0) > ksft_exit_fail_msg("%s test: Failed to create new process\n", > @@ -137,6 +154,13 @@ static int test_pidfd_send_signal_recycled_pid_fail(void) > pid_t pid1; > const char *test_name = "pidfd_send_signal signal recycled pid"; > > + if (!have_pidfd_send_signal) { > + ksft_test_result_skip( > + "%s test: pidfd_send_signal() syscall not supported\n", > + test_name); > + return 0; > + } > + > ret = unshare(CLONE_NEWPID); > if (ret < 0) > ksft_exit_fail_msg("%s test: Failed to unshare pid namespace\n", > @@ -325,13 +349,16 @@ static int test_pidfd_send_signal_syscall_support(void) > > ret = sys_pidfd_send_signal(pidfd, 0, NULL, 0); > if (ret < 0) { > - if (errno == ENOSYS) > - ksft_exit_skip( > + if (errno == ENOSYS) { > + ksft_test_result_skip( > "%s test: pidfd_send_signal() syscall not supported\n", > test_name); If pidfd_send_signal() is not supported, you're falling through and then you're reporting: ok 5 # SKIP pidfd_send_signal check for support test: pidfd_send_signal() syscall not supported ok 6 pidfd_send_signal check for support test: pidfd_send_signal() syscall is supported. Tests can be executed which seems wrong. > - > - ksft_exit_fail_msg("%s test: Failed to send signal\n", > - test_name); > + } else { > + ksft_exit_fail_msg("%s test: Failed to send signal\n", > + test_name); > + } > + } else { > + have_pidfd_send_signal = true; > } > > close(pidfd); > @@ -521,7 +548,7 @@ static void test_pidfd_poll_leader_exit(int use_waitpid) > int main(int argc, char **argv) > { > ksft_print_header(); > - ksft_set_plan(4); > + ksft_set_plan(8); > > test_pidfd_poll_exec(0); > test_pidfd_poll_exec(1); > -- > 2.26.2 > >
On 23/06/20 22:44, Christian Brauner wrote: >> ret = sys_pidfd_send_signal(pidfd, 0, NULL, 0); >> if (ret < 0) { >> - if (errno == ENOSYS) >> - ksft_exit_skip( >> + if (errno == ENOSYS) { >> + ksft_test_result_skip( >> "%s test: pidfd_send_signal() syscall not supported\n", >> test_name); > If pidfd_send_signal() is not supported, you're falling through and then > you're reporting: > > ok 5 # SKIP pidfd_send_signal check for support test: pidfd_send_signal() syscall not supported > ok 6 pidfd_send_signal check for support test: pidfd_send_signal() syscall is supported. Tests can be executed You're right, this needs a "return". Paolo
On 6/24/20 12:21 AM, Paolo Bonzini wrote: > On 23/06/20 22:44, Christian Brauner wrote: >>> ret = sys_pidfd_send_signal(pidfd, 0, NULL, 0); >>> if (ret < 0) { >>> - if (errno == ENOSYS) >>> - ksft_exit_skip( >>> + if (errno == ENOSYS) { >>> + ksft_test_result_skip( >>> "%s test: pidfd_send_signal() syscall not supported\n", >>> test_name); >> If pidfd_send_signal() is not supported, you're falling through and then >> you're reporting: >> >> ok 5 # SKIP pidfd_send_signal check for support test: pidfd_send_signal() syscall not supported >> ok 6 pidfd_send_signal check for support test: pidfd_send_signal() syscall is supported. Tests can be executed > > You're right, this needs a "return". > Hi Paulo, I am applying the rest of the patches in this series except this one. Please send v3 for this. thanks, -- Shuah
On 06/07/20 22:55, Shuah Khan wrote: > On 6/24/20 12:21 AM, Paolo Bonzini wrote: >> On 23/06/20 22:44, Christian Brauner wrote: >>>> ret = sys_pidfd_send_signal(pidfd, 0, NULL, 0); >>>> if (ret < 0) { >>>> - if (errno == ENOSYS) >>>> - ksft_exit_skip( >>>> + if (errno == ENOSYS) { >>>> + ksft_test_result_skip( >>>> "%s test: pidfd_send_signal() syscall not >>>> supported\n", >>>> test_name); >>> If pidfd_send_signal() is not supported, you're falling through and then >>> you're reporting: >>> >>> ok 5 # SKIP pidfd_send_signal check for support test: >>> pidfd_send_signal() syscall not supported >>> ok 6 pidfd_send_signal check for support test: pidfd_send_signal() >>> syscall is supported. Tests can be executed >> >> You're right, this needs a "return". >> > > Hi Paulo, > > I am applying the rest of the patches in this series except this one. > Please send v3 for this. Thanks, I was actually going to send everything but you're making it even simpler. I'll send v3 of this patch only. Paolo
diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c index 7aff2d3b42c0..380c6314e6a2 100644 --- a/tools/testing/selftests/pidfd/pidfd_test.c +++ b/tools/testing/selftests/pidfd/pidfd_test.c @@ -8,6 +8,7 @@ #include <sched.h> #include <signal.h> #include <stdio.h> +#include <stdbool.h> #include <stdlib.h> #include <string.h> #include <syscall.h> @@ -27,6 +28,8 @@ #define MAX_EVENTS 5 +static bool have_pidfd_send_signal = false; + static pid_t pidfd_clone(int flags, int *pidfd, int (*fn)(void *)) { size_t stack_size = 1024; @@ -56,6 +59,13 @@ static int test_pidfd_send_signal_simple_success(void) int pidfd, ret; const char *test_name = "pidfd_send_signal send SIGUSR1"; + if (!have_pidfd_send_signal) { + ksft_test_result_skip( + "%s test: pidfd_send_signal() syscall not supported\n", + test_name); + return 0; + } + pidfd = open("/proc/self", O_DIRECTORY | O_CLOEXEC); if (pidfd < 0) ksft_exit_fail_msg( @@ -86,6 +96,13 @@ static int test_pidfd_send_signal_exited_fail(void) pid_t pid; const char *test_name = "pidfd_send_signal signal exited process"; + if (!have_pidfd_send_signal) { + ksft_test_result_skip( + "%s test: pidfd_send_signal() syscall not supported\n", + test_name); + return 0; + } + pid = fork(); if (pid < 0) ksft_exit_fail_msg("%s test: Failed to create new process\n", @@ -137,6 +154,13 @@ static int test_pidfd_send_signal_recycled_pid_fail(void) pid_t pid1; const char *test_name = "pidfd_send_signal signal recycled pid"; + if (!have_pidfd_send_signal) { + ksft_test_result_skip( + "%s test: pidfd_send_signal() syscall not supported\n", + test_name); + return 0; + } + ret = unshare(CLONE_NEWPID); if (ret < 0) ksft_exit_fail_msg("%s test: Failed to unshare pid namespace\n", @@ -325,13 +349,16 @@ static int test_pidfd_send_signal_syscall_support(void) ret = sys_pidfd_send_signal(pidfd, 0, NULL, 0); if (ret < 0) { - if (errno == ENOSYS) - ksft_exit_skip( + if (errno == ENOSYS) { + ksft_test_result_skip( "%s test: pidfd_send_signal() syscall not supported\n", test_name); - - ksft_exit_fail_msg("%s test: Failed to send signal\n", - test_name); + } else { + ksft_exit_fail_msg("%s test: Failed to send signal\n", + test_name); + } + } else { + have_pidfd_send_signal = true; } close(pidfd); @@ -521,7 +548,7 @@ static void test_pidfd_poll_leader_exit(int use_waitpid) int main(int argc, char **argv) { ksft_print_header(); - ksft_set_plan(4); + ksft_set_plan(8); test_pidfd_poll_exec(0); test_pidfd_poll_exec(1);
Calling ksft_exit_skip after ksft_set_plan results in executing fewer tests than planned. Use ksft_test_result_skip instead. The plan passed to ksft_set_plan was wrong, too, so fix it while at it. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- tools/testing/selftests/pidfd/pidfd_test.c | 39 ++++++++++++++++++---- 1 file changed, 33 insertions(+), 6 deletions(-)