* [PATCH] selftests/bpf: Fix return value check in attach_bpf() @ 2021-05-28 9:07 Yu Kuai 2021-05-28 20:46 ` Daniel Borkmann 2021-05-28 21:16 ` John Fastabend 0 siblings, 2 replies; 6+ messages in thread From: Yu Kuai @ 2021-05-28 9:07 UTC (permalink / raw) To: shuah, ast, daniel, andrii Cc: linux-kselftest, netdev, bpf, linux-kernel, yukuai3, yi.zhang use libbpf_get_error() to check the return value of bpf_program__attach(). Reported-by: Hulk Robot <hulkci@huawei.com> Signed-off-by: Yu Kuai <yukuai3@huawei.com> --- tools/testing/selftests/bpf/benchs/bench_rename.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/testing/selftests/bpf/benchs/bench_rename.c b/tools/testing/selftests/bpf/benchs/bench_rename.c index c7ec114eca56..b7d4a1d74fca 100644 --- a/tools/testing/selftests/bpf/benchs/bench_rename.c +++ b/tools/testing/selftests/bpf/benchs/bench_rename.c @@ -65,7 +65,7 @@ static void attach_bpf(struct bpf_program *prog) struct bpf_link *link; link = bpf_program__attach(prog); - if (!link) { + if (libbpf_get_error(link)) { fprintf(stderr, "failed to attach program!\n"); exit(1); } -- 2.25.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] selftests/bpf: Fix return value check in attach_bpf() 2021-05-28 9:07 [PATCH] selftests/bpf: Fix return value check in attach_bpf() Yu Kuai @ 2021-05-28 20:46 ` Daniel Borkmann 2021-05-29 1:25 ` yukuai (C) 2021-05-28 21:16 ` John Fastabend 1 sibling, 1 reply; 6+ messages in thread From: Daniel Borkmann @ 2021-05-28 20:46 UTC (permalink / raw) To: Yu Kuai, shuah, ast, andrii Cc: linux-kselftest, netdev, bpf, linux-kernel, yi.zhang On 5/28/21 11:07 AM, Yu Kuai wrote: > use libbpf_get_error() to check the return value of > bpf_program__attach(). > > Reported-by: Hulk Robot <hulkci@huawei.com> > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > --- > tools/testing/selftests/bpf/benchs/bench_rename.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/bpf/benchs/bench_rename.c b/tools/testing/selftests/bpf/benchs/bench_rename.c > index c7ec114eca56..b7d4a1d74fca 100644 > --- a/tools/testing/selftests/bpf/benchs/bench_rename.c > +++ b/tools/testing/selftests/bpf/benchs/bench_rename.c > @@ -65,7 +65,7 @@ static void attach_bpf(struct bpf_program *prog) > struct bpf_link *link; > > link = bpf_program__attach(prog); > - if (!link) { > + if (libbpf_get_error(link)) { > fprintf(stderr, "failed to attach program!\n"); > exit(1); > } Could you explain the rationale of this patch? bad2e478af3b ("selftests/bpf: Turn on libbpf 1.0 mode and fix all IS_ERR checks") explains: 'Fix all the explicit IS_ERR checks that now will be broken because libbpf returns NULL on error (and sets errno).' So the !link check looks totally reasonable to me. Converting to libbpf_get_error() is not wrong in itself, but given you don't make any use of the err code, there is also no point in this diff here. Thanks, Daniel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] selftests/bpf: Fix return value check in attach_bpf() 2021-05-28 20:46 ` Daniel Borkmann @ 2021-05-29 1:25 ` yukuai (C) 2021-05-30 1:17 ` Andrii Nakryiko 0 siblings, 1 reply; 6+ messages in thread From: yukuai (C) @ 2021-05-29 1:25 UTC (permalink / raw) To: Daniel Borkmann, shuah, ast, andrii Cc: linux-kselftest, netdev, bpf, linux-kernel, yi.zhang On 2021/05/29 4:46, Daniel Borkmann wrote: > On 5/28/21 11:07 AM, Yu Kuai wrote: >> use libbpf_get_error() to check the return value of >> bpf_program__attach(). >> >> Reported-by: Hulk Robot <hulkci@huawei.com> >> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >> --- >> tools/testing/selftests/bpf/benchs/bench_rename.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/tools/testing/selftests/bpf/benchs/bench_rename.c >> b/tools/testing/selftests/bpf/benchs/bench_rename.c >> index c7ec114eca56..b7d4a1d74fca 100644 >> --- a/tools/testing/selftests/bpf/benchs/bench_rename.c >> +++ b/tools/testing/selftests/bpf/benchs/bench_rename.c >> @@ -65,7 +65,7 @@ static void attach_bpf(struct bpf_program *prog) >> struct bpf_link *link; >> link = bpf_program__attach(prog); >> - if (!link) { >> + if (libbpf_get_error(link)) { >> fprintf(stderr, "failed to attach program!\n"); >> exit(1); >> } > > Could you explain the rationale of this patch? bad2e478af3b > ("selftests/bpf: Turn > on libbpf 1.0 mode and fix all IS_ERR checks") explains: 'Fix all the > explicit > IS_ERR checks that now will be broken because libbpf returns NULL on > error (and > sets errno).' So the !link check looks totally reasonable to me. > Converting to > libbpf_get_error() is not wrong in itself, but given you don't make any > use of > the err code, there is also no point in this diff here. Hi, I was thinking that bpf_program__attach() can return error code theoretically(for example -ESRCH), and such case need to be handled. Thanks, Yu Kuai > > Thanks, > Daniel > . > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] selftests/bpf: Fix return value check in attach_bpf() 2021-05-29 1:25 ` yukuai (C) @ 2021-05-30 1:17 ` Andrii Nakryiko 0 siblings, 0 replies; 6+ messages in thread From: Andrii Nakryiko @ 2021-05-30 1:17 UTC (permalink / raw) To: yukuai (C) Cc: Daniel Borkmann, Shuah Khan, Alexei Starovoitov, Andrii Nakryiko, open list:KERNEL SELFTEST FRAMEWORK, Networking, bpf, open list, yi.zhang On Fri, May 28, 2021 at 6:25 PM yukuai (C) <yukuai3@huawei.com> wrote: > > On 2021/05/29 4:46, Daniel Borkmann wrote: > > On 5/28/21 11:07 AM, Yu Kuai wrote: > >> use libbpf_get_error() to check the return value of > >> bpf_program__attach(). > >> > >> Reported-by: Hulk Robot <hulkci@huawei.com> > >> Signed-off-by: Yu Kuai <yukuai3@huawei.com> > >> --- > >> tools/testing/selftests/bpf/benchs/bench_rename.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/tools/testing/selftests/bpf/benchs/bench_rename.c > >> b/tools/testing/selftests/bpf/benchs/bench_rename.c > >> index c7ec114eca56..b7d4a1d74fca 100644 > >> --- a/tools/testing/selftests/bpf/benchs/bench_rename.c > >> +++ b/tools/testing/selftests/bpf/benchs/bench_rename.c > >> @@ -65,7 +65,7 @@ static void attach_bpf(struct bpf_program *prog) > >> struct bpf_link *link; > >> link = bpf_program__attach(prog); > >> - if (!link) { > >> + if (libbpf_get_error(link)) { > >> fprintf(stderr, "failed to attach program!\n"); > >> exit(1); > >> } > > > > Could you explain the rationale of this patch? bad2e478af3b > > ("selftests/bpf: Turn > > on libbpf 1.0 mode and fix all IS_ERR checks") explains: 'Fix all the > > explicit > > IS_ERR checks that now will be broken because libbpf returns NULL on > > error (and > > sets errno).' So the !link check looks totally reasonable to me. > > Converting to > > libbpf_get_error() is not wrong in itself, but given you don't make any > > use of > > the err code, there is also no point in this diff here. > Hi, > > I was thinking that bpf_program__attach() can return error code > theoretically(for example -ESRCH), and such case need to be handled. > I explicitly changed to NULL check + libbpf 1.0 error reporting mode because I don't care about specific error in benchmarks. So as Daniel and John pointed out, existing code is correct and doesn't need adjustment. You are right, though, that error code is indeed returned, but you can check errno directly (but need to enable libbpf 1.0 mode) or use libbpf_get_error() (which will get deprecated some time before libbpf 1.0) if you don't know which mode your code will be run in. > Thanks, > Yu Kuai > > > > Thanks, > > Daniel > > . > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] selftests/bpf: Fix return value check in attach_bpf() 2021-05-28 9:07 [PATCH] selftests/bpf: Fix return value check in attach_bpf() Yu Kuai 2021-05-28 20:46 ` Daniel Borkmann @ 2021-05-28 21:16 ` John Fastabend 2021-05-28 21:19 ` John Fastabend 1 sibling, 1 reply; 6+ messages in thread From: John Fastabend @ 2021-05-28 21:16 UTC (permalink / raw) To: Yu Kuai, shuah, ast, daniel, andrii Cc: linux-kselftest, netdev, bpf, linux-kernel, yukuai3, yi.zhang Yu Kuai wrote: > use libbpf_get_error() to check the return value of > bpf_program__attach(). > > Reported-by: Hulk Robot <hulkci@huawei.com> > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > --- > tools/testing/selftests/bpf/benchs/bench_rename.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/bpf/benchs/bench_rename.c b/tools/testing/selftests/bpf/benchs/bench_rename.c > index c7ec114eca56..b7d4a1d74fca 100644 > --- a/tools/testing/selftests/bpf/benchs/bench_rename.c > +++ b/tools/testing/selftests/bpf/benchs/bench_rename.c > @@ -65,7 +65,7 @@ static void attach_bpf(struct bpf_program *prog) > struct bpf_link *link; > > link = bpf_program__attach(prog); > - if (!link) { > + if (libbpf_get_error(link)) { > fprintf(stderr, "failed to attach program!\n"); > exit(1); > } > -- Probably should be IS_ERR(link) same as the other benchs/*.c progs. ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] selftests/bpf: Fix return value check in attach_bpf() 2021-05-28 21:16 ` John Fastabend @ 2021-05-28 21:19 ` John Fastabend 0 siblings, 0 replies; 6+ messages in thread From: John Fastabend @ 2021-05-28 21:19 UTC (permalink / raw) To: John Fastabend, Yu Kuai, shuah, ast, daniel, andrii Cc: linux-kselftest, netdev, bpf, linux-kernel, yukuai3, yi.zhang John Fastabend wrote: > Yu Kuai wrote: > > use libbpf_get_error() to check the return value of > > bpf_program__attach(). > > > > Reported-by: Hulk Robot <hulkci@huawei.com> > > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > > --- > > tools/testing/selftests/bpf/benchs/bench_rename.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/testing/selftests/bpf/benchs/bench_rename.c b/tools/testing/selftests/bpf/benchs/bench_rename.c > > index c7ec114eca56..b7d4a1d74fca 100644 > > --- a/tools/testing/selftests/bpf/benchs/bench_rename.c > > +++ b/tools/testing/selftests/bpf/benchs/bench_rename.c > > @@ -65,7 +65,7 @@ static void attach_bpf(struct bpf_program *prog) > > struct bpf_link *link; > > > > link = bpf_program__attach(prog); > > - if (!link) { > > + if (libbpf_get_error(link)) { > > fprintf(stderr, "failed to attach program!\n"); > > exit(1); > > } > > -- > > Probably should be IS_ERR(link) same as the other benchs/*.c progs. Oops on wrong branch, agree with Daniel looks fine as !link otherwise need an explanation and fix the rest of the cases. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-30 1:17 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-28 9:07 [PATCH] selftests/bpf: Fix return value check in attach_bpf() Yu Kuai 2021-05-28 20:46 ` Daniel Borkmann 2021-05-29 1:25 ` yukuai (C) 2021-05-30 1:17 ` Andrii Nakryiko 2021-05-28 21:16 ` John Fastabend 2021-05-28 21:19 ` John Fastabend
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).