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!