| 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 |
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 >
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 >> >
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 --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;
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(-)