On Mon, Mar 18, 2024 at 8:30 AM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
>
>
> On 3/18/24 4:36 AM, Jiri Olsa wrote:
> > 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
>
> Since this patch intends to just replace CHECK with ASSERT_*.
> I tried to keep other parts of codes the same as before. But
> I can certainly do this. I will wait for other comments
> before sending another revision.
>
I applied it as is, but feel free to send suggested improvements as a follow up.
> >
> > 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;
> >> }
> [...]
