Re: [PATCH bpf-next 1/4] selftests/bpf: Replace CHECK with ASSERT macros for ksyms test

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

On 3/4/24 4:17 PM, Andrii Nakryiko wrote:
> On Sat, Mar 2, 2024 at 8:50 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>> I am going to modify ksyms test later so take this opportunity
>> to replace old CHECK macros with new ASSERT macros.
>> No functionality change.
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>   .../testing/selftests/bpf/prog_tests/ksyms.c  | 38 +++++++++----------
>>   1 file changed, 19 insertions(+), 19 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/ksyms.c b/tools/testing/selftests/bpf/prog_tests/ksyms.c
>> index b295969b263b..e081f8bf3f17 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/ksyms.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/ksyms.c
>> @@ -5,8 +5,6 @@
>>   #include "test_ksyms.skel.h"
>>   #include <sys/stat.h>
>>
>> -static int duration;
>> -
>>   void test_ksyms(void)
>>   {
>>          const char *btf_path = "/sys/kernel/btf/vmlinux";
>> @@ -18,43 +16,45 @@ void test_ksyms(void)
>>          int err;
>>
>>          err = kallsyms_find("bpf_link_fops", &link_fops_addr);
>> -       if (CHECK(err == -EINVAL, "kallsyms_fopen", "failed to open: %d\n", errno))
>> +       if (err == -EINVAL) {
>> +               ASSERT_TRUE(false, "kallsyms_fopen for bpf_link_fops");
> should this (and few other cases below) be ASSERT_EQ()/ASSERT_NEQ()
> (whichever makes sense, I can't reason about CHECK() conditions).

The below 'err == -ENOENT' case will later be modified in Patch 3 where I cannot
do ASSERT_EQ(err, -ENOENT) which is why I do 'err == -EINVAL' here. But you have
a good point that ASSERT_EQ/NEQ is easier to understand. Will fix it in the next
revision.

>
> ASSERT_TRUE(false) is a last resort way, we have more meaningful checks.
>
>>                  return;
>> -       if (CHECK(err == -ENOENT, "ksym_find", "symbol 'bpf_link_fops' not found\n"))
>> +       }
>> +       if (err == -ENOENT) {
>> +               ASSERT_TRUE(false, "ksym_find for bpf_link_fops");
>>                  return;
>> +       }
>>
>>          err = kallsyms_find("__per_cpu_start", &per_cpu_start_addr);
>> -       if (CHECK(err == -EINVAL, "kallsyms_fopen", "failed to open: %d\n", errno))
>> +       if (err == -EINVAL) {
>> +               ASSERT_TRUE(false, "kallsyms_fopen for __per_cpu_start");
>>                  return;
>> -       if (CHECK(err == -ENOENT, "ksym_find", "symbol 'per_cpu_start' not found\n"))
>> +       }
>> +       if (err == -ENOENT) {
>> +               ASSERT_TRUE(false, "ksym_find for __per_cpu_start");
>>                  return;
>> +       }
[...]