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!
Re: [PATCH bpf-next 0/6] Consistent prefixes for libbpf interfaces
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 >
Re: [PATCH bpf-next 0/6] Consistent prefixes for libbpf interfaces
[ +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. Thanks, Daniel
[PATCH bpf-next 0/6] Consistent prefixes for libbpf interfaces
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(-) -- 2.17.1