| Message ID | 20211101124310.3947887-1-yangyingliang@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [-next,v2] bpf/benchs: Fix return value check of bpf_program__attach() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| bpf/vmtest-bpf-next-PR | fail | PR summary |
| bpf/vmtest-bpf-PR | fail | merge-conflict |
| bpf/vmtest-bpf-next | fail | VM_Test |
On Mon, Nov 1, 2021 at 5:35 AM Yang Yingliang <yangyingliang@huawei.com> wrote: > > If bpf_program__attach() fails, it never returns NULL, > we should use libbpf_get_error() to check the return value. > > Reported-by: Hulk Robot <hulkci@huawei.com> > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > Acked-by: Yonghong Song <yhs@fb.com> > --- > v2: > don't use 'int err' > --- > .../selftests/bpf/benchs/bench_bloom_filter_map.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c b/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c > index 6eeeed2913e6..4afaa4adb327 100644 > --- a/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c > +++ b/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c > @@ -304,7 +304,7 @@ static void bloom_lookup_setup(void) > populate_maps(); > > link = bpf_program__attach(ctx.skel->progs.bloom_lookup); > - if (!link) { > + if (libbpf_get_error(link)) { Please use ASSERT_OK_PTR() instead. See how other tests are doing it.
On 11/1/21 3:00 PM, Alexei Starovoitov wrote: > On Mon, Nov 1, 2021 at 5:35 AM Yang Yingliang <yangyingliang@huawei.com> wrote: >> >> If bpf_program__attach() fails, it never returns NULL, >> we should use libbpf_get_error() to check the return value. >> >> Reported-by: Hulk Robot <hulkci@huawei.com> >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >> Acked-by: Yonghong Song <yhs@fb.com> >> --- >> v2: >> don't use 'int err' >> --- >> .../selftests/bpf/benchs/bench_bloom_filter_map.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c b/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c >> index 6eeeed2913e6..4afaa4adb327 100644 >> --- a/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c >> +++ b/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c >> @@ -304,7 +304,7 @@ static void bloom_lookup_setup(void) >> populate_maps(); >> >> link = bpf_program__attach(ctx.skel->progs.bloom_lookup); >> - if (!link) { >> + if (libbpf_get_error(link)) { > > Please use ASSERT_OK_PTR() instead. > See how other tests are doing it. I actually looked at this. ASSERT_OK_PTR() is defined in test_progs.h and test_progs.h is ONLY included in files which eventually linked to test_progs. That is why I didn't recommend to use ASSERT_OK_PTR(). Maybe it is okay to include test_progs.h in benchs/*.c. Or we may want to refactor to a separate header file to contain these macros which can be used for test_progs.h and other applications. >
On Mon, Nov 1, 2021 at 3:21 PM Yonghong Song <yhs@fb.com> wrote: > > > > On 11/1/21 3:00 PM, Alexei Starovoitov wrote: > > On Mon, Nov 1, 2021 at 5:35 AM Yang Yingliang <yangyingliang@huawei.com> wrote: > >> > >> If bpf_program__attach() fails, it never returns NULL, > >> we should use libbpf_get_error() to check the return value. > >> > >> Reported-by: Hulk Robot <hulkci@huawei.com> > >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > >> Acked-by: Yonghong Song <yhs@fb.com> > >> --- > >> v2: > >> don't use 'int err' > >> --- > >> .../selftests/bpf/benchs/bench_bloom_filter_map.c | 10 +++++----- > >> 1 file changed, 5 insertions(+), 5 deletions(-) > >> > >> diff --git a/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c b/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c > >> index 6eeeed2913e6..4afaa4adb327 100644 > >> --- a/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c > >> +++ b/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c > >> @@ -304,7 +304,7 @@ static void bloom_lookup_setup(void) > >> populate_maps(); > >> > >> link = bpf_program__attach(ctx.skel->progs.bloom_lookup); > >> - if (!link) { > >> + if (libbpf_get_error(link)) { > > > > Please use ASSERT_OK_PTR() instead. > > See how other tests are doing it. > > I actually looked at this. ASSERT_OK_PTR() is defined in test_progs.h > and test_progs.h is ONLY included in files which eventually linked to > test_progs. That is why I didn't recommend to use ASSERT_OK_PTR(). > > Maybe it is okay to include test_progs.h in benchs/*.c. Or we may > want to refactor to a separate header file to contain these macros > which can be used for test_progs.h and other applications. hmm. Looks like bench_ringbufs.c has the same issue doing: if (!link) and bench_rename.c too. Probably would be good to fix in all bench-s. If test_progs.h cannot be included directly copy-pasting ASSERT_OK_PTR in a reduced form into bench.h is probably cleaner than open coding libbpf_get_error.
On Mon, Nov 1, 2021 at 3:30 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Mon, Nov 1, 2021 at 3:21 PM Yonghong Song <yhs@fb.com> wrote: > > > > > > > > On 11/1/21 3:00 PM, Alexei Starovoitov wrote: > > > On Mon, Nov 1, 2021 at 5:35 AM Yang Yingliang <yangyingliang@huawei.com> wrote: > > >> > > >> If bpf_program__attach() fails, it never returns NULL, > > >> we should use libbpf_get_error() to check the return value. > > >> > > >> Reported-by: Hulk Robot <hulkci@huawei.com> > > >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > > >> Acked-by: Yonghong Song <yhs@fb.com> > > >> --- > > >> v2: > > >> don't use 'int err' > > >> --- > > >> .../selftests/bpf/benchs/bench_bloom_filter_map.c | 10 +++++----- > > >> 1 file changed, 5 insertions(+), 5 deletions(-) > > >> > > >> diff --git a/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c b/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c > > >> index 6eeeed2913e6..4afaa4adb327 100644 > > >> --- a/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c > > >> +++ b/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c > > >> @@ -304,7 +304,7 @@ static void bloom_lookup_setup(void) > > >> populate_maps(); > > >> > > >> link = bpf_program__attach(ctx.skel->progs.bloom_lookup); > > >> - if (!link) { > > >> + if (libbpf_get_error(link)) { > > > > > > Please use ASSERT_OK_PTR() instead. > > > See how other tests are doing it. > > > > I actually looked at this. ASSERT_OK_PTR() is defined in test_progs.h > > and test_progs.h is ONLY included in files which eventually linked to > > test_progs. That is why I didn't recommend to use ASSERT_OK_PTR(). > > > > Maybe it is okay to include test_progs.h in benchs/*.c. Or we may > > want to refactor to a separate header file to contain these macros > > which can be used for test_progs.h and other applications. > > hmm. > Looks like bench_ringbufs.c has the same issue doing: > if (!link) > and bench_rename.c too. bench.c does: libbpf_set_strict_mode(LIBBPF_STRICT_ALL); and so on error all the pointers will be NULL. So it's ok to check if (!link) and not use libbpf_get_error() at all. > > Probably would be good to fix in all bench-s. > > If test_progs.h cannot be included directly > copy-pasting ASSERT_OK_PTR in a reduced form into bench.h > is probably cleaner than open coding libbpf_get_error.
diff --git a/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c b/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c index 6eeeed2913e6..4afaa4adb327 100644 --- a/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c +++ b/tools/testing/selftests/bpf/benchs/bench_bloom_filter_map.c @@ -304,7 +304,7 @@ static void bloom_lookup_setup(void) populate_maps(); link = bpf_program__attach(ctx.skel->progs.bloom_lookup); - if (!link) { + if (libbpf_get_error(link)) { fprintf(stderr, "failed to attach program!\n"); exit(1); } @@ -321,7 +321,7 @@ static void bloom_update_setup(void) populate_maps(); link = bpf_program__attach(ctx.skel->progs.bloom_update); - if (!link) { + if (libbpf_get_error(link)) { fprintf(stderr, "failed to attach program!\n"); exit(1); } @@ -340,7 +340,7 @@ static void false_positive_setup(void) populate_maps(); link = bpf_program__attach(ctx.skel->progs.bloom_hashmap_lookup); - if (!link) { + if (libbpf_get_error(link)) { fprintf(stderr, "failed to attach program!\n"); exit(1); } @@ -358,7 +358,7 @@ static void hashmap_with_bloom_setup(void) populate_maps(); link = bpf_program__attach(ctx.skel->progs.bloom_hashmap_lookup); - if (!link) { + if (libbpf_get_error(link)) { fprintf(stderr, "failed to attach program!\n"); exit(1); } @@ -375,7 +375,7 @@ static void hashmap_no_bloom_setup(void) populate_maps(); link = bpf_program__attach(ctx.skel->progs.bloom_hashmap_lookup); - if (!link) { + if (libbpf_get_error(link)) { fprintf(stderr, "failed to attach program!\n"); exit(1); }