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

[Muhammad Usama Anjum <usama.anjum@collabora.com>]

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

-- 
BR,
Muhammad Usama Anjum