[PATCH 1/2] selftests: x86: vdso_restorer: remove manual counting of pass/fail tests

[PATCH 1/2] selftests: x86: vdso_restorer: remove manual counting of pass/fail tests

[Muhammad Usama Anjum]

Use kselftest wrapper to mark tests pass/fail instead of manually
counting. This is needed to return correct exit status. This also
improves readability and mainability.

Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
---
 tools/testing/selftests/x86/vdso_restorer.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/tools/testing/selftests/x86/vdso_restorer.c b/tools/testing/selftests/x86/vdso_restorer.c
index fe99f24341554..8e173d71291f6 100644
--- a/tools/testing/selftests/x86/vdso_restorer.c
+++ b/tools/testing/selftests/x86/vdso_restorer.c
@@ -21,6 +21,7 @@
 #include <unistd.h>
 #include <syscall.h>
 #include <sys/syscall.h>
+#include "../kselftest.h"
 
 /* Open-code this -- the headers are too messy to easily use them. */
 struct real_sigaction {
@@ -44,9 +45,10 @@ static void handler_without_siginfo(int sig)
 
 int main()
 {
-	int nerrs = 0;
 	struct real_sigaction sa;
 
+	ksft_print_header();
+
 	void *vdso = dlopen("linux-vdso.so.1",
 			    RTLD_LAZY | RTLD_LOCAL | RTLD_NOLOAD);
 	if (!vdso)
@@ -57,6 +59,8 @@ int main()
 		return 0;
 	}
 
+	ksft_set_plan(2);
+
 	memset(&sa, 0, sizeof(sa));
 	sa.handler = handler_with_siginfo;
 	sa.flags = SA_SIGINFO;
@@ -69,12 +73,7 @@ int main()
 
 	raise(SIGUSR1);
 
-	if (handler_called) {
-		printf("[OK]\tSA_SIGINFO handler returned successfully\n");
-	} else {
-		printf("[FAIL]\tSA_SIGINFO handler was not called\n");
-		nerrs++;
-	}
+	ksft_test_result(handler_called, "SA_SIGINFO handler returned\n");
 
 	printf("[RUN]\tRaise a signal, !SA_SIGINFO, sa.restorer == NULL\n");
 
@@ -86,10 +85,5 @@ int main()
 
 	raise(SIGUSR1);
 
-	if (handler_called) {
-		printf("[OK]\t!SA_SIGINFO handler returned successfully\n");
-	} else {
-		printf("[FAIL]\t!SA_SIGINFO handler was not called\n");
-		nerrs++;
-	}
+	ksft_test_result(handler_called, "SA_SIGINFO handler returned\n");
 }
-- 
2.39.2

[PATCH 2/2] selftests: x86: vdso_restorer: Return correct exit statuses

[Muhammad Usama Anjum]

Return correct exit status, KSFT_SKIP if the pre-conditions aren't met.
Return KSFT_FAIL if error occurs. Use ksft_finished() which will
compmare the total planned tests with passed tests to return the exit
value.

Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
---
 tools/testing/selftests/x86/vdso_restorer.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/x86/vdso_restorer.c b/tools/testing/selftests/x86/vdso_restorer.c
index 8e173d71291f6..54f33e8cda5cc 100644
--- a/tools/testing/selftests/x86/vdso_restorer.c
+++ b/tools/testing/selftests/x86/vdso_restorer.c
@@ -56,7 +56,7 @@ int main()
 			      RTLD_LAZY | RTLD_LOCAL | RTLD_NOLOAD);
 	if (!vdso) {
 		printf("[SKIP]\tFailed to find vDSO.  Tests are not expected to work.\n");
-		return 0;
+		return KSFT_SKIP;
 	}
 
 	ksft_set_plan(2);
@@ -69,7 +69,7 @@ int main()
 	printf("[RUN]\tRaise a signal, SA_SIGINFO, sa.restorer == NULL\n");
 
 	if (syscall(SYS_rt_sigaction, SIGUSR1, &sa, NULL, 8) != 0)
-		err(1, "raw rt_sigaction syscall");
+		err(KSFT_FAIL, "raw rt_sigaction syscall");
 
 	raise(SIGUSR1);
 
@@ -80,10 +80,12 @@ int main()
 	sa.flags = 0;
 	sa.handler = handler_without_siginfo;
 	if (syscall(SYS_sigaction, SIGUSR1, &sa, 0) != 0)
-		err(1, "raw sigaction syscall");
+		err(KSFT_FAIL, "raw sigaction syscall");
 	handler_called = 0;
 
 	raise(SIGUSR1);
 
 	ksft_test_result(handler_called, "SA_SIGINFO handler returned\n");
+
+	ksft_finished();
 }
-- 
2.39.2

Re: [PATCH 2/2] selftests: x86: vdso_restorer: Return correct exit statuses

[Shuah Khan]

On 7/12/24 01:30, Muhammad Usama Anjum wrote:
> Return correct exit status, KSFT_SKIP if the pre-conditions aren't met.
> Return KSFT_FAIL if error occurs. Use ksft_finished() which will
> compmare the total planned tests with passed tests to return the exit
> value.
> 
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>

Same comment - here. Include before and after output to show
how this change improves the report.

thanks,
-- Shuah

Re: [PATCH 2/2] selftests: x86: vdso_restorer: Return correct exit statuses

[Muhammad Usama Anjum]

On 7/17/24 3:01 AM, Shuah Khan wrote:
> On 7/12/24 01:30, Muhammad Usama Anjum wrote:
>> Return correct exit status, KSFT_SKIP if the pre-conditions aren't met.
>> Return KSFT_FAIL if error occurs. Use ksft_finished() which will
>> compmare the total planned tests with passed tests to return the exit
>> value.
>>
>> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> 
> Same comment - here. Include before and after output to show
> how this change improves the report.
Following results have been generated in the case when both tests fail:

# selftests: x86: vdso_restorer_32
# ERROR: ld.so: object '/usr/libexec/coreutils/libstdbuf.so' from
LD_PRELOAD cannot be preloaded (wrong ELF class: ELFCLASS64): ignored.
# [RUN]	Raise a signal, SA_SIGINFO, sa.restorer == NULL
# [FAIL]	SA_SIGINFO handler was not called
# [RUN]	Raise a signal, !SA_SIGINFO, sa.restorer == NULL
# [FAIL]	!SA_SIGINFO handler was not called
not ok 21 selftests: x86: vdso_restorer_32 # exit=2

# selftests: x86: vdso_restorer_32
# ERROR: ld.so: object '/usr/libexec/coreutils/libstdbuf.so' from
LD_PRELOAD cannot be preloaded (wrong ELF class: ELFCLASS64): ignored.
# TAP version 13
# 1..2
# [RUN]	Raise a signal, SA_SIGINFO, sa.restorer == NULL
# not ok 1 SA_SIGINFO handler returned
# [RUN]	Raise a signal, !SA_SIGINFO, sa.restorer == NULL
# not ok 2 SA_SIGINFO handler returned
# # Totals: pass:0 fail:2 xfail:0 xpass:0 skip:0 error:0
not ok 21 selftests: x86: vdso_restorer_32 # exit=1

Please let me know what you think?
> 
> thanks,
> -- Shuah
> 
> 

-- 
BR,
Muhammad Usama Anjum

Re: [PATCH 1/2] selftests: x86: vdso_restorer: remove manual counting of pass/fail tests

[Shuah Khan]

On 7/12/24 01:30, Muhammad Usama Anjum wrote:
> Use kselftest wrapper to mark tests pass/fail instead of manually
> counting. This is needed to return correct exit status. This also
> improves readability and mainability.
> 
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>

As mentioned earlier, include before and after output from test run
to see the improvement clearly.

thanks,
-- Shuah

Re: [PATCH 1/2] selftests: x86: vdso_restorer: remove manual counting of pass/fail tests

[Shuah Khan]

On 7/12/24 01:30, Muhammad Usama Anjum wrote:
> Use kselftest wrapper to mark tests pass/fail instead of manually
> counting.

You care combining two changes in the patch.

This is needed to return correct exit status. This also
> improves readability and mainability.

Spelling - "mainability" - checkpatch would have helped you
catch this.

The change to return the correct error fine and but not the
change thaT ADDS DUPLICATE tap header.

> 


> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> ---
>   tools/testing/selftests/x86/vdso_restorer.c | 20 +++++++-------------
>   1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/testing/selftests/x86/vdso_restorer.c b/tools/testing/selftests/x86/vdso_restorer.c
> index fe99f24341554..8e173d71291f6 100644
> --- a/tools/testing/selftests/x86/vdso_restorer.c
> +++ b/tools/testing/selftests/x86/vdso_restorer.c
> @@ -21,6 +21,7 @@
>   #include <unistd.h>
>   #include <syscall.h>
>   #include <sys/syscall.h>
> +#include "../kselftest.h"
>   
>   /* Open-code this -- the headers are too messy to easily use them. */
>   struct real_sigaction {
> @@ -44,9 +45,10 @@ static void handler_without_siginfo(int sig)
>   
>   int main()
>   {
> -	int nerrs = 0;
>   	struct real_sigaction sa;
>   
> +	ksft_print_header();

The problem with adding this header here is when
make kselftest TARGETS=vDSO is run there will be
duplicate TAP 13 headers.


> +
>   	void *vdso = dlopen("linux-vdso.so.1",
>   			    RTLD_LAZY | RTLD_LOCAL | RTLD_NOLOAD);
>   	if (!vdso)
> @@ -57,6 +59,8 @@ int main()
>   		return 0;
>   	}
>   
> +	ksft_set_plan(2);
> +
>   	memset(&sa, 0, sizeof(sa));
>   	sa.handler = handler_with_siginfo;
>   	sa.flags = SA_SIGINFO;
> @@ -69,12 +73,7 @@ int main()
>   
>   	raise(SIGUSR1);
>   
> -	if (handler_called) {
> -		printf("[OK]\tSA_SIGINFO handler returned successfully\n");
> -	} else {
> -		printf("[FAIL]\tSA_SIGINFO handler was not called\n");
> -		nerrs++;
> -	}
> +	ksft_test_result(handler_called, "SA_SIGINFO handler returned\n");
>   
>   	printf("[RUN]\tRaise a signal, !SA_SIGINFO, sa.restorer == NULL\n");
>   
> @@ -86,10 +85,5 @@ int main()
>   
>   	raise(SIGUSR1);
>   
> -	if (handler_called) {
> -		printf("[OK]\t!SA_SIGINFO handler returned successfully\n");
> -	} else {
> -		printf("[FAIL]\t!SA_SIGINFO handler was not called\n");
> -		nerrs++;
> -	}
> +	ksft_test_result(handler_called, "SA_SIGINFO handler returned\n");
>   }

thanks,
-- Shuah

Re: [PATCH 1/2] selftests: x86: vdso_restorer: remove manual counting of pass/fail tests

[Muhammad Usama Anjum]

[-- Attachment #1: Type: text/plain, Size: 2215 bytes --]

On 7/19/24 9:40 PM, Shuah Khan wrote:
> On 7/12/24 01:30, Muhammad Usama Anjum wrote:
>> Use kselftest wrapper to mark tests pass/fail instead of manually
>> counting.
> 
> You care combining two changes in the patch.
> 
> This is needed to return correct exit status. This also
>> improves readability and mainability.
> 
> Spelling - "mainability" - checkpatch would have helped you
> catch this.
Sorry I'll fix it after following discussion. I use checkpatch with
spelling checker. I may have missed it for this patch.

> 
> The change to return the correct error fine and but not the
> change thaT ADDS DUPLICATE tap header.
> 
>>
> 
> 
>> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
>> ---
>>   tools/testing/selftests/x86/vdso_restorer.c | 20 +++++++-------------
>>   1 file changed, 7 insertions(+), 13 deletions(-)
>>
>> diff --git a/tools/testing/selftests/x86/vdso_restorer.c
>> b/tools/testing/selftests/x86/vdso_restorer.c
>> index fe99f24341554..8e173d71291f6 100644
>> --- a/tools/testing/selftests/x86/vdso_restorer.c
>> +++ b/tools/testing/selftests/x86/vdso_restorer.c
>> @@ -21,6 +21,7 @@
>>   #include <unistd.h>
>>   #include <syscall.h>
>>   #include <sys/syscall.h>
>> +#include "../kselftest.h"
>>     /* Open-code this -- the headers are too messy to easily use them. */
>>   struct real_sigaction {
>> @@ -44,9 +45,10 @@ static void handler_without_siginfo(int sig)
>>     int main()
>>   {
>> -    int nerrs = 0;
>>       struct real_sigaction sa;
>>   +    ksft_print_header();
> 
> The problem with adding this header here is when
> make kselftest TARGETS=vDSO is run there will be
> duplicate TAP 13 headers.
Usually all TAP compliant tests print TAP 13 header at the start. These
tests when run from make run_tests have duplicate TAP 13 headers. I don't
think that this is the issue. Why do you think it is wrong?

For example, I've attached the logs of vDSO test suite. TAP header is
printed at the start. Then it is printed again at the start of the test if
it is TAP compliant e.g., vdso_test_abi and vdso_test_getrandom. These
tests are already TAP compliant. Other tests in this suite aren't TAP
compliant.

-- 
BR,
Muhammad Usama Anjum

[-- Attachment #2: selftests-vDSO.log --]
[-- Type: text/x-log, Size: 6523 bytes --]

make[1]: Entering directory '/home/usama/repos/kernel/linux_mainline/tools/testing/selftests/vDSO'
make[1]: Nothing to be done for 'all'.
make[1]: Leaving directory '/home/usama/repos/kernel/linux_mainline/tools/testing/selftests/vDSO'
make[1]: Entering directory '/home/usama/repos/kernel/linux_mainline/tools/testing/selftests/vDSO'
TAP version 13
1..7
# timeout set to 45
# selftests: vDSO: vdso_test_gettimeofday
# The time is 1721578846.946762
ok 1 selftests: vDSO: vdso_test_gettimeofday
# timeout set to 45
# selftests: vDSO: vdso_test_getcpu
# Running on CPU 4 node 0
ok 2 selftests: vDSO: vdso_test_getcpu
# timeout set to 45
# selftests: vDSO: vdso_test_abi
# TAP version 13
# 1..16
# # [vDSO kselftest] VDSO_VERSION: LINUX_2.6
# # The time is 1721578846.975721
# ok 1 __vdso_gettimeofday
# # clock_id: CLOCK_REALTIME
# # The time is 1721578846.975726259
# ok 2 __vdso_clock_gettime CLOCK_REALTIME
# # The vdso resolution is 0 1
# # The syscall resolution is 0 1
# ok 3 __vdso_clock_getres CLOCK_REALTIME
# # clock_id: CLOCK_BOOTTIME
# # The time is 1077008.105214424
# ok 4 __vdso_clock_gettime CLOCK_BOOTTIME
# # The vdso resolution is 0 1
# # The syscall resolution is 0 1
# ok 5 __vdso_clock_getres CLOCK_BOOTTIME
# # clock_id: CLOCK_TAI
# # The time is 1721578883.975737992
# ok 6 __vdso_clock_gettime CLOCK_TAI
# # The vdso resolution is 0 1
# # The syscall resolution is 0 1
# ok 7 __vdso_clock_getres CLOCK_TAI
# # clock_id: CLOCK_REALTIME_COARSE
# # The time is 1721578846.972275735
# ok 8 __vdso_clock_gettime CLOCK_REALTIME_COARSE
# # The vdso resolution is 0 4000000
# # The syscall resolution is 0 4000000
# ok 9 __vdso_clock_getres CLOCK_REALTIME_COARSE
# # clock_id: CLOCK_MONOTONIC
# # The time is 380977.228151647
# ok 10 __vdso_clock_gettime CLOCK_MONOTONIC
# # The vdso resolution is 0 1
# # The syscall resolution is 0 1
# ok 11 __vdso_clock_getres CLOCK_MONOTONIC
# # clock_id: CLOCK_MONOTONIC_RAW
# # The time is 380974.714205580
# ok 12 __vdso_clock_gettime CLOCK_MONOTONIC_RAW
# # The vdso resolution is 0 1
# # The syscall resolution is 0 1
# ok 13 __vdso_clock_getres CLOCK_MONOTONIC_RAW
# # clock_id: CLOCK_MONOTONIC_COARSE
# # The time is 380977.224680450
# ok 14 __vdso_clock_gettime CLOCK_MONOTONIC_COARSE
# # The vdso resolution is 0 4000000
# # The syscall resolution is 0 4000000
# ok 15 __vdso_clock_getres CLOCK_MONOTONIC_COARSE
# # The time in hours since January 1, 1970 is 478216
# ok 16 __vdso_time
# # Totals: pass:16 fail:0 xfail:0 xpass:0 skip:0 error:0
ok 3 selftests: vDSO: vdso_test_abi
# timeout set to 45
# selftests: vDSO: vdso_test_clock_getres
# clock_id: CLOCK_REALTIME [PASS]
# clock_id: CLOCK_BOOTTIME [PASS]
# clock_id: CLOCK_TAI [PASS]
# clock_id: CLOCK_REALTIME_COARSE [PASS]
# clock_id: CLOCK_MONOTONIC [PASS]
# clock_id: CLOCK_MONOTONIC_RAW [PASS]
# clock_id: CLOCK_MONOTONIC_COARSE [PASS]
ok 4 selftests: vDSO: vdso_test_clock_getres
# timeout set to 45
# selftests: vDSO: vdso_standalone_test_x86
# The time is           1721578847.000875
ok 5 selftests: vDSO: vdso_standalone_test_x86
# timeout set to 45
# selftests: vDSO: vdso_test_correctness
# Warning: failed to find vsyscall getcpu
# [RUN]	Testing clock_gettime for clock CLOCK_REALTIME (0)...
# 	1721578847.012888832 1721578847.012895886 1721578847.012897283
# [OK]	Test Passed.
# [RUN]	Testing clock_gettime for clock CLOCK_MONOTONIC (1)...
# 	380977.265307376 380977.265308284 380977.265309751
# [OK]	Test Passed.
# [RUN]	Testing clock_gettime for clock CLOCK_PROCESS_CPUTIME_ID (2)...
# 	0.001188240 0.001189102 0.001189623
# [OK]	Test Passed.
# [RUN]	Testing clock_gettime for clock CLOCK_THREAD_CPUTIME_ID (3)...
# 	0.001192668 0.001193169 0.001193610
# [OK]	Test Passed.
# [RUN]	Testing clock_gettime for clock CLOCK_MONOTONIC_RAW (4)...
# 	380974.751371197 380974.751372594 380974.751373572
# [OK]	Test Passed.
# [RUN]	Testing clock_gettime for clock CLOCK_REALTIME_COARSE (5)...
# 	1721578847.008276074 1721578847.008276074 1721578847.008276074
# [OK]	Test Passed.
# [RUN]	Testing clock_gettime for clock CLOCK_MONOTONIC_COARSE (6)...
# 	380977.260680789 380977.260680789 380977.260680789
# [OK]	Test Passed.
# [RUN]	Testing clock_gettime for clock CLOCK_BOOTTIME (7)...
# 	1077008.142411709 1077008.142413106 1077008.142414083
# [OK]	Test Passed.
# [RUN]	Testing clock_gettime for clock CLOCK_REALTIME_ALARM (8)...
# 	1721578847.012935836 1721578847.012937303 1721578847.012938699
# [OK]	Test Passed.
# [RUN]	Testing clock_gettime for clock CLOCK_BOOTTIME_ALARM (9)...
# 	1077008.142424141 1077008.142425537 1077008.142427004
# [OK]	Test Passed.
# [RUN]	Testing clock_gettime for clock CLOCK_SGI_CYCLE (10)...
# [OK]	No such clock.
# [RUN]	Testing clock_gettime for clock CLOCK_TAI (11)...
# 	1721578884.012950642 1721578884.012951620 1721578884.012953017
# [OK]	Test Passed.
# [RUN]	Testing clock_gettime for clock invalid (-1)...
# [OK]	No such clock.
# [RUN]	Testing clock_gettime for clock invalid (-2147483648)...
# [OK]	No such clock.
# [RUN]	Testing clock_gettime for clock invalid (2147483647)...
# [OK]	No such clock.
# [SKIP]	No vDSO, so skipping clock_gettime64() tests
# [RUN]	Testing gettimeofday...
# 	1721578847.012962 1721578847.012964 1721578847.012965
# [OK]	timezones match: minuteswest=0, dsttime=0
# [RUN]	Testing getcpu...
# [OK]	CPU 0: syscall: cpu 0, node 0 vdso: cpu 0, node 0
# [OK]	CPU 1: syscall: cpu 1, node 0 vdso: cpu 1, node 0
# [OK]	CPU 2: syscall: cpu 2, node 0 vdso: cpu 2, node 0
# [OK]	CPU 3: syscall: cpu 3, node 0 vdso: cpu 3, node 0
# [OK]	CPU 4: syscall: cpu 4, node 0 vdso: cpu 4, node 0
# [OK]	CPU 5: syscall: cpu 5, node 0 vdso: cpu 5, node 0
# [OK]	CPU 6: syscall: cpu 6, node 0 vdso: cpu 6, node 0
# [OK]	CPU 7: syscall: cpu 7, node 0 vdso: cpu 7, node 0
# [OK]	CPU 8: syscall: cpu 8, node 0 vdso: cpu 8, node 0
# [OK]	CPU 9: syscall: cpu 9, node 0 vdso: cpu 9, node 0
# [OK]	CPU 10: syscall: cpu 10, node 0 vdso: cpu 10, node 0
# [OK]	CPU 11: syscall: cpu 11, node 0 vdso: cpu 11, node 0
# [OK]	CPU 12: syscall: cpu 12, node 0 vdso: cpu 12, node 0
# [OK]	CPU 13: syscall: cpu 13, node 0 vdso: cpu 13, node 0
# [OK]	CPU 14: syscall: cpu 14, node 0 vdso: cpu 14, node 0
# [OK]	CPU 15: syscall: cpu 15, node 0 vdso: cpu 15, node 0
ok 6 selftests: vDSO: vdso_test_correctness
# timeout set to 45
# selftests: vDSO: vdso_test_getrandom
# TAP version 13
# 1..1
# __vdso_getrandom is missing!
not ok 7 selftests: vDSO: vdso_test_getrandom # exit=1
make[1]: Leaving directory '/home/usama/repos/kernel/linux_mainline/tools/testing/selftests/vDSO'

Re: [PATCH 1/2] selftests: x86: vdso_restorer: remove manual counting of pass/fail tests

[Muhammad Usama Anjum]

On 7/21/24 9:24 PM, Muhammad Usama Anjum wrote:
> On 7/19/24 9:40 PM, Shuah Khan wrote:
>> On 7/12/24 01:30, Muhammad Usama Anjum wrote:
>>> Use kselftest wrapper to mark tests pass/fail instead of manually
>>> counting.
>>
>> You care combining two changes in the patch.
>>
>> This is needed to return correct exit status. This also
>>> improves readability and mainability.
>>
>> Spelling - "mainability" - checkpatch would have helped you
>> catch this.
> Sorry I'll fix it after following discussion. I use checkpatch with
> spelling checker. I may have missed it for this patch.
> 
>>
>> The change to return the correct error fine and but not the
>> change thaT ADDS DUPLICATE tap header.
>>
>>>
>>
>>
>>> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
>>> ---
>>>   tools/testing/selftests/x86/vdso_restorer.c | 20 +++++++-------------
>>>   1 file changed, 7 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/x86/vdso_restorer.c
>>> b/tools/testing/selftests/x86/vdso_restorer.c
>>> index fe99f24341554..8e173d71291f6 100644
>>> --- a/tools/testing/selftests/x86/vdso_restorer.c
>>> +++ b/tools/testing/selftests/x86/vdso_restorer.c
>>> @@ -21,6 +21,7 @@
>>>   #include <unistd.h>
>>>   #include <syscall.h>
>>>   #include <sys/syscall.h>
>>> +#include "../kselftest.h"
>>>     /* Open-code this -- the headers are too messy to easily use them. */
>>>   struct real_sigaction {
>>> @@ -44,9 +45,10 @@ static void handler_without_siginfo(int sig)
>>>     int main()
>>>   {
>>> -    int nerrs = 0;
>>>       struct real_sigaction sa;
>>>   +    ksft_print_header();
>>
>> The problem with adding this header here is when
>> make kselftest TARGETS=vDSO is run there will be
>> duplicate TAP 13 headers.
> Usually all TAP compliant tests print TAP 13 header at the start. These
> tests when run from make run_tests have duplicate TAP 13 headers. I don't
> think that this is the issue. Why do you think it is wrong?
> 
> For example, I've attached the logs of vDSO test suite. TAP header is
> printed at the start. Then it is printed again at the start of the test if
> it is TAP compliant e.g., vdso_test_abi and vdso_test_getrandom. These
> tests are already TAP compliant. Other tests in this suite aren't TAP
> compliant.
On CIs (make runtests or make kselftest) is used to run the tests. I'm not
aware of the ancient history. AFAIU following is the format of messages
(make kselftests). The TAP header mention that a new test has started. One
test may have multiple sub-tests. For example:

TAP version 13
1..4
# selftests: vDSO: test1
# TAP version 13
1..5
ok 1
ok 2
ok 3
ok 4
ok 5
# # Totals: pass:5 fail:0 xfail:0 xpass:0 skip:0 error:0
ok 1 selftests: vDSO: test1
# selftests: vDSO: test2
# TAP version 13
1..5
ok 1
ok 2
not ok 3
# # Totals: pass:2 fail:1 xfail:0 xpass:0 skip:0 error:0
not ok 2 selftests: vDSO: test2 # exit=1
# selftests: vDSO: test3
ok 1
ok 2
ok 3
not ok 3 selftests: vDSO: test3
# selftests: vDSO: test4
ok 1
not ok 3 selftests: vDSO: test4


The test1 and test2 are TAP compliant and print header and footer of the
tests mentioning total number of tests. The test3 and test4 don't print TAP
header and footer. The boundary between test3 and test4 isn't that clear,
but seems fine. Overall I would say TAP compliant tests have better boundry
when they print header/footer and total number of tests.

Do you agree with above layout's current state because we have both TAP
compliant and non-compliant tests.

-- 
BR,
Muhammad Usama Anjum

Re: [PATCH 1/2] selftests: x86: vdso_restorer: remove manual counting of pass/fail tests

[Shuah Khan]

On 7/21/24 10:37, Muhammad Usama Anjum wrote:
> On 7/21/24 9:24 PM, Muhammad Usama Anjum wrote:
>> On 7/19/24 9:40 PM, Shuah Khan wrote:
>>> On 7/12/24 01:30, Muhammad Usama Anjum wrote:
>>>> Use kselftest wrapper to mark tests pass/fail instead of manually
>>>> counting.
>>>
>>> You care combining two changes in the patch.
>>>
>>> This is needed to return correct exit status. This also
>>>> improves readability and mainability.
>>>
>>> Spelling - "mainability" - checkpatch would have helped you
>>> catch this.
>> Sorry I'll fix it after following discussion. I use checkpatch with
>> spelling checker. I may have missed it for this patch.
>>
>>>
>>> The change to return the correct error fine and but not the
>>> change thaT ADDS DUPLICATE tap header.
>>>
>>>>
>>>
>>>
>>>> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
>>>> ---
>>>>    tools/testing/selftests/x86/vdso_restorer.c | 20 +++++++-------------
>>>>    1 file changed, 7 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/x86/vdso_restorer.c
>>>> b/tools/testing/selftests/x86/vdso_restorer.c
>>>> index fe99f24341554..8e173d71291f6 100644
>>>> --- a/tools/testing/selftests/x86/vdso_restorer.c
>>>> +++ b/tools/testing/selftests/x86/vdso_restorer.c
>>>> @@ -21,6 +21,7 @@
>>>>    #include <unistd.h>
>>>>    #include <syscall.h>
>>>>    #include <sys/syscall.h>
>>>> +#include "../kselftest.h"
>>>>      /* Open-code this -- the headers are too messy to easily use them. */
>>>>    struct real_sigaction {
>>>> @@ -44,9 +45,10 @@ static void handler_without_siginfo(int sig)
>>>>      int main()
>>>>    {
>>>> -    int nerrs = 0;
>>>>        struct real_sigaction sa;
>>>>    +    ksft_print_header();
>>>
>>> The problem with adding this header here is when
>>> make kselftest TARGETS=vDSO is run there will be
>>> duplicate TAP 13 headers.
>> Usually all TAP compliant tests print TAP 13 header at the start. These
>> tests when run from make run_tests have duplicate TAP 13 headers. I don't
>> think that this is the issue. Why do you think it is wrong?
>>
>> For example, I've attached the logs of vDSO test suite. TAP header is
>> printed at the start. Then it is printed again at the start of the test if
>> it is TAP compliant e.g., vdso_test_abi and vdso_test_getrandom. These
>> tests are already TAP compliant. Other tests in this suite aren't TAP
>> compliant.
> On CIs (make runtests or make kselftest) is used to run the tests. I'm not
> aware of the ancient history. AFAIU following is the format of messages
> (make kselftests). The TAP header mention that a new test has started. One
> test may have multiple sub-tests. For example:
> 
> TAP version 13
> 1..4
> # selftests: vDSO: test1
> # TAP version 13
> 1..5
> ok 1
> ok 2
> ok 3
> ok 4
> ok 5
> # # Totals: pass:5 fail:0 xfail:0 xpass:0 skip:0 error:0
> ok 1 selftests: vDSO: test1
> # selftests: vDSO: test2
> # TAP version 13
> 1..5
> ok 1
> ok 2
> not ok 3
> # # Totals: pass:2 fail:1 xfail:0 xpass:0 skip:0 error:0
> not ok 2 selftests: vDSO: test2 # exit=1
> # selftests: vDSO: test3
> ok 1
> ok 2
> ok 3
> not ok 3 selftests: vDSO: test3
> # selftests: vDSO: test4
> ok 1
> not ok 3 selftests: vDSO: test4
> 
> 
> The test1 and test2 are TAP compliant and print header and footer of the
> tests mentioning total number of tests. The test3 and test4 don't print TAP
> header and footer. The boundary between test3 and test4 isn't that clear,
> but seems fine. Overall I would say TAP compliant tests have better boundry
> when they print header/footer and total number of tests.
> 
> Do you agree with above layout's current state because we have both TAP
> compliant and non-compliant tests.
> 

Usama,

There is a reason for not using nested TAP headers - this is what the outcome
would be with the changes you are making. The reason went with the direction
of adding TAP headers once for each test suite is to avoid nested TAP headers.

Unless that changes and we can stay backwards compatible with TAP 13, I am
not going accept TAP headers for individual patches.

You can take it as a nack from me for all such patches.

thanks,
-- Shuah