[PATCH 1/3] selftests/exec: Add the overall result line accourding to TAP

[PATCH 1/3] selftests/exec: Add the overall result line accourding to TAP

[Muhammad Usama Anjum]

The following line is missing from the test's execution. Add it to make
it fully TAP conformant:
  # Totals: pass:27 fail:0 xfail:0 xpass:0 skip:0 error:0

Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
---
 tools/testing/selftests/exec/binfmt_script.py | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/exec/binfmt_script.py b/tools/testing/selftests/exec/binfmt_script.py
index 05f94a741c7aa..2c575a2c0eab4 100755
--- a/tools/testing/selftests/exec/binfmt_script.py
+++ b/tools/testing/selftests/exec/binfmt_script.py
@@ -16,6 +16,8 @@ SIZE=256
 NAME_MAX=int(subprocess.check_output(["getconf", "NAME_MAX", "."]))
 
 test_num=0
+pass_num=0
+fail_num=0
 
 code='''#!/usr/bin/perl
 print "Executed interpreter! Args:\n";
@@ -42,7 +44,7 @@ foreach my $a (@ARGV) {
 # ...
 def test(name, size, good=True, leading="", root="./", target="/perl",
                      fill="A", arg="", newline="\n", hashbang="#!"):
-    global test_num, tests, NAME_MAX
+    global test_num, pass_num, fail_num, tests, NAME_MAX
     test_num += 1
     if test_num > tests:
         raise ValueError("more binfmt_script tests than expected! (want %d, expected %d)"
@@ -80,16 +82,20 @@ def test(name, size, good=True, leading="", root="./", target="/perl",
         if good:
             print("ok %d - binfmt_script %s (successful good exec)"
                   % (test_num, name))
+            pass_num += 1
         else:
             print("not ok %d - binfmt_script %s succeeded when it should have failed"
                   % (test_num, name))
+            fail_num = 1
     else:
         if good:
             print("not ok %d - binfmt_script %s failed when it should have succeeded (rc:%d)"
                   % (test_num, name, proc.returncode))
+            fail_num = 1
         else:
             print("ok %d - binfmt_script %s (correctly failed bad exec)"
                   % (test_num, name))
+            pass_num += 1
 
     # Clean up crazy binaries
     os.unlink(script)
@@ -166,6 +172,8 @@ test(name="two-under-trunc-arg", size=int(SIZE/2), arg=" ")
 test(name="two-under-leading",   size=int(SIZE/2), leading=" ")
 test(name="two-under-lead-trunc-arg", size=int(SIZE/2), leading=" ", arg=" ")
 
+print("# Totals: pass:%d fail:%d xfail:0 xpass:0 skip:0 error:0" % (pass_num, fail_num))
+
 if test_num != tests:
     raise ValueError("fewer binfmt_script tests than expected! (ran %d, expected %d"
                      % (test_num, tests))
-- 
2.39.2

[PATCH 2/3] selftest/exec: conform test to TAP format output

[Muhammad Usama Anjum]

Conform the layout, informational and status messages to TAP. No
functional change is intended other than the layout of output messages.

Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
---
 tools/testing/selftests/exec/load_address.c | 34 +++++++++------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/tools/testing/selftests/exec/load_address.c b/tools/testing/selftests/exec/load_address.c
index d487c2f6a6150..17e3207d34ae7 100644
--- a/tools/testing/selftests/exec/load_address.c
+++ b/tools/testing/selftests/exec/load_address.c
@@ -5,6 +5,7 @@
 #include <link.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include "../kselftest.h"
 
 struct Statistics {
 	unsigned long long load_address;
@@ -41,28 +42,23 @@ int main(int argc, char **argv)
 	unsigned long long misalign;
 	int ret;
 
+	ksft_print_header();
+	ksft_set_plan(1);
+
 	ret = dl_iterate_phdr(ExtractStatistics, &extracted);
-	if (ret != 1) {
-		fprintf(stderr, "FAILED\n");
-		return 1;
-	}
+	if (ret != 1)
+		ksft_exit_fail_msg("FAILED: dl_iterate_phdr\n");
 
-	if (extracted.alignment == 0) {
-		fprintf(stderr, "No alignment found\n");
-		return 1;
-	} else if (extracted.alignment & (extracted.alignment - 1)) {
-		fprintf(stderr, "Alignment is not a power of 2\n");
-		return 1;
-	}
+	if (extracted.alignment == 0)
+		ksft_exit_fail_msg("FAILED: No alignment found\n");
+	else if (extracted.alignment & (extracted.alignment - 1))
+		ksft_exit_fail_msg("FAILED: Alignment is not a power of 2\n");
 
 	misalign = extracted.load_address & (extracted.alignment - 1);
-	if (misalign) {
-		printf("alignment = %llu, load_address = %llu\n",
-			extracted.alignment, extracted.load_address);
-		fprintf(stderr, "FAILED\n");
-		return 1;
-	}
+	if (misalign)
+		ksft_exit_fail_msg("FAILED: alignment = %llu, load_address = %llu\n",
+				   extracted.alignment, extracted.load_address);
 
-	fprintf(stderr, "PASS\n");
-	return 0;
+	ksft_test_result_pass("Completed\n");
+	ksft_finished();
 }
-- 
2.39.2

[PATCH 3/3] selftests/exec: conform test to TAP format output

[Muhammad Usama Anjum]

Conform the layout, informational and status messages to TAP. No
functional change is intended other than the layout of output messages.
While at it, do minor cleanups like move the declarations of the variables
on top of the function.

Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
---
 .../testing/selftests/exec/recursion-depth.c  | 53 +++++++++----------
 1 file changed, 26 insertions(+), 27 deletions(-)

diff --git a/tools/testing/selftests/exec/recursion-depth.c b/tools/testing/selftests/exec/recursion-depth.c
index 2dbd5bc45b3ed..b2f37d86a5f62 100644
--- a/tools/testing/selftests/exec/recursion-depth.c
+++ b/tools/testing/selftests/exec/recursion-depth.c
@@ -23,45 +23,44 @@
 #include <fcntl.h>
 #include <sys/mount.h>
 #include <unistd.h>
+#include "../kselftest.h"
 
 int main(void)
 {
+	int fd, rv;
+
+	ksft_print_header();
+	ksft_set_plan(1);
+
 	if (unshare(CLONE_NEWNS) == -1) {
 		if (errno == ENOSYS || errno == EPERM) {
-			fprintf(stderr, "error: unshare, errno %d\n", errno);
-			return 4;
+			ksft_test_result_skip("error: unshare, errno %d\n", errno);
+			ksft_finished();
 		}
-		fprintf(stderr, "error: unshare, errno %d\n", errno);
-		return 1;
-	}
-	if (mount(NULL, "/", NULL, MS_PRIVATE|MS_REC, NULL) == -1) {
-		fprintf(stderr, "error: mount '/', errno %d\n", errno);
-		return 1;
+		ksft_exit_fail_msg("error: unshare, errno %d\n", errno);
 	}
+
+	if (mount(NULL, "/", NULL, MS_PRIVATE | MS_REC, NULL) == -1)
+		ksft_exit_fail_msg("error: mount '/', errno %d\n", errno);
+
 	/* Require "exec" filesystem. */
-	if (mount(NULL, "/tmp", "ramfs", 0, NULL) == -1) {
-		fprintf(stderr, "error: mount ramfs, errno %d\n", errno);
-		return 1;
-	}
+	if (mount(NULL, "/tmp", "ramfs", 0, NULL) == -1)
+		ksft_exit_fail_msg("error: mount ramfs, errno %d\n", errno);
 
 #define FILENAME "/tmp/1"
 
-	int fd = creat(FILENAME, 0700);
-	if (fd == -1) {
-		fprintf(stderr, "error: creat, errno %d\n", errno);
-		return 1;
-	}
+	fd = creat(FILENAME, 0700);
+	if (fd == -1)
+		ksft_exit_fail_msg("error: creat, errno %d\n", errno);
+
 #define S "#!" FILENAME "\n"
-	if (write(fd, S, strlen(S)) != strlen(S)) {
-		fprintf(stderr, "error: write, errno %d\n", errno);
-		return 1;
-	}
+	if (write(fd, S, strlen(S)) != strlen(S))
+		ksft_exit_fail_msg("error: write, errno %d\n", errno);
+
 	close(fd);
 
-	int rv = execve(FILENAME, NULL, NULL);
-	if (rv == -1 && errno == ELOOP) {
-		return 0;
-	}
-	fprintf(stderr, "error: execve, rv %d, errno %d\n", rv, errno);
-	return 1;
+	rv = execve(FILENAME, NULL, NULL);
+	ksft_test_result(rv == -1 && errno == ELOOP,
+			 "execve failed as expected (ret %d, errno %d)\n", rv, errno);
+	ksft_finished();
 }
-- 
2.39.2

Re: [PATCH 1/3] selftests/exec: Add the overall result line accourding to TAP

[Kees Cook]

On Mon, Mar 04, 2024 at 08:59:23PM +0500, Muhammad Usama Anjum wrote:
> The following line is missing from the test's execution. Add it to make
> it fully TAP conformant:
>   # Totals: pass:27 fail:0 xfail:0 xpass:0 skip:0 error:0
> 
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>

Thanks, looks good.

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH 2/3] selftest/exec: conform test to TAP format output

[Kees Cook]

On Mon, Mar 04, 2024 at 08:59:24PM +0500, Muhammad Usama Anjum wrote:
> Conform the layout, informational and status messages to TAP. No
> functional change is intended other than the layout of output messages.
> 
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> ---
>  tools/testing/selftests/exec/load_address.c | 34 +++++++++------------
>  1 file changed, 15 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/testing/selftests/exec/load_address.c b/tools/testing/selftests/exec/load_address.c
> index d487c2f6a6150..17e3207d34ae7 100644
> --- a/tools/testing/selftests/exec/load_address.c
> +++ b/tools/testing/selftests/exec/load_address.c
> @@ -5,6 +5,7 @@
>  #include <link.h>
>  #include <stdio.h>
>  #include <stdlib.h>
> +#include "../kselftest.h"
>  
>  struct Statistics {
>  	unsigned long long load_address;
> @@ -41,28 +42,23 @@ int main(int argc, char **argv)
>  	unsigned long long misalign;
>  	int ret;
>  
> +	ksft_print_header();
> +	ksft_set_plan(1);
> +
>  	ret = dl_iterate_phdr(ExtractStatistics, &extracted);
> -	if (ret != 1) {
> -		fprintf(stderr, "FAILED\n");
> -		return 1;
> -	}
> +	if (ret != 1)
> +		ksft_exit_fail_msg("FAILED: dl_iterate_phdr\n");

I'm for this series, but I do note a weird glitch in the ksft API.
ksft_exit_fail_msg does:

        va_start(args, msg);
        printf("Bail out! ");
        errno = saved_errno;
        vprintf(msg, args);
        va_end(args);

"Bail out!" is not very descriptive. I think I'd rather this should be:

	"FAILED: "

and then that added prefix doesn't need to be added everywhere in this
patch, nor the "error: " prefix in the next patch.

-- 
Kees Cook

Re: [PATCH 2/3] selftest/exec: conform test to TAP format output

[Muhammad Usama Anjum]

On 3/5/24 1:58 AM, Kees Cook wrote:
> On Mon, Mar 04, 2024 at 08:59:24PM +0500, Muhammad Usama Anjum wrote:
>> Conform the layout, informational and status messages to TAP. No
>> functional change is intended other than the layout of output messages.
>>
>> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
>> ---
>>  tools/testing/selftests/exec/load_address.c | 34 +++++++++------------
>>  1 file changed, 15 insertions(+), 19 deletions(-)
>>
>> diff --git a/tools/testing/selftests/exec/load_address.c b/tools/testing/selftests/exec/load_address.c
>> index d487c2f6a6150..17e3207d34ae7 100644
>> --- a/tools/testing/selftests/exec/load_address.c
>> +++ b/tools/testing/selftests/exec/load_address.c
>> @@ -5,6 +5,7 @@
>>  #include <link.h>
>>  #include <stdio.h>
>>  #include <stdlib.h>
>> +#include "../kselftest.h"
>>  
>>  struct Statistics {
>>  	unsigned long long load_address;
>> @@ -41,28 +42,23 @@ int main(int argc, char **argv)
>>  	unsigned long long misalign;
>>  	int ret;
>>  
>> +	ksft_print_header();
>> +	ksft_set_plan(1);
>> +
>>  	ret = dl_iterate_phdr(ExtractStatistics, &extracted);
>> -	if (ret != 1) {
>> -		fprintf(stderr, "FAILED\n");
>> -		return 1;
>> -	}
>> +	if (ret != 1)
>> +		ksft_exit_fail_msg("FAILED: dl_iterate_phdr\n");
> 
> I'm for this series, but I do note a weird glitch in the ksft API.
> ksft_exit_fail_msg does:
> 
>         va_start(args, msg);
>         printf("Bail out! ");
>         errno = saved_errno;
>         vprintf(msg, args);
>         va_end(args);
> 
> "Bail out!" is not very descriptive. I think I'd rather this should be:
> 
> 	"FAILED: "
> 
> and then that added prefix doesn't need to be added everywhere in this
> patch, nor the "error: " prefix in the next patch.
If we want to make this change, FAILED should be removed from all the
tests. We should do it in separate patch. I've taken note and will do it
separate from this series.

-- 
BR,
Muhammad Usama Anjum

Re: [PATCH 1/3] selftests/exec: Add the overall result line accourding to TAP

[Muhammad Usama Anjum]

Soft reminder!

On 3/4/24 8:59 PM, Muhammad Usama Anjum wrote:
> The following line is missing from the test's execution. Add it to make
> it fully TAP conformant:
>   # Totals: pass:27 fail:0 xfail:0 xpass:0 skip:0 error:0
> 
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> ---
>  tools/testing/selftests/exec/binfmt_script.py | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/exec/binfmt_script.py b/tools/testing/selftests/exec/binfmt_script.py
> index 05f94a741c7aa..2c575a2c0eab4 100755
> --- a/tools/testing/selftests/exec/binfmt_script.py
> +++ b/tools/testing/selftests/exec/binfmt_script.py
> @@ -16,6 +16,8 @@ SIZE=256
>  NAME_MAX=int(subprocess.check_output(["getconf", "NAME_MAX", "."]))
>  
>  test_num=0
> +pass_num=0
> +fail_num=0
>  
>  code='''#!/usr/bin/perl
>  print "Executed interpreter! Args:\n";
> @@ -42,7 +44,7 @@ foreach my $a (@ARGV) {
>  # ...
>  def test(name, size, good=True, leading="", root="./", target="/perl",
>                       fill="A", arg="", newline="\n", hashbang="#!"):
> -    global test_num, tests, NAME_MAX
> +    global test_num, pass_num, fail_num, tests, NAME_MAX
>      test_num += 1
>      if test_num > tests:
>          raise ValueError("more binfmt_script tests than expected! (want %d, expected %d)"
> @@ -80,16 +82,20 @@ def test(name, size, good=True, leading="", root="./", target="/perl",
>          if good:
>              print("ok %d - binfmt_script %s (successful good exec)"
>                    % (test_num, name))
> +            pass_num += 1
>          else:
>              print("not ok %d - binfmt_script %s succeeded when it should have failed"
>                    % (test_num, name))
> +            fail_num = 1
>      else:
>          if good:
>              print("not ok %d - binfmt_script %s failed when it should have succeeded (rc:%d)"
>                    % (test_num, name, proc.returncode))
> +            fail_num = 1
>          else:
>              print("ok %d - binfmt_script %s (correctly failed bad exec)"
>                    % (test_num, name))
> +            pass_num += 1
>  
>      # Clean up crazy binaries
>      os.unlink(script)
> @@ -166,6 +172,8 @@ test(name="two-under-trunc-arg", size=int(SIZE/2), arg=" ")
>  test(name="two-under-leading",   size=int(SIZE/2), leading=" ")
>  test(name="two-under-lead-trunc-arg", size=int(SIZE/2), leading=" ", arg=" ")
>  
> +print("# Totals: pass:%d fail:%d xfail:0 xpass:0 skip:0 error:0" % (pass_num, fail_num))
> +
>  if test_num != tests:
>      raise ValueError("fewer binfmt_script tests than expected! (ran %d, expected %d"
>                       % (test_num, tests))

-- 
BR,
Muhammad Usama Anjum

Re: [PATCH 1/3] selftests/exec: Add the overall result line accourding to TAP

[Kees Cook]

On Mon, Mar 11, 2024 at 10:10:21PM +0500, Muhammad Usama Anjum wrote:
> Soft reminder!

Ah yes! Thanks for the reminder on these. I will get them into -next
shortly.

-- 
Kees Cook