Re: [PATCH bpf-next 0/6] Consistent prefixes for libbpf interfaces

[Yonghong Song <yhs@fb.com>]

On 10/4/18 7:22 AM, Daniel Borkmann wrote:
> [ +Yonghong ]
> 
> On 10/04/2018 12:26 AM, Andrey Ignatov wrote:
>> This patch set renames a few interfaces in libbpf, mostly netlink related,
>> so that all symbols provided by the library have only three possible
>> prefixes:
>>
>> % nm -D tools/lib/bpf/libbpf.so  | \
>>      awk '$2 == "T" {sub(/[_\(].*/, "", $3); if ($3) print $3}' | \
>>      sort | \
>>      uniq -c
>>       91 bpf
>>        8 btf
>>       14 libbpf
>>
>> libbpf is used more and more outside kernel tree. That means the library
>> should follow good practices in library design and implementation to
>> play well with third party code that uses it.
>>
>> One of such practices is to have a common prefix (or a few) for every
>> interface, function or data structure, library provides. It helps to
>> avoid name conflicts with other libraries and keeps API/ABI consistent.
>>
>> Inconsistent names in libbpf already cause problems in real life. E.g.
>> an application can't use both libbpf and libnl due to conflicting
>> symbols (specifically nla_parse, nla_parse_nested and a few others).
>>
>> Some of problematic global symbols are not part of ABI and can be
>> restricted from export with either visibility attribute/pragma or export
>> map (what is useful by itself and can be done in addition). That won't
>> solve the problem for those that are part of ABI though. Also export
>> restrictions would help only in DSO case. If third party application links
>> libbpf statically it won't help, and people do it (e.g. Facebook links
>> most of libraries statically, including libbpf).
>>
>> libbpf already uses the following prefixes for its interfaces:
>> * bpf_ for bpf system call wrappers, program/map/elf-object
>>    abstractions and a few other things;
>> * btf_ for BTF related API;
>> * libbpf_ for everything else.
>>
>> The patch adds libbpf_ prefix to interfaces that use none of mentioned
>> above prefixes and don't fit well into the first two categories.
>>
>> Long term benefits of having common prefix should outweigh possible
>> inconvenience of changing API for those functions now.
>>
>> Patches 2-4 add libbpf_ prefix to libbpf interfaces: separate patch per
>> header. Other patches are simple improvements in API.
>>
>>
>> Andrey Ignatov (6):
>>    libbpf: Move __dump_nlmsg_t from API to implementation
>>    libbpf: Consistent prefixes for interfaces in libbpf.h.
>>    libbpf: Consistent prefixes for interfaces in nlattr.h.
>>    libbpf: Consistent prefixes for interfaces in str_error.h.
>>    libbpf: Make include guards consistent
>>    libbpf: Use __u32 instead of u32 in bpf_program__load
>>
>>   tools/bpf/bpftool/net.c            | 41 ++++++++++---------
>>   tools/bpf/bpftool/netlink_dumper.c | 32 ++++++++-------
>>   tools/lib/bpf/bpf.h                |  6 +--
>>   tools/lib/bpf/btf.h                |  6 +--
>>   tools/lib/bpf/libbpf.c             | 22 +++++-----
>>   tools/lib/bpf/libbpf.h             | 31 +++++++-------
>>   tools/lib/bpf/netlink.c            | 48 ++++++++++++----------
>>   tools/lib/bpf/nlattr.c             | 64 +++++++++++++++--------------
>>   tools/lib/bpf/nlattr.h             | 65 +++++++++++++++---------------
>>   tools/lib/bpf/str_error.c          |  2 +-
>>   tools/lib/bpf/str_error.h          |  8 ++--
>>   11 files changed, 171 insertions(+), 154 deletions(-)
> 
> Overall agree that this is needed, and I've therefore applied the
> set, thanks for cleaning up, Andrey!
> 
> But, I would actually like to see this going one step further, in
> particular, we should keep all the netlink related stuff inside
> libbpf-/only/. Meaning, the goal of libbpf is not to provide yet
> another set of netlink primitives but instead e.g. for the bpftool
> dumper this should be abstracted away such that we pass in a callback
> from bpftool side and have an iterator object which will then be
> populated from inside the libbpf logic, meaning, bpftool shouldn't
> even be aware of anything netlink there.

Agreed. This indeed make sense, the user really only cares a few fields
like devname/id, attachment_type, prog_id, etc. I will take a look at
this later if nobody works on it.

> 
> Thanks,
> Daniel
>