Date:   Wed, 10 Oct 2018 00:17:59 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Yonghong Song <yhs@...com>, Andrey Ignatov <rdna@...com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Cc:     "ast@...nel.org" <ast@...nel.org>, Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next 0/6] Consistent prefixes for libbpf interfaces

On 10/09/2018 08:43 AM, Yonghong Song wrote:
> 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.

Awesome, that would be great, thanks!

Powered by blists - more mailing lists