On 06/14/2017 04:00 AM, Bird, Timothy wrote: >> -----Original Message----- >> From: Greg Kroah-Hartman [mailto:gregkh@xxxxxxxxxxxxxxxxxxx] >> Sent: Monday, June 12, 2017 3:57 PM >> To: shuah@xxxxxxxxxx >> Cc: Paul Elder <paul.elder@xxxxxxxx>; linux-kselftest@xxxxxxxxxxxxxxx; Greg >> Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Bird, Timothy >> <Tim.Bird@xxxxxxxx>; Alice Ferrazzi <alice.ferrazzi@xxxxxxxxx> >> Subject: [PATCH v2 2/4] kselftest: membarrier: convert to TAP13 output >> >> From: Paul Elder <paul.elder@xxxxxxxx> >> >> Make the membarrier test output in the TAP13 format by using the >> TAP13 output functions defined in kselftest.h >> >> Signed-off-by: Paul Elder <paul.elder@xxxxxxxx> >> Signed-off-by: Alice Ferrazzi <alice.ferrazzi@xxxxxxxxx> >> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> >> --- >> v2: Just use the standard function names, no _tap suffix - Alice >> >> >> .../testing/selftests/membarrier/membarrier_test.c | 35 ++++++++++++---- >> ------ >> 1 file changed, 19 insertions(+), 16 deletions(-) >> >> diff --git a/tools/testing/selftests/membarrier/membarrier_test.c >> b/tools/testing/selftests/membarrier/membarrier_test.c >> index 535f0fef4d0b..cae8c984dfb0 100644 >> --- a/tools/testing/selftests/membarrier/membarrier_test.c >> +++ b/tools/testing/selftests/membarrier/membarrier_test.c >> @@ -21,36 +21,42 @@ static int sys_membarrier(int cmd, int flags) >> static enum test_membarrier_status test_membarrier_cmd_fail(void) >> { >> int cmd = -1, flags = 0; >> + const char *test_name = "membarrier command fail"; >> >> if (sys_membarrier(cmd, flags) != -1) { >> - printf("membarrier: Wrong command should fail but >> passed.\n"); >> + ksft_test_result_fail(test_name); > > As written, the conversion to TAP loses this explanation of the error. > > If you follow my recommendations for patch 1/4, then this becomes: > ksft_fail(test_name, "membarrier: Wrong command should fail but passed.") Looking at the TAP 13 specification, I can see your point: where the test description is invariant whereas the reason for failure should be printed separately. However, as I have pointed out in my reply for patch 1/4, I think that the "reason" output should be outputted with the yaml. I do see that the failure reason should be tied to the test, but then wouldn't the yaml closing become messy if the test wants to output more yaml after the failure reason? Thanks, Paul >> return TEST_MEMBARRIER_FAIL; >> } >> + >> + ksft_test_result_pass(test_name); > And this becomes: > ksft_pass(test_name); > >> return TEST_MEMBARRIER_PASS; >> } >> >> static enum test_membarrier_status test_membarrier_flags_fail(void) >> { >> int cmd = MEMBARRIER_CMD_QUERY, flags = 1; >> + const char *test_name = "Wrong flags should fail"; >> >> if (sys_membarrier(cmd, flags) != -1) { >> - printf("membarrier: Wrong flags should fail but passed.\n"); >> + ksft_test_result_fail(test_name); > ksft_fail(test_name, "membarrier: Wrong flags should fail but passed."); > >> return TEST_MEMBARRIER_FAIL; >> } >> + >> + ksft_test_result_pass(test_name); > ksft_pass(test_name); > >> return TEST_MEMBARRIER_PASS; >> } >> >> static enum test_membarrier_status test_membarrier_success(void) >> { >> int cmd = MEMBARRIER_CMD_SHARED, flags = 0; >> + const char *test_name = "execute MEMBARRIER_CMD_SHARED"; >> >> if (sys_membarrier(cmd, flags) != 0) { >> - printf("membarrier: Executing MEMBARRIER_CMD_SHARED >> failed. %s.\n", >> - strerror(errno)); >> + ksft_test_result_fail(test_name); > Same as above. > >> return TEST_MEMBARRIER_FAIL; >> } >> >> - printf("membarrier: MEMBARRIER_CMD_SHARED success.\n"); >> + ksft_test_result_pass(test_name); >> return TEST_MEMBARRIER_PASS; >> } >> >> @@ -74,32 +80,30 @@ static enum test_membarrier_status >> test_membarrier_query(void) >> { >> int flags = 0, ret; > > This should be the same style as other tests. In particular, this conflates > the test name with the test results (in the pass case). This should be: > const char *test_name = "sys_membarrier test" >> >> - printf("membarrier MEMBARRIER_CMD_QUERY "); >> ret = sys_membarrier(MEMBARRIER_CMD_QUERY, flags); >> if (ret < 0) { >> - printf("failed. %s.\n", strerror(errno)); >> - switch (errno) { >> - case ENOSYS: >> + if (errno == ENOSYS) { >> /* >> * It is valid to build a kernel with >> * CONFIG_MEMBARRIER=n. However, this skips the >> tests. >> */ >> - return TEST_MEMBARRIER_SKIP; >> - case EINVAL: >> - default: >> - return TEST_MEMBARRIER_FAIL; >> + ksft_test_result_skip("CONFIG_MEMBARRIER is not >> enabled\n"); > kfst_skip(test_name, "CONFIG_MEMBARRIER is not enabled") > >> + return ksft_exit_skip(); >> } >> + ksft_test_result_fail("sys_membarrier() failed\n"); > ksft_fail(test_name, "sys_membarrier failed") > >> + return TEST_MEMBARRIER_FAIL; >> } >> if (!(ret & MEMBARRIER_CMD_SHARED)) { >> - printf("command MEMBARRIER_CMD_SHARED is not >> supported.\n"); >> + ksft_test_result_fail("command >> MEMBARRIER_CMD_SHARED is not supported.\n"); > ksft_fail(test_name, "command MEMBARRIER_CMD_SHARED is t supported."); > >> return TEST_MEMBARRIER_FAIL; >> } >> - printf("syscall available.\n"); >> + ksft_test_result_pass("sys_membarrier available"); > ksft_pass(test_name); >> return TEST_MEMBARRIER_PASS; >> } >> >> int main(int argc, char **argv) >> { >> + ksft_print_header(); >> switch (test_membarrier_query()) { >> case TEST_MEMBARRIER_FAIL: >> return ksft_exit_fail(); >> @@ -113,6 +117,5 @@ int main(int argc, char **argv) >> return ksft_exit_skip(); >> } >> >> - printf("membarrier: tests done!\n"); >> return ksft_exit_pass(); >> } >> -- >> 2.13.1 >> > -- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
