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/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.

>
> 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;
>>   }
[...]