diff mbox series

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

Message ID 20240315184859.2975543-1-yonghong.song@linux.dev (mailing list archive)
State Accepted
Commit 84239a24d10174fcfc7d6760cb120435a6ff69af
Delegated to: BPF
Headers show
Series bpf: Allow helper bpf_get_[ns_]current_pid_tgid() for all prog types | expand

Checks

Context Check Description
netdev/series_format warning Series does not have a cover letter
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 10 maintainers not CCed: haoluo@google.com linux-kselftest@vger.kernel.org eddyz87@gmail.com sdf@google.com song@kernel.org kpsingh@kernel.org shuah@kernel.org martin.lau@linux.dev jolsa@kernel.org mykolal@fb.com
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 83 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 fail Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization

Commit Message

Yonghong Song March 15, 2024, 6:48 p.m. UTC 
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(-)

Comments

Jiri Olsa March 18, 2024, 11:36 a.m. UTC | #1 
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

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;
>  }
>  
>  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
> 
>
Yonghong Song March 18, 2024, 3:30 p.m. UTC | #2 
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;
>>   }
[...]
Andrii Nakryiko March 19, 2024, 9:52 p.m. UTC | #3 
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.


> >
> > 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;
> >>   }
> [...]
Yonghong Song March 20, 2024, 12:52 a.m. UTC | #4 
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;
>>>>    }
>> [...]
Yonghong Song March 21, 2024, 8:15 p.m. UTC | #5 
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.

Actually the patch (after this commit)
   4d4bd29e363c  selftests/bpf: Refactor out some functions in ns_current_pid_tgid test
already did the refactoring. So I don't need to do anything.

>
>
>>> Acked-by: Jiri Olsa <jolsa@kernel.org>
>>>
>>>
>>> jirka
[...]
diff mbox series

Patch

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;
 
 	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();
 }