Message-ID: <9bf13cb9-c139-1706-d75e-e6e34d5136f6@fb.com>
Date:   Fri, 31 Mar 2017 20:18:56 -0700
From:   Alexei Starovoitov <ast@...com>
To:     "Wangnan (F)" <wangnan0@...wei.com>,
        "David S . Miller" <davem@...emloft.net>
CC:     Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>, <netdev@...r.kernel.org>,
        <kernel-team@...com>
Subject: Re: [PATCH v2 net-next 3/6] tools/lib/bpf: expose
 bpf_program__set_type()

On 3/31/17 7:29 PM, Wangnan (F) wrote:
>
>
> On 2017/3/31 12:45, Alexei Starovoitov wrote:
>> expose bpf_program__set_type() to set program type
>>
>> Signed-off-by: Alexei Starovoitov <ast@...nel.org>
>> Acked-by: Daniel Borkmann <daniel@...earbox.net>
>> Acked-by: Martin KaFai Lau <kafai@...com>
>> ---
>>   tools/lib/bpf/libbpf.c | 3 +--
>>   tools/lib/bpf/libbpf.h | 2 ++
>>   2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index ac6eb863b2a4..1a2c07eb7795 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -1618,8 +1618,7 @@ int bpf_program__nth_fd(struct bpf_program
>> *prog, int n)
>>       return fd;
>>   }
>>   -static void bpf_program__set_type(struct bpf_program *prog,
>> -                  enum bpf_prog_type type)
>> +void bpf_program__set_type(struct bpf_program *prog, enum
>> bpf_prog_type type)
>>   {
>>       prog->type = type;
>>   }
>
> Since it become a public interface, we need to check if prog is a
> NULL pointer and check if the value of type is okay, and let it
> return an errno.

I strongly disagree with such defensive programming. It's a cause
of bugs for users of this library.
I think you're trying to mimic c++ setters/getters, but c++
never checks 'this != null', since passing null into setter
is a bug of the user of the library and not the library.
The setters also should have 'void' return type when setters
cannot fail. That is exactly the case here.
If, in the future, we decide that this libbpf shouldn't support
all bpf program types then you'd need to change the prototype
of this function to return error code and change all places
where this function is called to check for error code.
It may or may not be the right approach.
For example today the only user of bpf_program__set*() methods
is perf/util/bpf-loader.c and it calls bpf_program__set_kprobe() and
bpf_program__set_tracepoint() _without_ checking the return value
which is _correct_ thing to do. Instead the current prototype of
'int bpf_program__set_tracepoint(struct bpf_program *prog);
is not correct and I suggest you to fix it.

You also need to do other cleanup. Like in bpf_object__elf_finish()
you have:
         if (!obj_elf_valid(obj))
                 return;

         if (obj->efile.elf) {

which is redundant. It's another example where mistakes creep in
due to defensive programming.

Another bug in bpf_object__close() which does:
         if (!obj)
                 return;
again defensive programming strikes, since
you're not checking IS_ERR(obj) and that's what bpf_object__open()
returns, so most users of the library (who don't read the source
code and just using it based on .h) will do

obj = bpf_object__open(...);
bpf_object__close(obj);

and current 'if (!obj)' won't help and it will segfault.
I hit this issue will developing this patch set.

>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>> index b30394f9947a..32c7252f734e 100644
>> --- a/tools/lib/bpf/libbpf.h
>> +++ b/tools/lib/bpf/libbpf.h
>> @@ -25,6 +25,7 @@
>>   #include <stdint.h>
>>   #include <stdbool.h>
>>   #include <sys/types.h>  // for size_t
>> +#include <linux/bpf.h>
>>     enum libbpf_errno {
>>       __LIBBPF_ERRNO__START = 4000,
>> @@ -185,6 +186,7 @@ int bpf_program__set_sched_cls(struct bpf_program
>> *prog);
>>   int bpf_program__set_sched_act(struct bpf_program *prog);
>>   int bpf_program__set_xdp(struct bpf_program *prog);
>>   int bpf_program__set_perf_event(struct bpf_program *prog);
>> +void bpf_program__set_type(struct bpf_program *prog, enum
>> bpf_prog_type type);
>>
>
> The above bpf_program__set_xxx become redundancy. It should be generated
> using macro as static inline functions.
>
>>   bool bpf_program__is_socket_filter(struct bpf_program *prog);
>>   bool bpf_program__is_tracepoint(struct bpf_program *prog);
>
> bpf_program__is_xxx should be updated like bpf_program__set_xxx, since
> enum bpf_prog_type is not a problem now.

All of these suggestions is a cleanup for your code that you
need to do yourself. I actually suggest you kill all bpf_program__is*()
and all but one bpf_program__set*() functions.
The current user perf/util/bpf-loader.c should be converted
to using bpf_program__set_type() with _void_ return code that
I'm introducing here.

Overall, I think, tools/lib/bpf/ is a nice library and it can be used
by many projects, but I suggest to stop making excuses based on
your proprietary usage of it.

Also please cc me in the future on changes to the library. It still
has my copyrights, though a lot has changed, since last time
I looked at it and it's my fault for not pay attention earlier.

Powered by blists - more mailing lists