[PATCH v2 0/6] kselftest: fix TAP output for skipped test and ksft_set_plan misuse

[PATCH v2 0/6] kselftest: fix TAP output for skipped test and ksft_set_plan misuse

[Paolo Bonzini]

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(-)

-- 
2.26.2

[PATCH v2 1/6] kselftest: fix TAP output for skipped tests

[Paolo Bonzini]

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"
-- 
2.26.2

[PATCH v2 2/6] selftests: breakpoints: fix computation of test plan

[Paolo Bonzini]

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;
 
-- 
2.26.2

[PATCH v2 3/6] selftests: breakpoints: do not use ksft_exit_skip after ksft_set_plan

[Paolo Bonzini]

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;
 		}
 	}
 
-- 
2.26.2

[PATCH v2 4/6] selftests: pidfd: do not use ksft_exit_skip after ksft_set_plan

[Paolo Bonzini]

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);
-- 
2.26.2

[PATCH v2 5/6] selftests: sigaltstack: do not use ksft_exit_skip after ksft_set_plan

[Paolo Bonzini]

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
-- 
2.26.2

[PATCH v2 6/6] selftests: sync_test: do not use ksft_exit_skip after ksft_set_plan

[Paolo Bonzini]

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");
 
-- 
2.26.2

Re: [PATCH v2 4/6] selftests: pidfd: do not use ksft_exit_skip after ksft_set_plan

[Christian Brauner]

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
> 
> 

Re: [PATCH v2 4/6] selftests: pidfd: do not use ksft_exit_skip after ksft_set_plan

[Paolo Bonzini]

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

Re: [PATCH v2 4/6] selftests: pidfd: do not use ksft_exit_skip after ksft_set_plan

[Shuah Khan]

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

Re: [PATCH v2 4/6] selftests: pidfd: do not use ksft_exit_skip after ksft_set_plan

[Paolo Bonzini]

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