Re: [PATCH bpf-next v2 2/5] selftests/bpf: Replace CHECK with ASSERT_* in ns_current_pid_tgid test

[Yonghong Song <yonghong.song@linux.dev>]

On 3/19/24 2:52 PM, Andrii Nakryiko wrote:
> On Mon, Mar 18, 2024 at 8:30 AM Yonghong Song <yonghong.song@linux.dev> 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@linux.dev>
>>>> ---
>>>>    .../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.

Thanks! Will send a followup later.

>
>
>>> Acked-by: Jiri Olsa <jolsa@kernel.org>
>>>
>>>
>>> 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;
>>>>    }
>> [...]