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
>
