This is v2 of the patch to fix TAP output for skipped tests. I noticed and fixed two other occurrences of "not ok ... # SKIP" which according to the TAP specification should be marked as "ok ... # SKIP" instead.
Unfortunately, closer analysis showed ksft_exit_skip to be a badly misused API. It should be used when the remainder of the testcase is being skipped, but TAP only supports this before the test plan has been emitted (in which case you're supposed to print "1..0 # SKIP".
Therefore, in patch 1 I'm mostly trying to do something sensible, printing "1..0 # SKIP" is possible or "ok ... # SKIP" if not (which is no worse than what was doing before). The remaining five patches show what needs to be done in order to avoid ksft_exit_skip misuse; while working on them I found other bugs that I've fixed at the same time; see patch 2 for an example.
In the interest of full disclosure, I won't be able to do more cleanups of ksft_exit_skip callers. However, I have fixed all those that did call ksft_set_plan() as those at least try to produce TAP output.
Paolo
Paolo Bonzini (6): kselftest: fix TAP output for skipped tests selftests: breakpoints: fix computation of test plan selftests: breakpoints: do not use ksft_exit_skip after ksft_set_plan selftests: pidfd: do not use ksft_exit_skip after ksft_set_plan selftests: sigaltstack: do not use ksft_exit_skip after ksft_set_plan selftests: sync_test: do not use ksft_exit_skip after ksft_set_plan
.../breakpoints/step_after_suspend_test.c | 53 +++++++++++-------- tools/testing/selftests/kselftest.h | 28 +++++++--- tools/testing/selftests/kselftest/runner.sh | 2 +- tools/testing/selftests/pidfd/pidfd_test.c | 39 +++++++++++--- tools/testing/selftests/sigaltstack/sas.c | 4 +- tools/testing/selftests/sync/sync_test.c | 2 +- 6 files changed, 87 insertions(+), 41 deletions(-)
According to the TAP specification, a skipped test must be marked as "ok" and annotated with the SKIP directive, for example
ok 23 # skip Insufficient flogiston pressure. (https://testanything.org/tap-specification.html)
Fix the kselftest infrastructure to match this.
For ksft_exit_skip, it is preferrable to emit a dummy plan line that indicates the whole test was skipped, but this is not always possible because of ksft_exit_skip being used as a "shortcut" by the tests. In that case, print the test counts and a normal "ok" line. The format is now the same independent of whether msg is NULL or not (but it is never NULL in any caller right now).
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- tools/testing/selftests/kselftest.h | 28 +++++++++++++++------ tools/testing/selftests/kselftest/runner.sh | 2 +- 2 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h index 862eee734553..1b0075359734 100644 --- a/tools/testing/selftests/kselftest.h +++ b/tools/testing/selftests/kselftest.h @@ -128,7 +128,7 @@ static inline void ksft_test_result_skip(const char *msg, ...) ksft_cnt.ksft_xskip++;
va_start(args, msg); - printf("not ok %d # SKIP ", ksft_test_num()); + printf("ok %d # SKIP ", ksft_test_num()); errno = saved_errno; vprintf(msg, args); va_end(args); @@ -190,18 +190,30 @@ static inline int ksft_exit_xpass(void)
static inline int ksft_exit_skip(const char *msg, ...) { - if (msg) { - int saved_errno = errno; - va_list args; + int saved_errno = errno; + va_list args;
- va_start(args, msg); - printf("not ok %d # SKIP ", 1 + ksft_test_num()); + va_start(args, msg); + + /* + * FIXME: several tests misuse ksft_exit_skip so produce + * something sensible if some tests have already been run + * or a plan has been printed. Those tests should use + * ksft_test_result_skip or ksft_exit_fail_msg instead. + */ + if (ksft_plan || ksft_test_num()) { + ksft_cnt.ksft_xskip++; + printf("ok %d # SKIP ", 1 + ksft_test_num()); + } else { + printf("1..0 # SKIP "); + } + if (msg) { errno = saved_errno; vprintf(msg, args); va_end(args); - } else { - ksft_print_cnts(); } + if (ksft_test_num()) + ksft_print_cnts(); exit(KSFT_SKIP); }
diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh index 676b3a8b114d..f4815cbcd60f 100644 --- a/tools/testing/selftests/kselftest/runner.sh +++ b/tools/testing/selftests/kselftest/runner.sh @@ -77,7 +77,7 @@ run_one() echo "ok $test_num $TEST_HDR_MSG") || (rc=$?; \ if [ $rc -eq $skip_rc ]; then \ - echo "not ok $test_num $TEST_HDR_MSG # SKIP" + echo "ok $test_num $TEST_HDR_MSG # SKIP" elif [ $rc -eq $timeout_rc ]; then \ echo "#" echo "not ok $test_num $TEST_HDR_MSG # TIMEOUT"
The computation of the test plan uses the available_cpus bitset before calling sched_getaffinity to fill it in. The resulting plan is bogus, fix it.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- .../selftests/breakpoints/step_after_suspend_test.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/breakpoints/step_after_suspend_test.c b/tools/testing/selftests/breakpoints/step_after_suspend_test.c index b3ead29c6089..983ee6182e25 100644 --- a/tools/testing/selftests/breakpoints/step_after_suspend_test.c +++ b/tools/testing/selftests/breakpoints/step_after_suspend_test.c @@ -183,6 +183,10 @@ int main(int argc, char **argv) } }
+ err = sched_getaffinity(0, sizeof(available_cpus), &available_cpus); + if (err < 0) + ksft_exit_fail_msg("sched_getaffinity() failed\n"); + for (cpu = 0; cpu < CPU_SETSIZE; cpu++) { if (!CPU_ISSET(cpu, &available_cpus)) continue; @@ -193,10 +197,6 @@ int main(int argc, char **argv) if (do_suspend) suspend();
- err = sched_getaffinity(0, sizeof(available_cpus), &available_cpus); - if (err < 0) - ksft_exit_fail_msg("sched_getaffinity() failed\n"); - for (cpu = 0; cpu < CPU_SETSIZE; cpu++) { bool test_success;
Calling ksft_exit_skip after ksft_set_plan results in executing fewer tests than planned. Use ksft_test_result_skip for the individual tests. The call in suspend() is fine, but ksft_set_plan should be after it.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- .../breakpoints/step_after_suspend_test.c | 45 +++++++++++-------- 1 file changed, 26 insertions(+), 19 deletions(-)
diff --git a/tools/testing/selftests/breakpoints/step_after_suspend_test.c b/tools/testing/selftests/breakpoints/step_after_suspend_test.c index 983ee6182e25..2cf6f10ab7c4 100644 --- a/tools/testing/selftests/breakpoints/step_after_suspend_test.c +++ b/tools/testing/selftests/breakpoints/step_after_suspend_test.c @@ -47,7 +47,7 @@ void child(int cpu) _exit(0); }
-bool run_test(int cpu) +int run_test(int cpu) { int status; pid_t pid = fork(); @@ -55,7 +55,7 @@ bool run_test(int cpu)
if (pid < 0) { ksft_print_msg("fork() failed: %s\n", strerror(errno)); - return false; + return KSFT_FAIL; } if (pid == 0) child(cpu); @@ -63,67 +63,68 @@ bool run_test(int cpu) wpid = waitpid(pid, &status, __WALL); if (wpid != pid) { ksft_print_msg("waitpid() failed: %s\n", strerror(errno)); - return false; + return KSFT_FAIL; } if (!WIFSTOPPED(status)) { ksft_print_msg("child did not stop: %s\n", strerror(errno)); - return false; + return KSFT_FAIL; } if (WSTOPSIG(status) != SIGSTOP) { ksft_print_msg("child did not stop with SIGSTOP: %s\n", strerror(errno)); - return false; + return KSFT_FAIL; }
if (ptrace(PTRACE_SINGLESTEP, pid, NULL, NULL) < 0) { if (errno == EIO) { - ksft_exit_skip( + ksft_print_msg( "ptrace(PTRACE_SINGLESTEP) not supported on this architecture: %s\n", strerror(errno)); + return KSFT_SKIP; } ksft_print_msg("ptrace(PTRACE_SINGLESTEP) failed: %s\n", strerror(errno)); - return false; + return KSFT_FAIL; }
wpid = waitpid(pid, &status, __WALL); if (wpid != pid) { ksft_print_msg("waitpid() failed: $s\n", strerror(errno)); - return false; + return KSFT_FAIL; } if (WIFEXITED(status)) { ksft_print_msg("child did not single-step: %s\n", strerror(errno)); - return false; + return KSFT_FAIL; } if (!WIFSTOPPED(status)) { ksft_print_msg("child did not stop: %s\n", strerror(errno)); - return false; + return KSFT_FAIL; } if (WSTOPSIG(status) != SIGTRAP) { ksft_print_msg("child did not stop with SIGTRAP: %s\n", strerror(errno)); - return false; + return KSFT_FAIL; }
if (ptrace(PTRACE_CONT, pid, NULL, NULL) < 0) { ksft_print_msg("ptrace(PTRACE_CONT) failed: %s\n", strerror(errno)); - return false; + return KSFT_FAIL; }
wpid = waitpid(pid, &status, __WALL); if (wpid != pid) { ksft_print_msg("waitpid() failed: %s\n", strerror(errno)); - return false; + return KSFT_FAIL; } if (!WIFEXITED(status)) { ksft_print_msg("child did not exit after PTRACE_CONT: %s\n", strerror(errno)); - return false; + return KSFT_FAIL; }
- return true; + return KSFT_PASS; }
void suspend(void) @@ -192,23 +193,29 @@ int main(int argc, char **argv) continue; tests++; } - ksft_set_plan(tests);
if (do_suspend) suspend();
+ ksft_set_plan(tests); for (cpu = 0; cpu < CPU_SETSIZE; cpu++) { - bool test_success; + int test_success;
if (!CPU_ISSET(cpu, &available_cpus)) continue;
test_success = run_test(cpu); - if (test_success) { + switch (test_success) { + case KSFT_PASS: ksft_test_result_pass("CPU %d\n", cpu); - } else { + break; + case KSFT_SKIP: + ksft_test_result_skip("CPU %d\n", cpu); + break; + case KSFT_FAIL: ksft_test_result_fail("CPU %d\n", cpu); succeeded = false; + break; } }
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(-)
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);
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
Calling ksft_exit_skip after ksft_set_plan results in executing fewer tests than planned. Use ksft_test_result_skip when possible, or just bail out if memory corruption is detected.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- tools/testing/selftests/sigaltstack/sas.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/sigaltstack/sas.c b/tools/testing/selftests/sigaltstack/sas.c index ad0f8df2ca0a..8934a3766d20 100644 --- a/tools/testing/selftests/sigaltstack/sas.c +++ b/tools/testing/selftests/sigaltstack/sas.c @@ -71,7 +71,7 @@ void my_usr1(int sig, siginfo_t *si, void *u) swapcontext(&sc, &uc); ksft_print_msg("%s\n", p->msg); if (!p->flag) { - ksft_exit_skip("[RUN]\tAborting\n"); + ksft_exit_fail_msg("[RUN]\tAborting\n"); exit(EXIT_FAILURE); } } @@ -144,7 +144,7 @@ int main(void) err = sigaltstack(&stk, NULL); if (err) { if (errno == EINVAL) { - ksft_exit_skip( + ksft_test_result_skip( "[NOTE]\tThe running kernel doesn't support SS_AUTODISARM\n"); /* * If test cases for the !SS_AUTODISARM variant were
Calling ksft_exit_skip after ksft_set_plan results in executing fewer tests than planned. Move it before.
Signed-off-by: Paolo Bonzini pbonzini@redhat.com --- tools/testing/selftests/sync/sync_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/sync/sync_test.c b/tools/testing/selftests/sync/sync_test.c index 3824b66f41a0..414a617db993 100644 --- a/tools/testing/selftests/sync/sync_test.c +++ b/tools/testing/selftests/sync/sync_test.c @@ -86,9 +86,9 @@ int main(void) int err;
ksft_print_header(); - ksft_set_plan(3 + 7);
sync_api_supported(); + ksft_set_plan(3 + 7);
ksft_print_msg("[RUN]\tTesting sync framework\n");
linux-kselftest-mirror@lists.linaro.org
-
Christian Brauner -
Paolo Bonzini -
Shuah Khan