diff mbox series

selftests/bpf: Fix return value check in attach_bpf()

Message ID 20210528090758.1108464-1-yukuai3@huawei.com (mailing list archive)
State New
Headers show
Series selftests/bpf: Fix return value check in attach_bpf() | expand

Commit Message

Yu Kuai May 28, 2021, 9:07 a.m. UTC
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(-)

Comments

Daniel Borkmann May 28, 2021, 8:46 p.m. UTC | #1
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
John Fastabend May 28, 2021, 9:16 p.m. UTC | #2
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.
John Fastabend May 28, 2021, 9:19 p.m. UTC | #3
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.
Yu Kuai May 29, 2021, 1:25 a.m. UTC | #4
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
> .
>
Andrii Nakryiko May 30, 2021, 1:17 a.m. UTC | #5
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
> > .
> >
diff mbox series

Patch

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