On Fri, Mar 15, 2024 at 11:48:59AM -0700, Yonghong Song wrote:
> Replace CHECK in selftest ns_current_pid_tgid with recommended ASSERT_* style.
> I also shortened subtest name as the prefix of subtest name is covered
> by the test name already.
>
> This patch does fix a testing issue. Currently even if bss->user_{pid,tgid}
> is not correct, the test still passed since the clone func returns 0.
> I fixed it to return a non-zero value if bss->user_{pid,tgid} is incorrect.
>
> Signed-off-by: Yonghong Song <yonghong.song@xxxxxxxxx>
> ---
> .../bpf/prog_tests/ns_current_pid_tgid.c | 36 ++++++++++---------
> 1 file changed, 19 insertions(+), 17 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c b/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c
> index 24d493482ffc..3a0664a86243 100644
> --- a/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c
> +++ b/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c
> @@ -20,19 +20,19 @@ static int test_current_pid_tgid(void *args)
> {
> struct test_ns_current_pid_tgid__bss *bss;
> struct test_ns_current_pid_tgid *skel;
> - int err = -1, duration = 0;
> + int ret = -1, err;
> pid_t tgid, pid;
> struct stat st;
>
> skel = test_ns_current_pid_tgid__open_and_load();
> - if (CHECK(!skel, "skel_open_load", "failed to load skeleton\n"))
> - goto cleanup;
> + if (!ASSERT_OK_PTR(skel, "test_ns_current_pid_tgid__open_and_load"))
> + goto out;
you could just return in here so there's no need for the out label
otherwise lgtm
Acked-by: Jiri Olsa <jolsa@xxxxxxxxxx>
jirka
>
> pid = syscall(SYS_gettid);
> tgid = getpid();
>
> err = stat("/proc/self/ns/pid", &st);
> - if (CHECK(err, "stat", "failed /proc/self/ns/pid: %d\n", err))
> + if (!ASSERT_OK(err, "stat /proc/self/ns/pid"))
> goto cleanup;
>
> bss = skel->bss;
> @@ -42,24 +42,26 @@ static int test_current_pid_tgid(void *args)
> bss->user_tgid = 0;
>
> err = test_ns_current_pid_tgid__attach(skel);
> - if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
> + if (!ASSERT_OK(err, "test_ns_current_pid_tgid__attach"))
> goto cleanup;
>
> /* trigger tracepoint */
> usleep(1);
> - ASSERT_EQ(bss->user_pid, pid, "pid");
> - ASSERT_EQ(bss->user_tgid, tgid, "tgid");
> - err = 0;
> + if (!ASSERT_EQ(bss->user_pid, pid, "pid"))
> + goto cleanup;
> + if (!ASSERT_EQ(bss->user_tgid, tgid, "tgid"))
> + goto cleanup;
> + ret = 0;
>
> cleanup:
> - test_ns_current_pid_tgid__destroy(skel);
> -
> - return err;
> + test_ns_current_pid_tgid__destroy(skel);
> +out:
> + return ret;
> }
>
> static void test_ns_current_pid_tgid_new_ns(void)
> {
> - int wstatus, duration = 0;
> + int wstatus;
> pid_t cpid;
>
> /* Create a process in a new namespace, this process
> @@ -68,21 +70,21 @@ static void test_ns_current_pid_tgid_new_ns(void)
> cpid = clone(test_current_pid_tgid, child_stack + STACK_SIZE,
> CLONE_NEWPID | SIGCHLD, NULL);
>
> - if (CHECK(cpid == -1, "clone", "%s\n", strerror(errno)))
> + if (!ASSERT_NEQ(cpid, -1, "clone"))
> return;
>
> - if (CHECK(waitpid(cpid, &wstatus, 0) == -1, "waitpid", "%s\n", strerror(errno)))
> + if (!ASSERT_NEQ(waitpid(cpid, &wstatus, 0), -1, "waitpid"))
> return;
>
> - if (CHECK(WEXITSTATUS(wstatus) != 0, "newns_pidtgid", "failed"))
> + if (!ASSERT_OK(WEXITSTATUS(wstatus), "newns_pidtgid"))
> return;
> }
>
> /* TODO: use a different tracepoint */
> void serial_test_ns_current_pid_tgid(void)
> {
> - if (test__start_subtest("ns_current_pid_tgid_root_ns"))
> + if (test__start_subtest("root_ns_tp"))
> test_current_pid_tgid(NULL);
> - if (test__start_subtest("ns_current_pid_tgid_new_ns"))
> + if (test__start_subtest("new_ns_tp"))
> test_ns_current_pid_tgid_new_ns();
> }
> --
> 2.43.0
>
>
