diff mbox series

[2/2] selftests: kvm: replace exit() with ksft_exit_fail_msg()

Message ID 20240612104500.425012-2-usama.anjum@collabora.com (mailing list archive)
State New, archived
Headers show
Series [1/2] selftests: kvm: remove print_skip() | expand

Commit Message

Muhammad Usama Anjum June 12, 2024, 10:44 a.m. UTC
The KSFT_FAIL, exit code must be used instead of exit(254). The 254 code
here seems like anciant relic. Its even better if we use
ksft_exit_fail_msg() which will print out "Bail out" meaning the test
exited without completing. This string is TAP protocol specific.

Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
---
 tools/testing/selftests/kvm/lib/assert.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Sean Christopherson June 12, 2024, 7:01 p.m. UTC | #1
On Wed, Jun 12, 2024, Muhammad Usama Anjum wrote:
> The KSFT_FAIL, exit code must be used instead of exit(254).

This needs more justification.  KVM selftests have worked just fine for 6+ years
using exit(254), so stating they "must" use KSFT_FAIL is obviously not true.

I'm not personally opposed to switching to KSFT_FAIL, but it is a potentially
breaking change.  E.g. some of Google's internal test infrastructure explicitly
relies on the exit code being 254.  I don't _think_ that infrastructure interacts
with KVM selftests, nor do I think that forcing upstream KVM selftests to sacrifice
TAP compliance just to play nice with someone's crusty test infrastructure is a
good tradeoff, but this and all of the TAP compliance work needs to be done with
more thought and care.

> The 254 code here seems like anciant relic.

As above, AFAICT it comes from Google's internal test infrastructure (KVM selftests
came from Google).

> Its even better if we use ksft_exit_fail_msg() which will print out "Bail
> out" meaning the test exited without completing. This string is TAP protocol
> specific.

This is debatable and not obviously correct.  The documentation says:

  Bail out!
  As an emergency measure a test script can decide that further tests are
  useless (e.g. missing dependencies) and testing should stop immediately. In
  that case the test script prints the magic words

which suggests that a test should only emit "Bail out!" if it wants to stop
entirely.  We definitely don't want KVM selftests to bail out if a TEST_ASSERT()
fails in one testcase.

> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> ---
>  tools/testing/selftests/kvm/lib/assert.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/kvm/lib/assert.c b/tools/testing/selftests/kvm/lib/assert.c
> index 33651f5b3a7fd..db648a7ac429b 100644
> --- a/tools/testing/selftests/kvm/lib/assert.c
> +++ b/tools/testing/selftests/kvm/lib/assert.c
> @@ -87,7 +87,7 @@ test_assert(bool exp, const char *exp_str,
>  
>  		if (errno == EACCES)
>  			ksft_exit_skip("Access denied - Exiting\n");
> -		exit(254);
> +		ksft_exit_fail_msg("\n");
>  	}
>  
>  	return;
> -- 
> 2.39.2
>
Muhammad Usama Anjum June 13, 2024, 9:40 a.m. UTC | #2
Hi Sean,

Thank you for replying in detail. I wasn't aware of true origin of these tests.

On 6/13/24 12:01 AM, Sean Christopherson wrote:
> On Wed, Jun 12, 2024, Muhammad Usama Anjum wrote:
>> The KSFT_FAIL, exit code must be used instead of exit(254).
> 
> This needs more justification.  KVM selftests have worked just fine for 6+ years
> using exit(254), so stating they "must" use KSFT_FAIL is obviously not true.
The selftests scripts read the exit code and mark the test status
pass/fail. Maybe selftests run_tests target isn't being used or this code
path wasn't being triggered.

> 
> I'm not personally opposed to switching to KSFT_FAIL, but it is a potentially
> breaking change.  E.g. some of Google's internal test infrastructure explicitly
> relies on the exit code being 254.  I don't _think_ that infrastructure interacts
> with KVM selftests, nor do I think that forcing upstream KVM selftests to sacrifice
> TAP compliance just to play nice with someone's crusty test infrastructure is a
> good tradeoff, but this and all of the TAP compliance work needs to be done with
> more thought and care.
You have given your perspective from KVM selftest suite perspective. I've
been thinking from kselftests subsystem perspective that how TAP compliance
and exit codes help the entire subsystem. It is understandable from KVM
suite's perspective as not all the suites are compliant and work the same.

> 
>> The 254 code here seems like anciant relic.
> 
> As above, AFAICT it comes from Google's internal test infrastructure (KVM selftests
> came from Google).
> 
>> Its even better if we use ksft_exit_fail_msg() which will print out "Bail
>> out" meaning the test exited without completing. This string is TAP protocol
>> specific.
> 
> This is debatable and not obviously correct.  The documentation says:
> 
>   Bail out!
>   As an emergency measure a test script can decide that further tests are
>   useless (e.g. missing dependencies) and testing should stop immediately. In
>   that case the test script prints the magic words
> 
> which suggests that a test should only emit "Bail out!" if it wants to stop
> entirely.  We definitely don't want KVM selftests to bail out if a TEST_ASSERT()
> fails in one testcase.
But KVM tests are bailing out if assert fails, exit(254) is being called
which stops the further execution of the test cases. It is same as
ksft_exit_fail_msg() behavior.

> 
>> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
>> ---
>>  tools/testing/selftests/kvm/lib/assert.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/kvm/lib/assert.c b/tools/testing/selftests/kvm/lib/assert.c
>> index 33651f5b3a7fd..db648a7ac429b 100644
>> --- a/tools/testing/selftests/kvm/lib/assert.c
>> +++ b/tools/testing/selftests/kvm/lib/assert.c
>> @@ -87,7 +87,7 @@ test_assert(bool exp, const char *exp_str,
>>  
>>  		if (errno == EACCES)
>>  			ksft_exit_skip("Access denied - Exiting\n");
>> -		exit(254);
>> +		ksft_exit_fail_msg("\n");
>>  	}
>>  
>>  	return;
>> -- 
>> 2.39.2
>>
>
Sean Christopherson June 13, 2024, 6:57 p.m. UTC | #3
On Thu, Jun 13, 2024, Muhammad Usama Anjum wrote:
> > As above, AFAICT it comes from Google's internal test infrastructure (KVM selftests
> > came from Google).
> > 
> >> Its even better if we use ksft_exit_fail_msg() which will print out "Bail
> >> out" meaning the test exited without completing. This string is TAP protocol
> >> specific.
> > 
> > This is debatable and not obviously correct.  The documentation says:
> > 
> >   Bail out!
> >   As an emergency measure a test script can decide that further tests are
> >   useless (e.g. missing dependencies) and testing should stop immediately. In
> >   that case the test script prints the magic words
> > 
> > which suggests that a test should only emit "Bail out!" if it wants to stop
> > entirely.  We definitely don't want KVM selftests to bail out if a TEST_ASSERT()
> > fails in one testcase.
> But KVM tests are bailing out if assert fails, exit(254) is being called
> which stops the further execution of the test cases.

Not if the TEST_ASSERT() fires from within a test fixture, in which case the
magic in tools/testing/selftests/kselftest_harness.h captures the failure but
continues on with the next test.
diff mbox series

Patch

diff --git a/tools/testing/selftests/kvm/lib/assert.c b/tools/testing/selftests/kvm/lib/assert.c
index 33651f5b3a7fd..db648a7ac429b 100644
--- a/tools/testing/selftests/kvm/lib/assert.c
+++ b/tools/testing/selftests/kvm/lib/assert.c
@@ -87,7 +87,7 @@  test_assert(bool exp, const char *exp_str,
 
 		if (errno == EACCES)
 			ksft_exit_skip("Access denied - Exiting\n");
-		exit(254);
+		ksft_exit_fail_msg("\n");
 	}
 
 	return;