On 07/27/2017 01:58 AM, Shuah Khan wrote: > Hi Paul, > > Please see comments below: > > On 07/25/2017 11:12 AM, Paul Elder wrote: >> Convert exec test output to TAP13 format, using the ksft framework. >> >> Signed-off-by: Paul Elder <paul.elder@xxxxxxxx> >> --- >> >> Depends on Shuah Khan's patch: "[PATCH 2/3] selftests: kselftest framework: add API to return pass/fail/* counts" (https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.spinics.net%2Flists%2Flinux-kselftest%2Fmsg01964.html&data=01%7C01%7Cpaul.elder%40pitt.edu%7C0709e3fc974f4ccf6fcb08d4d4478c78%7C9ef9f489e0a04eeb87cc3a526112fd0d%7C1&sdata=G3AQ%2FJRvrD0AXNupCNBbi4Ne9%2FAbYBZxZ3GryWev%2FN4%3D&reserved=0) >> >> tools/testing/selftests/exec/execveat.c | 148 +++++++++++++++++--------------- >> 1 file changed, 81 insertions(+), 67 deletions(-) >> >> diff --git a/tools/testing/selftests/exec/execveat.c b/tools/testing/selftests/exec/execveat.c >> index 8d5d1d2ee7c1..dc94c0661459 100644 >> --- a/tools/testing/selftests/exec/execveat.c >> +++ b/tools/testing/selftests/exec/execveat.c >> @@ -20,6 +20,8 @@ >> #include <string.h> >> #include <unistd.h> >> >> +#include "../kselftest.h" >> + >> static char longpath[2 * PATH_MAX] = ""; >> static char *envp[] = { "IN_TEST=yes", NULL, NULL }; >> static char *argv[] = { "execveat", "99", NULL }; >> @@ -41,23 +43,26 @@ static int _check_execveat_fail(int fd, const char *path, int flags, >> int expected_errno, const char *errno_str) >> { >> int rc; >> + char msg[512]; > > You need a line between declarations and code. Okay. > >> + snprintf(msg, 512, "Check failure of execveat(%d, '%s', %d) with %s...\n", >> + fd, path?:"(null)", flags, errno_str); > > Is there is a need to use snprinf() here? ksft_test_result_fail() does suppoort > var args now. Also, please use sizeof(msg) instead of 512. As you have noted later, it is to populate the static msg to be printed on both failure and success. >> >> errno = 0; >> - printf("Check failure of execveat(%d, '%s', %d) with %s... ", >> - fd, path?:"(null)", flags, errno_str); >> rc = execveat_(fd, path, argv, envp, flags); >> >> if (rc > 0) { >> - printf("[FAIL] (unexpected success from execveat(2))\n"); >> + ksft_test_result_fail(msg); >> + ksft_print_msg("unexpected success from execveat(2)\n"); > > I think one single message to indicate the test result is desirable as > opposed to two messages. The ksft_test_result_fail(msg) prints the test name and increments the fail counter, then the ksft_print_msg() gives information regarding the failure. > >> return 1; >> } >> if (errno != expected_errno) { >> - printf("[FAIL] (expected errno %d (%s) not %d (%s)\n", >> + ksft_test_result_fail(msg); >> + ksft_print_msg("expected errno %d (%s) not %d (%s)\n", >> expected_errno, strerror(expected_errno), >> errno, strerror(errno)); > > Same as above. > >> return 1; >> } >> - printf("[OK]\n"); >> + ksft_test_result_pass(msg); >> return 0; >> } >> >> @@ -68,43 +73,48 @@ static int check_execveat_invoked_rc(int fd, const char *path, int flags, >> int rc; >> pid_t child; >> int pathlen = path ? strlen(path) : 0; >> + char msg[512]; >> >> if (pathlen > 40) >> - printf("Check success of execveat(%d, '%.20s...%s', %d)... ", >> + snprintf(msg, 512, "Check success of execveat(%d, '%.20s...%s', %d)...\n", >> fd, path, (path + pathlen - 20), flags); >> else >> - printf("Check success of execveat(%d, '%s', %d)... ", >> + snprintf(msg, 512, "Check success of execveat(%d, '%s', %d)...\n", >> fd, path?:"(null)", flags); > > Is there is a need to use snprinf() here? ksft_test_result_fail() does suppoort > var args now. Also, please use sizeof(msg) instead of 512. > >> child = fork(); >> if (child < 0) { >> - printf("[FAIL] (fork() failed)\n"); >> + ksft_test_result_fail(msg); >> + ksft_print_msg("fork() failed\n"); >> return 1; >> } >> if (child == 0) { >> /* Child: do execveat(). */ >> rc = execveat_(fd, path, argv, envp, flags); >> - printf("[FAIL]: execveat() failed, rc=%d errno=%d (%s)\n", >> + ksft_exit_fail_msg("execveat() failed, rc=%d errno=%d (%s)\n", >> rc, errno, strerror(errno)); >> exit(1); /* should not reach here */ >> } >> /* Parent: wait for & check child's exit status. */ >> rc = waitpid(child, &status, 0); >> if (rc != child) { >> - printf("[FAIL] (waitpid(%d,...) returned %d)\n", child, rc); >> + ksft_test_result_fail(msg); >> + ksft_print_msg("waitpid(%d,...) returned %d\n", child, rc); > > I think one single message to indicate the test result is desirable as > opposed to two messages. > >> return 1; >> } >> if (!WIFEXITED(status)) { >> - printf("[FAIL] (child %d did not exit cleanly, status=%08x)\n", >> + ksft_test_result_fail(msg); >> + ksft_print_msg("child %d did not exit cleanly, status=%08x\n", >> child, status); > > I think one single message to indicate the test result is desirable as > opposed to two messages. > >> return 1; >> } >> if ((WEXITSTATUS(status) != expected_rc) && >> (WEXITSTATUS(status) != expected_rc2)) { >> - printf("[FAIL] (child %d exited with %d not %d nor %d)\n", >> + ksft_test_result_fail(msg); >> + ksft_print_msg("child %d exited with %d not %d nor %d\n", >> child, WEXITSTATUS(status), expected_rc, expected_rc2); >> return 1; >> } >> - printf("[OK]\n"); >> + ksft_test_result_pass(msg); > > I see why you are doing snprintf() - looks like you are using the msg buffer. > That is okay in this case. > >> return 0; >> } >> >> @@ -127,7 +137,7 @@ static int open_or_die(const char *filename, int flags) >> int fd = open(filename, flags); >> >> if (fd < 0) { >> - printf("Failed to open '%s'; " >> + ksft_exit_fail_msg("Failed to open '%s'; " >> "check prerequisites are available\n", filename); >> exit(1); >> } >> @@ -180,11 +190,11 @@ static int check_execveat_pathmax(int dot_dfd, const char *src, int is_script) >> */ >> fd = open(longpath, O_RDONLY); >> if (fd > 0) { >> - printf("Invoke copy of '%s' via filename of length %zu:\n", >> + ksft_print_msg("Invoke copy of '%s' via filename of length %zu:\n", >> src, strlen(longpath)); >> fail += check_execveat(fd, "", AT_EMPTY_PATH); >> } else { >> - printf("Failed to open length %zu filename, errno=%d (%s)\n", >> + ksft_print_msg("Failed to open length %zu filename, errno=%d (%s)\n", >> strlen(longpath), errno, strerror(errno)); >> fail++; >> } >> @@ -210,6 +220,8 @@ static int check_execveat_pathmax(int dot_dfd, const char *src, int is_script) >> >> static int run_tests(void) >> { >> + ksft_print_header(); > > This goes below the declaration block. Okay. > >> + >> int fail = 0; >> char *fullname = realpath("execveat", NULL); >> char *fullname_script = realpath("script", NULL); >> @@ -238,7 +250,7 @@ static int run_tests(void) >> errno = 0; >> execveat_(-1, NULL, NULL, NULL, 0); >> if (errno == ENOSYS) { >> - printf("[FAIL] ENOSYS calling execveat - no kernel support?\n"); >> + ksft_print_msg("[FAIL] ENOSYS calling execveat - no kernel support?\n"); >> return 1; >> } >> >> @@ -247,115 +259,115 @@ static int run_tests(void) >> >> /* Normal executable file: */ >> /* dfd + path */ >> - fail += check_execveat(subdir_dfd, "../execveat", 0); >> - fail += check_execveat(dot_dfd, "execveat", 0); >> - fail += check_execveat(dot_dfd_path, "execveat", 0); >> + check_execveat(subdir_dfd, "../execveat", 0); >> + check_execveat(dot_dfd, "execveat", 0); >> + check_execveat(dot_dfd_path, "execveat", 0); >> /* absolute path */ >> - fail += check_execveat(AT_FDCWD, fullname, 0); >> + check_execveat(AT_FDCWD, fullname, 0); >> /* absolute path with nonsense dfd */ >> - fail += check_execveat(99, fullname, 0); >> + check_execveat(99, fullname, 0); >> /* fd + no path */ >> - fail += check_execveat(fd, "", AT_EMPTY_PATH); >> + check_execveat(fd, "", AT_EMPTY_PATH); >> /* O_CLOEXEC fd + no path */ >> - fail += check_execveat(fd_cloexec, "", AT_EMPTY_PATH); >> + check_execveat(fd_cloexec, "", AT_EMPTY_PATH); >> /* O_PATH fd */ >> - fail += check_execveat(fd_path, "", AT_EMPTY_PATH); >> + check_execveat(fd_path, "", AT_EMPTY_PATH); >> >> /* Mess with executable file that's already open: */ >> /* fd + no path to a file that's been renamed */ >> rename("execveat.ephemeral", "execveat.moved"); >> - fail += check_execveat(fd_ephemeral, "", AT_EMPTY_PATH); >> + check_execveat(fd_ephemeral, "", AT_EMPTY_PATH); >> /* fd + no path to a file that's been deleted */ >> unlink("execveat.moved"); /* remove the file now fd open */ >> - fail += check_execveat(fd_ephemeral, "", AT_EMPTY_PATH); >> + check_execveat(fd_ephemeral, "", AT_EMPTY_PATH); >> >> /* Mess with executable file that's already open with O_PATH */ >> /* fd + no path to a file that's been deleted */ >> unlink("execveat.path.ephemeral"); >> - fail += check_execveat(fd_ephemeral_path, "", AT_EMPTY_PATH); >> + check_execveat(fd_ephemeral_path, "", AT_EMPTY_PATH); >> >> /* Invalid argument failures */ >> - fail += check_execveat_fail(fd, "", 0, ENOENT); >> - fail += check_execveat_fail(fd, NULL, AT_EMPTY_PATH, EFAULT); >> + check_execveat_fail(fd, "", 0, ENOENT); >> + check_execveat_fail(fd, NULL, AT_EMPTY_PATH, EFAULT); >> >> /* Symlink to executable file: */ >> /* dfd + path */ >> - fail += check_execveat(dot_dfd, "execveat.symlink", 0); >> - fail += check_execveat(dot_dfd_path, "execveat.symlink", 0); >> + check_execveat(dot_dfd, "execveat.symlink", 0); >> + check_execveat(dot_dfd_path, "execveat.symlink", 0); >> /* absolute path */ >> - fail += check_execveat(AT_FDCWD, fullname_symlink, 0); >> + check_execveat(AT_FDCWD, fullname_symlink, 0); >> /* fd + no path, even with AT_SYMLINK_NOFOLLOW (already followed) */ >> - fail += check_execveat(fd_symlink, "", AT_EMPTY_PATH); >> - fail += check_execveat(fd_symlink, "", >> + check_execveat(fd_symlink, "", AT_EMPTY_PATH); >> + check_execveat(fd_symlink, "", >> AT_EMPTY_PATH|AT_SYMLINK_NOFOLLOW); >> >> /* Symlink fails when AT_SYMLINK_NOFOLLOW set: */ >> /* dfd + path */ >> - fail += check_execveat_fail(dot_dfd, "execveat.symlink", >> + check_execveat_fail(dot_dfd, "execveat.symlink", >> AT_SYMLINK_NOFOLLOW, ELOOP); >> - fail += check_execveat_fail(dot_dfd_path, "execveat.symlink", >> + check_execveat_fail(dot_dfd_path, "execveat.symlink", >> AT_SYMLINK_NOFOLLOW, ELOOP); >> /* absolute path */ >> - fail += check_execveat_fail(AT_FDCWD, fullname_symlink, >> + check_execveat_fail(AT_FDCWD, fullname_symlink, >> AT_SYMLINK_NOFOLLOW, ELOOP); >> >> /* Shell script wrapping executable file: */ >> /* dfd + path */ >> - fail += check_execveat(subdir_dfd, "../script", 0); >> - fail += check_execveat(dot_dfd, "script", 0); >> - fail += check_execveat(dot_dfd_path, "script", 0); >> + check_execveat(subdir_dfd, "../script", 0); >> + check_execveat(dot_dfd, "script", 0); >> + check_execveat(dot_dfd_path, "script", 0); >> /* absolute path */ >> - fail += check_execveat(AT_FDCWD, fullname_script, 0); >> + check_execveat(AT_FDCWD, fullname_script, 0); >> /* fd + no path */ >> - fail += check_execveat(fd_script, "", AT_EMPTY_PATH); >> - fail += check_execveat(fd_script, "", >> + check_execveat(fd_script, "", AT_EMPTY_PATH); >> + check_execveat(fd_script, "", >> AT_EMPTY_PATH|AT_SYMLINK_NOFOLLOW); >> /* O_CLOEXEC fd fails for a script (as script file inaccessible) */ >> - fail += check_execveat_fail(fd_script_cloexec, "", AT_EMPTY_PATH, >> + check_execveat_fail(fd_script_cloexec, "", AT_EMPTY_PATH, >> ENOENT); >> - fail += check_execveat_fail(dot_dfd_cloexec, "script", 0, ENOENT); >> + check_execveat_fail(dot_dfd_cloexec, "script", 0, ENOENT); >> >> /* Mess with script file that's already open: */ >> /* fd + no path to a file that's been renamed */ >> rename("script.ephemeral", "script.moved"); >> - fail += check_execveat(fd_script_ephemeral, "", AT_EMPTY_PATH); >> + check_execveat(fd_script_ephemeral, "", AT_EMPTY_PATH); >> /* fd + no path to a file that's been deleted */ >> unlink("script.moved"); /* remove the file while fd open */ >> - fail += check_execveat(fd_script_ephemeral, "", AT_EMPTY_PATH); >> + check_execveat(fd_script_ephemeral, "", AT_EMPTY_PATH); >> >> /* Rename a subdirectory in the path: */ >> rename("subdir.ephemeral", "subdir.moved"); >> - fail += check_execveat(subdir_dfd_ephemeral, "../script", 0); >> - fail += check_execveat(subdir_dfd_ephemeral, "script", 0); >> + check_execveat(subdir_dfd_ephemeral, "../script", 0); >> + check_execveat(subdir_dfd_ephemeral, "script", 0); >> /* Remove the subdir and its contents */ >> unlink("subdir.moved/script"); >> unlink("subdir.moved"); >> /* Shell loads via deleted subdir OK because name starts with .. */ >> - fail += check_execveat(subdir_dfd_ephemeral, "../script", 0); >> - fail += check_execveat_fail(subdir_dfd_ephemeral, "script", 0, ENOENT); >> + check_execveat(subdir_dfd_ephemeral, "../script", 0); >> + check_execveat_fail(subdir_dfd_ephemeral, "script", 0, ENOENT); >> >> /* Flag values other than AT_SYMLINK_NOFOLLOW => EINVAL */ >> - fail += check_execveat_fail(dot_dfd, "execveat", 0xFFFF, EINVAL); >> + check_execveat_fail(dot_dfd, "execveat", 0xFFFF, EINVAL); >> /* Invalid path => ENOENT */ >> - fail += check_execveat_fail(dot_dfd, "no-such-file", 0, ENOENT); >> - fail += check_execveat_fail(dot_dfd_path, "no-such-file", 0, ENOENT); >> - fail += check_execveat_fail(AT_FDCWD, "no-such-file", 0, ENOENT); >> + check_execveat_fail(dot_dfd, "no-such-file", 0, ENOENT); >> + check_execveat_fail(dot_dfd_path, "no-such-file", 0, ENOENT); >> + check_execveat_fail(AT_FDCWD, "no-such-file", 0, ENOENT); >> /* Attempt to execute directory => EACCES */ >> fail += check_execveat_fail(dot_dfd, "", AT_EMPTY_PATH, EACCES); >> /* Attempt to execute non-executable => EACCES */ >> - fail += check_execveat_fail(dot_dfd, "Makefile", 0, EACCES); >> - fail += check_execveat_fail(fd_denatured, "", AT_EMPTY_PATH, EACCES); >> - fail += check_execveat_fail(fd_denatured_path, "", AT_EMPTY_PATH, >> + check_execveat_fail(dot_dfd, "Makefile", 0, EACCES); >> + check_execveat_fail(fd_denatured, "", AT_EMPTY_PATH, EACCES); >> + check_execveat_fail(fd_denatured_path, "", AT_EMPTY_PATH, >> EACCES); >> /* Attempt to execute nonsense FD => EBADF */ >> - fail += check_execveat_fail(99, "", AT_EMPTY_PATH, EBADF); >> - fail += check_execveat_fail(99, "execveat", 0, EBADF); >> + check_execveat_fail(99, "", AT_EMPTY_PATH, EBADF); >> + check_execveat_fail(99, "execveat", 0, EBADF); >> /* Attempt to execute relative to non-directory => ENOTDIR */ >> - fail += check_execveat_fail(fd, "execveat", 0, ENOTDIR); >> + check_execveat_fail(fd, "execveat", 0, ENOTDIR); >> >> - fail += check_execveat_pathmax(dot_dfd, "execveat", 0); >> - fail += check_execveat_pathmax(dot_dfd, "script", 1); >> - return fail; >> + check_execveat_pathmax(dot_dfd, "execveat", 0); >> + check_execveat_pathmax(dot_dfd, "script", 1); >> + return ksft_get_fail_cnt(); >> } >> >> static void prerequisites(void) >> @@ -405,8 +417,10 @@ int main(int argc, char **argv) >> if (verbose) >> envp[1] = "VERBOSE=1"; >> rc = run_tests(); >> - if (rc > 0) >> - printf("%d tests failed\n", rc); >> + if (rc) >> + ksft_exit_fail(); >> + else >> + ksft_exit_pass(); >> } >> return rc; >> } >> > > Looks like main() still needs some changes for: > if (!in_test || strcmp(in_test, "yes") != 0) { > > case. Yes, it does. Looks like I missed it. Thank you, Paul > > thanks, > -- Shuah > -- 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
