[PATCH v3 0/2] selftests: pidfd: prefer ksft_test_result_skip to ksft_exit_*

[PATCH v3 0/2] selftests: pidfd: prefer ksft_test_result_skip to ksft_exit_*

[Paolo Bonzini]

Calling ksft_exit_* results in executing fewer tests than planned, which
is wrong for ksft_exit_skip or suboptimal (because it results in a bail
out) for ksft_exit_fail_msg.

Using ksft_test_result_skip instead skips only one test and lets the
test plan proceed as promised by ksft_set_plan.

Paolo

Paolo Bonzini (2):
  selftests: pidfd: do not use ksft_exit_skip after ksft_set_plan
  selftests: pidfd: skip test if unshare fails with EPERM

 tools/testing/selftests/pidfd/pidfd_test.c | 55 ++++++++++++++++++----
 1 file changed, 46 insertions(+), 9 deletions(-)

-- 
2.26.2

[PATCH v3 1/2] 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>
Message-Id: <20200623001547.22255-5-pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tools/testing/selftests/pidfd/pidfd_test.c | 34 +++++++++++++++++++---
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c
index 7aff2d3b42c0..f65ad4e32353 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,15 +349,17 @@ 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);
-
+			return 0;
+		}
 		ksft_exit_fail_msg("%s test: Failed to send signal\n",
 				   test_name);
 	}
 
+	have_pidfd_send_signal = true;
 	close(pidfd);
 	ksft_test_result_pass(
 		"%s test: pidfd_send_signal() syscall is supported. Tests can be executed\n",
@@ -521,7 +547,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 v3 1/2] selftests: pidfd: do not use ksft_exit_skip after ksft_set_plan

[Shuah Khan]

On 7/7/20 4:19 AM, 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>
> Message-Id: <20200623001547.22255-5-pbonzini@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   tools/testing/selftests/pidfd/pidfd_test.c | 34 +++++++++++++++++++---
>   1 file changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c
> index 7aff2d3b42c0..f65ad4e32353 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;

You don't need to initialize this to false. Rest looks good.

> +
>   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,15 +349,17 @@ 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);
> -
> +			return 0;
> +		}
>   		ksft_exit_fail_msg("%s test: Failed to send signal\n",
>   				   test_name);
>   	}
>   
> +	have_pidfd_send_signal = true;
>   	close(pidfd);
>   	ksft_test_result_pass(
>   		"%s test: pidfd_send_signal() syscall is supported. Tests can be executed\n",
> @@ -521,7 +547,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);
> 

thanks,
-- Shuah

[PATCH v3 2/2] selftests: pidfd: skip test if unshare fails with EPERM

[Paolo Bonzini]

Similar to how ENOSYS causes a skip if pidfd_send_signal is not present,
we can do the same for unshare if it fails with EPERM.  This way, running
the test without privileges causes four tests to skip but no early bail out.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tools/testing/selftests/pidfd/pidfd_test.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c
index f65ad4e32353..dcc86e8f7a9f 100644
--- a/tools/testing/selftests/pidfd/pidfd_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_test.c
@@ -162,15 +162,26 @@ static int test_pidfd_send_signal_recycled_pid_fail(void)
 	}
 
 	ret = unshare(CLONE_NEWPID);
-	if (ret < 0)
+	if (ret < 0) {
+		if (errno == EPERM) {
+			ksft_test_result_skip("%s test: Unsharing pid namespace not permitted\n",
+					      test_name);
+			return 0;
+		}
 		ksft_exit_fail_msg("%s test: Failed to unshare pid namespace\n",
 				   test_name);
+	}
 
 	ret = unshare(CLONE_NEWNS);
-	if (ret < 0)
-		ksft_exit_fail_msg(
-			"%s test: Failed to unshare mount namespace\n",
-			test_name);
+	if (ret < 0) {
+		if (errno == EPERM) {
+			ksft_test_result_skip("%s test: Unsharing mount namespace not permitted\n",
+					      test_name);
+			return 0;
+		}
+		ksft_exit_fail_msg("%s test: Failed to unshare mount namespace\n",
+				   test_name);
+	}
 
 	ret = mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, 0);
 	if (ret < 0)
-- 
2.26.2

Re: [PATCH v3 0/2] selftests: pidfd: prefer ksft_test_result_skip to ksft_exit_*

[Christian Brauner]

On Tue, Jul 07, 2020 at 06:19:34AM -0400, Paolo Bonzini wrote:
> Calling ksft_exit_* results in executing fewer tests than planned, which
> is wrong for ksft_exit_skip or suboptimal (because it results in a bail
> out) for ksft_exit_fail_msg.
> 
> Using ksft_test_result_skip instead skips only one test and lets the
> test plan proceed as promised by ksft_set_plan.
> 
> Paolo

Thanks for fixing this, Paolo!
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

Shuah, want me to take it or do you want to take it?

Re: [PATCH v3 0/2] selftests: pidfd: prefer ksft_test_result_skip to ksft_exit_*

[Shuah Khan]

On 7/7/20 7:52 AM, Christian Brauner wrote:
> On Tue, Jul 07, 2020 at 06:19:34AM -0400, Paolo Bonzini wrote:
>> Calling ksft_exit_* results in executing fewer tests than planned, which
>> is wrong for ksft_exit_skip or suboptimal (because it results in a bail
>> out) for ksft_exit_fail_msg.
>>
>> Using ksft_test_result_skip instead skips only one test and lets the
>> test plan proceed as promised by ksft_set_plan.
>>
>> Paolo
> 
> Thanks for fixing this, Paolo!
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> 
> Shuah, want me to take it or do you want to take it?
> 

I will apply it to my tree with Paolo's other patches in this series.

thanks,
-- Shuah