Consistent prefixes for libbpf interfaces

PATCH In review Andrey Ignatov 3 participants 10 messages Oct 4, 2018 – Oct 10, 2018

Subsystems (5)

Networking Network drivers BPF libbpf +1 more subsystems

lore

Share & tools

lore

Andrey Ignatov

author

2018-10-04 05:17 UTC #0 lore raw

cover 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(-)

Signature · 1 line

2.17.1

Andrey Ignatov

author

2018-10-04 05:17 UTC #1 lore raw

PATCH 1/6 libbpf: Move __dump_nlmsg_t from API to implementation

This typedef is used only by implementation in netlink.c. Nothing uses it in public API. Move it to netlink.c.

Signed-off-by Andrey Ignatov <[email protected]>

Acked-by Alexei Starovoitov <[email protected]>

---
 tools/lib/bpf/libbpf.h  | 3 ---
 tools/lib/bpf/netlink.c | 3 +++
 2 files changed, 3 insertions(+), 3 deletions(-)

tools/lib/bpf/libbpf.h +0 -3

index 2ed24d3f80b3..8388be525388 100644
@@ -304,11 +304,8 @@ int bpf_perf_event_read_simple(void *mem, unsigned long size,
 			       void **buf, size_t *buf_len,
 			       bpf_perf_event_print_t fn, void *priv);
 
-struct nlmsghdr;
 struct nlattr;
 typedef int (*dump_nlmsg_t)(void *cookie, void *msg, struct nlattr **tb);
-typedef int (*__dump_nlmsg_t)(struct nlmsghdr *nlmsg, dump_nlmsg_t,
-			      void *cookie);
 int bpf_netlink_open(unsigned int *nl_pid);
 int nl_get_link(int sock, unsigned int nl_pid, dump_nlmsg_t dump_link_nlmsg,
 		void *cookie);

tools/lib/bpf/netlink.c +3 -0

index fde1d7bf8199..da46d9358d9d 100644
@@ -18,6 +18,9 @@
 #define SOL_NETLINK 270
 #endif
 
+typedef int (*__dump_nlmsg_t)(struct nlmsghdr *nlmsg, dump_nlmsg_t,
+			      void *cookie);
+
 int bpf_netlink_open(__u32 *nl_pid)
 {
 	struct sockaddr_nl sa;

Signature · 1 line

2.17.1

Andrey Ignatov

author

2018-10-04 05:17 UTC #2 lore raw

PATCH 2/6 libbpf: Consistent prefixes for interfaces in libbpf.h.

In reply to Andrey Ignatov

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. I helps to avoid name conflicts with other libraries and keeps API 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.

Having common prefix will help to fix current and avoid future problems.

The patch adds libbpf_ prefix to functions and typedef in libbpf.h that use none of mentioned above prefixes and doesn't fit well into the first two categories.

Since affected part of API is used in bpftool, the patch applies corresponding change to bpftool as well. Having it in a separate patch will cause a state of tree where bpftool is broken what may not be a good idea.

Signed-off-by Andrey Ignatov <[email protected]>

Acked-by Alexei Starovoitov <[email protected]>

---
 tools/bpf/bpftool/net.c | 31 +++++++++++++++----------------
 tools/lib/bpf/libbpf.h  | 20 ++++++++++----------
 tools/lib/bpf/netlink.c | 37 ++++++++++++++++++++-----------------
 3 files changed, 45 insertions(+), 43 deletions(-)

tools/bpf/bpftool/net.c +15 ~~-16~~

index ed205ee57655..ef83e8a08490 100644
@@ -127,14 +127,14 @@ static int show_dev_tc_bpf(int sock, unsigned int nl_pid,
 	tcinfo.array_len = 0;
 
 	tcinfo.is_qdisc = false;
-	ret = nl_get_class(sock, nl_pid, dev->ifindex, dump_class_qdisc_nlmsg,
-			   &tcinfo);
+	ret = libbpf_nl_get_class(sock, nl_pid, dev->ifindex,
+				  dump_class_qdisc_nlmsg, &tcinfo);
 	if (ret)
 		goto out;
 
 	tcinfo.is_qdisc = true;
-	ret = nl_get_qdisc(sock, nl_pid, dev->ifindex, dump_class_qdisc_nlmsg,
-			   &tcinfo);
+	ret = libbpf_nl_get_qdisc(sock, nl_pid, dev->ifindex,
+				  dump_class_qdisc_nlmsg, &tcinfo);
 	if (ret)
 		goto out;

@@ -142,10 +142,9 @@ static int show_dev_tc_bpf(int sock, unsigned int nl_pid,
 	filter_info.ifindex = dev->ifindex;
 	for (i = 0; i < tcinfo.used_len; i++) {
 		filter_info.kind = tcinfo.handle_array[i].kind;
-		ret = nl_get_filter(sock, nl_pid, dev->ifindex,
-				    tcinfo.handle_array[i].handle,
-				    dump_filter_nlmsg,
-				    &filter_info);
+		ret = libbpf_nl_get_filter(sock, nl_pid, dev->ifindex,
+					   tcinfo.handle_array[i].handle,
+					   dump_filter_nlmsg, &filter_info);
 		if (ret)
 			goto out;
 	}

@@ -153,22 +152,22 @@ static int show_dev_tc_bpf(int sock, unsigned int nl_pid,
 	/* root, ingress and egress handle */
 	handle = TC_H_ROOT;
 	filter_info.kind = "root";
-	ret = nl_get_filter(sock, nl_pid, dev->ifindex, handle,
-			    dump_filter_nlmsg, &filter_info);
+	ret = libbpf_nl_get_filter(sock, nl_pid, dev->ifindex, handle,
+				   dump_filter_nlmsg, &filter_info);
 	if (ret)
 		goto out;
 
 	handle = TC_H_MAKE(TC_H_CLSACT, TC_H_MIN_INGRESS);
 	filter_info.kind = "clsact/ingress";
-	ret = nl_get_filter(sock, nl_pid, dev->ifindex, handle,
-			    dump_filter_nlmsg, &filter_info);
+	ret = libbpf_nl_get_filter(sock, nl_pid, dev->ifindex, handle,
+				   dump_filter_nlmsg, &filter_info);
 	if (ret)
 		goto out;
 
 	handle = TC_H_MAKE(TC_H_CLSACT, TC_H_MIN_EGRESS);
 	filter_info.kind = "clsact/egress";
-	ret = nl_get_filter(sock, nl_pid, dev->ifindex, handle,
-			    dump_filter_nlmsg, &filter_info);
+	ret = libbpf_nl_get_filter(sock, nl_pid, dev->ifindex, handle,
+				   dump_filter_nlmsg, &filter_info);
 	if (ret)
 		goto out;

@@ -196,7 +195,7 @@ static int do_show(int argc, char **argv)
 		usage();
 	}
 
-	sock = bpf_netlink_open(&nl_pid);
+	sock = libbpf_netlink_open(&nl_pid);
 	if (sock < 0) {
 		fprintf(stderr, "failed to open netlink sock\n");
 		return -1;

@@ -211,7 +210,7 @@ static int do_show(int argc, char **argv)
 		jsonw_start_array(json_wtr);
 	NET_START_OBJECT;
 	NET_START_ARRAY("xdp", "%s:\n");
-	ret = nl_get_link(sock, nl_pid, dump_link_nlmsg, &dev_array);
+	ret = libbpf_nl_get_link(sock, nl_pid, dump_link_nlmsg, &dev_array);
 	NET_END_ARRAY("\n");
 
 	if (!ret) {

tools/lib/bpf/libbpf.h +10 ~~-10~~

index 8388be525388..710ff5724980 100644
@@ -305,14 +305,14 @@ int bpf_perf_event_read_simple(void *mem, unsigned long size,
 			       bpf_perf_event_print_t fn, void *priv);
 
 struct nlattr;
-typedef int (*dump_nlmsg_t)(void *cookie, void *msg, struct nlattr **tb);
-int bpf_netlink_open(unsigned int *nl_pid);
-int nl_get_link(int sock, unsigned int nl_pid, dump_nlmsg_t dump_link_nlmsg,
-		void *cookie);
-int nl_get_class(int sock, unsigned int nl_pid, int ifindex,
-		 dump_nlmsg_t dump_class_nlmsg, void *cookie);
-int nl_get_qdisc(int sock, unsigned int nl_pid, int ifindex,
-		 dump_nlmsg_t dump_qdisc_nlmsg, void *cookie);
-int nl_get_filter(int sock, unsigned int nl_pid, int ifindex, int handle,
-		  dump_nlmsg_t dump_filter_nlmsg, void *cookie);
+typedef int (*libbpf_dump_nlmsg_t)(void *cookie, void *msg, struct nlattr **tb);
+int libbpf_netlink_open(unsigned int *nl_pid);
+int libbpf_nl_get_link(int sock, unsigned int nl_pid,
+		       libbpf_dump_nlmsg_t dump_link_nlmsg, void *cookie);
+int libbpf_nl_get_class(int sock, unsigned int nl_pid, int ifindex,
+			libbpf_dump_nlmsg_t dump_class_nlmsg, void *cookie);
+int libbpf_nl_get_qdisc(int sock, unsigned int nl_pid, int ifindex,
+			libbpf_dump_nlmsg_t dump_qdisc_nlmsg, void *cookie);
+int libbpf_nl_get_filter(int sock, unsigned int nl_pid, int ifindex, int handle,
+			 libbpf_dump_nlmsg_t dump_filter_nlmsg, void *cookie);
 #endif

tools/lib/bpf/netlink.c +20 ~~-17~~

index da46d9358d9d..506bdfdbcab0 100644
@@ -18,10 +18,10 @@
 #define SOL_NETLINK 270
 #endif
 
-typedef int (*__dump_nlmsg_t)(struct nlmsghdr *nlmsg, dump_nlmsg_t,
+typedef int (*__dump_nlmsg_t)(struct nlmsghdr *nlmsg, libbpf_dump_nlmsg_t,
 			      void *cookie);
 
-int bpf_netlink_open(__u32 *nl_pid)
+int libbpf_netlink_open(__u32 *nl_pid)
 {
 	struct sockaddr_nl sa;
 	socklen_t addrlen;

@@ -65,7 +65,7 @@ int bpf_netlink_open(__u32 *nl_pid)
 }
 
 static int bpf_netlink_recv(int sock, __u32 nl_pid, int seq,
-			    __dump_nlmsg_t _fn, dump_nlmsg_t fn,
+			    __dump_nlmsg_t _fn, libbpf_dump_nlmsg_t fn,
 			    void *cookie)
 {
 	bool multipart = true;

@@ -133,7 +133,7 @@ int bpf_set_link_xdp_fd(int ifindex, int fd, __u32 flags)
 	} req;
 	__u32 nl_pid;
 
-	sock = bpf_netlink_open(&nl_pid);
+	sock = libbpf_netlink_open(&nl_pid);
 	if (sock < 0)
 		return sock;

@@ -181,8 +181,8 @@ int bpf_set_link_xdp_fd(int ifindex, int fd, __u32 flags)
 	return ret;
 }
 
-static int __dump_link_nlmsg(struct nlmsghdr *nlh, dump_nlmsg_t dump_link_nlmsg,
-			     void *cookie)
+static int __dump_link_nlmsg(struct nlmsghdr *nlh,
+			     libbpf_dump_nlmsg_t dump_link_nlmsg, void *cookie)
 {
 	struct nlattr *tb[IFLA_MAX + 1], *attr;
 	struct ifinfomsg *ifi = NLMSG_DATA(nlh);

@@ -196,8 +196,8 @@ static int __dump_link_nlmsg(struct nlmsghdr *nlh, dump_nlmsg_t dump_link_nlmsg,
 	return dump_link_nlmsg(cookie, ifi, tb);
 }
 
-int nl_get_link(int sock, unsigned int nl_pid, dump_nlmsg_t dump_link_nlmsg,
-		void *cookie)
+int libbpf_nl_get_link(int sock, unsigned int nl_pid,
+		       libbpf_dump_nlmsg_t dump_link_nlmsg, void *cookie)
 {
 	struct {
 		struct nlmsghdr nlh;

@@ -219,7 +219,8 @@ int nl_get_link(int sock, unsigned int nl_pid, dump_nlmsg_t dump_link_nlmsg,
 }
 
 static int __dump_class_nlmsg(struct nlmsghdr *nlh,
-			      dump_nlmsg_t dump_class_nlmsg, void *cookie)
+			      libbpf_dump_nlmsg_t dump_class_nlmsg,
+			      void *cookie)
 {
 	struct nlattr *tb[TCA_MAX + 1], *attr;
 	struct tcmsg *t = NLMSG_DATA(nlh);

@@ -233,8 +234,8 @@ static int __dump_class_nlmsg(struct nlmsghdr *nlh,
 	return dump_class_nlmsg(cookie, t, tb);
 }
 
-int nl_get_class(int sock, unsigned int nl_pid, int ifindex,
-		 dump_nlmsg_t dump_class_nlmsg, void *cookie)
+int libbpf_nl_get_class(int sock, unsigned int nl_pid, int ifindex,
+			libbpf_dump_nlmsg_t dump_class_nlmsg, void *cookie)
 {
 	struct {
 		struct nlmsghdr nlh;

@@ -257,7 +258,8 @@ int nl_get_class(int sock, unsigned int nl_pid, int ifindex,
 }
 
 static int __dump_qdisc_nlmsg(struct nlmsghdr *nlh,
-			      dump_nlmsg_t dump_qdisc_nlmsg, void *cookie)
+			      libbpf_dump_nlmsg_t dump_qdisc_nlmsg,
+			      void *cookie)
 {
 	struct nlattr *tb[TCA_MAX + 1], *attr;
 	struct tcmsg *t = NLMSG_DATA(nlh);

@@ -271,8 +273,8 @@ static int __dump_qdisc_nlmsg(struct nlmsghdr *nlh,
 	return dump_qdisc_nlmsg(cookie, t, tb);
 }
 
-int nl_get_qdisc(int sock, unsigned int nl_pid, int ifindex,
-		 dump_nlmsg_t dump_qdisc_nlmsg, void *cookie)
+int libbpf_nl_get_qdisc(int sock, unsigned int nl_pid, int ifindex,
+			libbpf_dump_nlmsg_t dump_qdisc_nlmsg, void *cookie)
 {
 	struct {
 		struct nlmsghdr nlh;

@@ -295,7 +297,8 @@ int nl_get_qdisc(int sock, unsigned int nl_pid, int ifindex,
 }
 
 static int __dump_filter_nlmsg(struct nlmsghdr *nlh,
-			       dump_nlmsg_t dump_filter_nlmsg, void *cookie)
+			       libbpf_dump_nlmsg_t dump_filter_nlmsg,
+			       void *cookie)
 {
 	struct nlattr *tb[TCA_MAX + 1], *attr;
 	struct tcmsg *t = NLMSG_DATA(nlh);

@@ -309,8 +312,8 @@ static int __dump_filter_nlmsg(struct nlmsghdr *nlh,
 	return dump_filter_nlmsg(cookie, t, tb);
 }
 
-int nl_get_filter(int sock, unsigned int nl_pid, int ifindex, int handle,
-		  dump_nlmsg_t dump_filter_nlmsg, void *cookie)
+int libbpf_nl_get_filter(int sock, unsigned int nl_pid, int ifindex, int handle,
+			 libbpf_dump_nlmsg_t dump_filter_nlmsg, void *cookie)
 {
 	struct {
 		struct nlmsghdr nlh;

Signature · 1 line

2.17.1

Andrey Ignatov

author

2018-10-04 05:17 UTC #3 lore raw

PATCH 3/6 libbpf: Consistent prefixes for interfaces in nlattr.h.

In reply to Andrey Ignatov

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.

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.

Having common prefix will help to fix current and avoid future problems.

The patch adds libbpf_ prefix to interfaces in nlattr.h that use none of mentioned above prefixes and doesn't fit well into the first two categories.

Signed-off-by Andrey Ignatov <[email protected]>

Acked-by Alexei Starovoitov <[email protected]>

---
 tools/bpf/bpftool/net.c            | 10 +++--
 tools/bpf/bpftool/netlink_dumper.c | 32 ++++++++-------
 tools/lib/bpf/netlink.c            | 10 ++---
 tools/lib/bpf/nlattr.c             | 64 ++++++++++++++++--------------
 tools/lib/bpf/nlattr.h             | 59 +++++++++++++--------------
 5 files changed, 94 insertions(+), 81 deletions(-)

tools/bpf/bpftool/net.c +7 -3

index ef83e8a08490..d441bb7035ca 100644
@@ -69,7 +69,9 @@ static int dump_link_nlmsg(void *cookie, void *msg, struct nlattr **tb)
 	snprintf(netinfo->devices[netinfo->used_len].devname,
 		 sizeof(netinfo->devices[netinfo->used_len].devname),
 		 "%s",
-		 tb[IFLA_IFNAME] ? nla_getattr_str(tb[IFLA_IFNAME]) : "");
+		 tb[IFLA_IFNAME]
+			 ? libbpf_nla_getattr_str(tb[IFLA_IFNAME])
+			 : "");
 	netinfo->used_len++;
 
 	return do_xdp_dump(ifinfo, tb);

@@ -83,7 +85,7 @@ static int dump_class_qdisc_nlmsg(void *cookie, void *msg, struct nlattr **tb)
 	if (tcinfo->is_qdisc) {
 		/* skip clsact qdisc */
 		if (tb[TCA_KIND] &&
-		    strcmp(nla_data(tb[TCA_KIND]), "clsact") == 0)
+		    strcmp(libbpf_nla_data(tb[TCA_KIND]), "clsact") == 0)
 			return 0;
 		if (info->tcm_handle == 0)
 			return 0;

@@ -101,7 +103,9 @@ static int dump_class_qdisc_nlmsg(void *cookie, void *msg, struct nlattr **tb)
 	snprintf(tcinfo->handle_array[tcinfo->used_len].kind,
 		 sizeof(tcinfo->handle_array[tcinfo->used_len].kind),
 		 "%s",
-		 tb[TCA_KIND] ? nla_getattr_str(tb[TCA_KIND]) : "unknown");
+		 tb[TCA_KIND]
+			 ? libbpf_nla_getattr_str(tb[TCA_KIND])
+			 : "unknown");
 	tcinfo->used_len++;
 
 	return 0;

tools/bpf/bpftool/netlink_dumper.c +18 ~~-14~~

index 6f5e9cc6836c..4e9f4531269f 100644
@@ -21,7 +21,7 @@ static void xdp_dump_prog_id(struct nlattr **tb, int attr,
 	if (new_json_object)
 		NET_START_OBJECT
 	NET_DUMP_STR("mode", " %s", mode);
-	NET_DUMP_UINT("id", " id %u", nla_getattr_u32(tb[attr]))
+	NET_DUMP_UINT("id", " id %u", libbpf_nla_getattr_u32(tb[attr]))
 	if (new_json_object)
 		NET_END_OBJECT
 }

@@ -32,13 +32,13 @@ static int do_xdp_dump_one(struct nlattr *attr, unsigned int ifindex,
 	struct nlattr *tb[IFLA_XDP_MAX + 1];
 	unsigned char mode;
 
-	if (nla_parse_nested(tb, IFLA_XDP_MAX, attr, NULL) < 0)
+	if (libbpf_nla_parse_nested(tb, IFLA_XDP_MAX, attr, NULL) < 0)
 		return -1;
 
 	if (!tb[IFLA_XDP_ATTACHED])
 		return 0;
 
-	mode = nla_getattr_u8(tb[IFLA_XDP_ATTACHED]);
+	mode = libbpf_nla_getattr_u8(tb[IFLA_XDP_ATTACHED]);
 	if (mode == XDP_ATTACHED_NONE)
 		return 0;

@@ -75,14 +75,14 @@ int do_xdp_dump(struct ifinfomsg *ifinfo, struct nlattr **tb)
 		return 0;
 
 	return do_xdp_dump_one(tb[IFLA_XDP], ifinfo->ifi_index,
-			       nla_getattr_str(tb[IFLA_IFNAME]));
+			       libbpf_nla_getattr_str(tb[IFLA_IFNAME]));
 }
 
 static int do_bpf_dump_one_act(struct nlattr *attr)
 {
 	struct nlattr *tb[TCA_ACT_BPF_MAX + 1];
 
-	if (nla_parse_nested(tb, TCA_ACT_BPF_MAX, attr, NULL) < 0)
+	if (libbpf_nla_parse_nested(tb, TCA_ACT_BPF_MAX, attr, NULL) < 0)
 		return -LIBBPF_ERRNO__NLPARSE;
 
 	if (!tb[TCA_ACT_BPF_PARMS])

@@ -91,10 +91,10 @@ static int do_bpf_dump_one_act(struct nlattr *attr)
 	NET_START_OBJECT_NESTED2;
 	if (tb[TCA_ACT_BPF_NAME])
 		NET_DUMP_STR("name", "%s",
-			     nla_getattr_str(tb[TCA_ACT_BPF_NAME]));
+			     libbpf_nla_getattr_str(tb[TCA_ACT_BPF_NAME]));
 	if (tb[TCA_ACT_BPF_ID])
 		NET_DUMP_UINT("id", " id %u",
-			      nla_getattr_u32(tb[TCA_ACT_BPF_ID]));
+			      libbpf_nla_getattr_u32(tb[TCA_ACT_BPF_ID]));
 	NET_END_OBJECT_NESTED;
 	return 0;
 }

@@ -106,10 +106,11 @@ static int do_dump_one_act(struct nlattr *attr)
 	if (!attr)
 		return 0;
 
-	if (nla_parse_nested(tb, TCA_ACT_MAX, attr, NULL) < 0)
+	if (libbpf_nla_parse_nested(tb, TCA_ACT_MAX, attr, NULL) < 0)
 		return -LIBBPF_ERRNO__NLPARSE;
 
-	if (tb[TCA_ACT_KIND] && strcmp(nla_data(tb[TCA_ACT_KIND]), "bpf") == 0)
+	if (tb[TCA_ACT_KIND] &&
+	    strcmp(libbpf_nla_data(tb[TCA_ACT_KIND]), "bpf") == 0)
 		return do_bpf_dump_one_act(tb[TCA_ACT_OPTIONS]);
 
 	return 0;

@@ -120,7 +121,7 @@ static int do_bpf_act_dump(struct nlattr *attr)
 	struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
 	int act, ret;
 
-	if (nla_parse_nested(tb, TCA_ACT_MAX_PRIO, attr, NULL) < 0)
+	if (libbpf_nla_parse_nested(tb, TCA_ACT_MAX_PRIO, attr, NULL) < 0)
 		return -LIBBPF_ERRNO__NLPARSE;
 
 	NET_START_ARRAY("act", " %s [");

@@ -139,13 +140,15 @@ static int do_bpf_filter_dump(struct nlattr *attr)
 	struct nlattr *tb[TCA_BPF_MAX + 1];
 	int ret;
 
-	if (nla_parse_nested(tb, TCA_BPF_MAX, attr, NULL) < 0)
+	if (libbpf_nla_parse_nested(tb, TCA_BPF_MAX, attr, NULL) < 0)
 		return -LIBBPF_ERRNO__NLPARSE;
 
 	if (tb[TCA_BPF_NAME])
-		NET_DUMP_STR("name", " %s", nla_getattr_str(tb[TCA_BPF_NAME]));
+		NET_DUMP_STR("name", " %s",
+			     libbpf_nla_getattr_str(tb[TCA_BPF_NAME]));
 	if (tb[TCA_BPF_ID])
-		NET_DUMP_UINT("id", " id %u", nla_getattr_u32(tb[TCA_BPF_ID]));
+		NET_DUMP_UINT("id", " id %u",
+			      libbpf_nla_getattr_u32(tb[TCA_BPF_ID]));
 	if (tb[TCA_BPF_ACT]) {
 		ret = do_bpf_act_dump(tb[TCA_BPF_ACT]);
 		if (ret)

@@ -160,7 +163,8 @@ int do_filter_dump(struct tcmsg *info, struct nlattr **tb, const char *kind,
 {
 	int ret = 0;
 
-	if (tb[TCA_OPTIONS] && strcmp(nla_data(tb[TCA_KIND]), "bpf") == 0) {
+	if (tb[TCA_OPTIONS] &&
+	    strcmp(libbpf_nla_data(tb[TCA_KIND]), "bpf") == 0) {
 		NET_START_OBJECT;
 		if (devname[0] != '\0')
 			NET_DUMP_STR("devname", "%s", devname);

tools/lib/bpf/netlink.c +5 -5

index 506bdfdbcab0..2d2edbbd8ae8 100644
@@ -103,7 +103,7 @@ static int bpf_netlink_recv(int sock, __u32 nl_pid, int seq,
 				if (!err->error)
 					continue;
 				ret = err->error;
-				nla_dump_errormsg(nh);
+				libbpf_nla_dump_errormsg(nh);
 				goto done;
 			case NLMSG_DONE:
 				return 0;

@@ -190,7 +190,7 @@ static int __dump_link_nlmsg(struct nlmsghdr *nlh,
 
 	len = nlh->nlmsg_len - NLMSG_LENGTH(sizeof(*ifi));
 	attr = (struct nlattr *) ((void *) ifi + NLMSG_ALIGN(sizeof(*ifi)));
-	if (nla_parse(tb, IFLA_MAX, attr, len, NULL) != 0)
+	if (libbpf_nla_parse(tb, IFLA_MAX, attr, len, NULL) != 0)
 		return -LIBBPF_ERRNO__NLPARSE;
 
 	return dump_link_nlmsg(cookie, ifi, tb);

@@ -228,7 +228,7 @@ static int __dump_class_nlmsg(struct nlmsghdr *nlh,
 
 	len = nlh->nlmsg_len - NLMSG_LENGTH(sizeof(*t));
 	attr = (struct nlattr *) ((void *) t + NLMSG_ALIGN(sizeof(*t)));
-	if (nla_parse(tb, TCA_MAX, attr, len, NULL) != 0)
+	if (libbpf_nla_parse(tb, TCA_MAX, attr, len, NULL) != 0)
 		return -LIBBPF_ERRNO__NLPARSE;
 
 	return dump_class_nlmsg(cookie, t, tb);

@@ -267,7 +267,7 @@ static int __dump_qdisc_nlmsg(struct nlmsghdr *nlh,
 
 	len = nlh->nlmsg_len - NLMSG_LENGTH(sizeof(*t));
 	attr = (struct nlattr *) ((void *) t + NLMSG_ALIGN(sizeof(*t)));
-	if (nla_parse(tb, TCA_MAX, attr, len, NULL) != 0)
+	if (libbpf_nla_parse(tb, TCA_MAX, attr, len, NULL) != 0)
 		return -LIBBPF_ERRNO__NLPARSE;
 
 	return dump_qdisc_nlmsg(cookie, t, tb);

@@ -306,7 +306,7 @@ static int __dump_filter_nlmsg(struct nlmsghdr *nlh,
 
 	len = nlh->nlmsg_len - NLMSG_LENGTH(sizeof(*t));
 	attr = (struct nlattr *) ((void *) t + NLMSG_ALIGN(sizeof(*t)));
-	if (nla_parse(tb, TCA_MAX, attr, len, NULL) != 0)
+	if (libbpf_nla_parse(tb, TCA_MAX, attr, len, NULL) != 0)
 		return -LIBBPF_ERRNO__NLPARSE;
 
 	return dump_filter_nlmsg(cookie, t, tb);

tools/lib/bpf/nlattr.c +34 ~~-30~~

131 lines not on this page · raw message

tools/lib/bpf/nlattr.h +30 ~~-29~~

106 lines not on this page · raw message

Signature · 1 line

2.17.1

Andrey Ignatov

author

2018-10-04 05:17 UTC #4 lore raw

PATCH 4/6 libbpf: Consistent prefixes for interfaces in str_error.h.

In reply to Andrey Ignatov

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.

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.

Having common prefix will help to fix current and avoid future problems.

The patch renames function in str_error.h to have libbpf_ prefix since it misses one and doesn't fit well into the first two categories.

Signed-off-by Andrey Ignatov <[email protected]>

Acked-by Alexei Starovoitov <[email protected]>

---
 tools/lib/bpf/libbpf.c    | 20 +++++++++++---------
 tools/lib/bpf/str_error.c |  2 +-
 tools/lib/bpf/str_error.h |  2 +-
 3 files changed, 13 insertions(+), 11 deletions(-)

tools/lib/bpf/libbpf.c +11 -9

index 9e68fd9fcfca..02888d36b805 100644
@@ -470,7 +470,8 @@ static int bpf_object__elf_init(struct bpf_object *obj)
 		obj->efile.fd = open(obj->path, O_RDONLY);
 		if (obj->efile.fd < 0) {
 			char errmsg[STRERR_BUFSIZE];
-			char *cp = str_error(errno, errmsg, sizeof(errmsg));
+			char *cp = libbpf_strerror_r(errno, errmsg,
+						     sizeof(errmsg));
 
 			pr_warning("failed to open %s: %s\n", obj->path, cp);
 			return -errno;

@@ -811,7 +812,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
 						      data->d_size, name, idx);
 			if (err) {
 				char errmsg[STRERR_BUFSIZE];
-				char *cp = str_error(-err, errmsg, sizeof(errmsg));
+				char *cp = libbpf_strerror_r(-err, errmsg,
+							     sizeof(errmsg));
 
 				pr_warning("failed to alloc program %s (%s): %s",
 					   name, obj->path, cp);

@@ -1140,7 +1142,7 @@ bpf_object__create_maps(struct bpf_object *obj)
 
 		*pfd = bpf_create_map_xattr(&create_attr);
 		if (*pfd < 0 && create_attr.btf_key_type_id) {
-			cp = str_error(errno, errmsg, sizeof(errmsg));
+			cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
 			pr_warning("Error in bpf_create_map_xattr(%s):%s(%d). Retrying without BTF.\n",
 				   map->name, cp, errno);
 			create_attr.btf_fd = 0;

@@ -1155,7 +1157,7 @@ bpf_object__create_maps(struct bpf_object *obj)
 			size_t j;
 
 			err = *pfd;
-			cp = str_error(errno, errmsg, sizeof(errmsg));
+			cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
 			pr_warning("failed to create map (name: '%s'): %s\n",
 				   map->name, cp);
 			for (j = 0; j < i; j++)

@@ -1339,7 +1341,7 @@ load_program(enum bpf_prog_type type, enum bpf_attach_type expected_attach_type,
 	}
 
 	ret = -LIBBPF_ERRNO__LOAD;
-	cp = str_error(errno, errmsg, sizeof(errmsg));
+	cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
 	pr_warning("load bpf program failed: %s\n", cp);
 
 	if (log_buf && log_buf[0] != '\0') {

@@ -1655,7 +1657,7 @@ static int check_path(const char *path)
 
 	dir = dirname(dname);
 	if (statfs(dir, &st_fs)) {
-		cp = str_error(errno, errmsg, sizeof(errmsg));
+		cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
 		pr_warning("failed to statfs %s: %s\n", dir, cp);
 		err = -errno;
 	}

@@ -1691,7 +1693,7 @@ int bpf_program__pin_instance(struct bpf_program *prog, const char *path,
 	}
 
 	if (bpf_obj_pin(prog->instances.fds[instance], path)) {
-		cp = str_error(errno, errmsg, sizeof(errmsg));
+		cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
 		pr_warning("failed to pin program: %s\n", cp);
 		return -errno;
 	}

@@ -1709,7 +1711,7 @@ static int make_dir(const char *path)
 		err = -errno;
 
 	if (err) {
-		cp = str_error(-err, errmsg, sizeof(errmsg));
+		cp = libbpf_strerror_r(-err, errmsg, sizeof(errmsg));
 		pr_warning("failed to mkdir %s: %s\n", path, cp);
 	}
 	return err;

@@ -1771,7 +1773,7 @@ int bpf_map__pin(struct bpf_map *map, const char *path)
 	}
 
 	if (bpf_obj_pin(map->fd, path)) {
-		cp = str_error(errno, errmsg, sizeof(errmsg));
+		cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
 		pr_warning("failed to pin map: %s\n", cp);
 		return -errno;
 	}

tools/lib/bpf/str_error.c +1 -1

index b8798114a357..3d211b642cb5 100644
@@ -9,7 +9,7 @@
  * libc, while checking strerror_r() return to avoid having to check this in
  * all places calling it.
  */
-char *str_error(int err, char *dst, int len)
+char *libbpf_strerror_r(int err, char *dst, int len)
 {
 	int ret = strerror_r(err, dst, len);
 	if (ret)

tools/lib/bpf/str_error.h +1 -1

index 355b1db571d1..998eff7d6710 100644
@@ -2,5 +2,5 @@
 #ifndef BPF_STR_ERROR
 #define BPF_STR_ERROR
 
-char *str_error(int err, char *dst, int len);
+char *libbpf_strerror_r(int err, char *dst, int len);
 #endif // BPF_STR_ERROR

Signature · 1 line

2.17.1

Andrey Ignatov

author

2018-10-04 05:17 UTC #5 lore raw

PATCH 5/6 libbpf: Make include guards consistent

In reply to Andrey Ignatov

Rename include guards to have consistent names "__LIBBPF_<header_name>".

Signed-off-by Andrey Ignatov <[email protected]>

Acked-by Alexei Starovoitov <[email protected]>

---
 tools/lib/bpf/bpf.h       | 6 +++---
 tools/lib/bpf/btf.h       | 6 +++---
 tools/lib/bpf/libbpf.h    | 6 +++---
 tools/lib/bpf/nlattr.h    | 6 +++---
 tools/lib/bpf/str_error.h | 6 +++---
 5 files changed, 15 insertions(+), 15 deletions(-)

tools/lib/bpf/bpf.h +3 -3

index 6f38164b2618..4c78f61b7c71 100644
@@ -20,8 +20,8 @@
  * You should have received a copy of the GNU Lesser General Public
  * License along with this program; if not,  see <http://www.gnu.org/licenses>
  */
-#ifndef __BPF_BPF_H
-#define __BPF_BPF_H
+#ifndef __LIBBPF_BPF_H
+#define __LIBBPF_BPF_H
 
 #include <linux/bpf.h>
 #include <stdbool.h>

@@ -111,4 +111,4 @@ int bpf_load_btf(void *btf, __u32 btf_size, char *log_buf, __u32 log_buf_size,
 int bpf_task_fd_query(int pid, int fd, __u32 flags, char *buf, __u32 *buf_len,
 		      __u32 *prog_id, __u32 *fd_type, __u64 *probe_offset,
 		      __u64 *probe_addr);
-#endif
+#endif /* __LIBBPF_BPF_H */

tools/lib/bpf/btf.h +3 -3

index 4897e0724d4e..d5d20682eeb6 100644
@@ -1,8 +1,8 @@
 /* SPDX-License-Identifier: LGPL-2.1 */
 /* Copyright (c) 2018 Facebook */
 
-#ifndef __BPF_BTF_H
-#define __BPF_BTF_H
+#ifndef __LIBBPF_BTF_H
+#define __LIBBPF_BTF_H
 
 #include <linux/types.h>

@@ -23,4 +23,4 @@ int btf__resolve_type(const struct btf *btf, __u32 type_id);
 int btf__fd(const struct btf *btf);
 const char *btf__name_by_offset(const struct btf *btf, __u32 offset);
 
-#endif
+#endif /* __LIBBPF_BTF_H */

tools/lib/bpf/libbpf.h +3 -3

index 710ff5724980..28f83dd6022b 100644
@@ -20,8 +20,8 @@
  * You should have received a copy of the GNU Lesser General Public
  * License along with this program; if not,  see <http://www.gnu.org/licenses>
  */
-#ifndef __BPF_LIBBPF_H
-#define __BPF_LIBBPF_H
+#ifndef __LIBBPF_LIBBPF_H
+#define __LIBBPF_LIBBPF_H
 
 #include <stdio.h>
 #include <stdint.h>

@@ -315,4 +315,4 @@ int libbpf_nl_get_qdisc(int sock, unsigned int nl_pid, int ifindex,
 			libbpf_dump_nlmsg_t dump_qdisc_nlmsg, void *cookie);
 int libbpf_nl_get_filter(int sock, unsigned int nl_pid, int ifindex, int handle,
 			 libbpf_dump_nlmsg_t dump_filter_nlmsg, void *cookie);
-#endif
+#endif /* __LIBBPF_LIBBPF_H */

tools/lib/bpf/nlattr.h +3 -3

index 755a3312c87f..7198584a3040 100644
@@ -11,8 +11,8 @@
  * Copyright (c) 2003-2013 Thomas Graf <[email protected]>
  */
 
-#ifndef __NLATTR_H
-#define __NLATTR_H
+#ifndef __LIBBPF_NLATTR_H
+#define __LIBBPF_NLATTR_H
 
 #include <stdint.h>
 #include <linux/netlink.h>

@@ -108,4 +108,4 @@ int libbpf_nla_parse_nested(struct nlattr *tb[], int maxtype,
 
 int libbpf_nla_dump_errormsg(struct nlmsghdr *nlh);
 
-#endif /* __NLATTR_H */
+#endif /* __LIBBPF_NLATTR_H */

tools/lib/bpf/str_error.h +3 -3

index 998eff7d6710..b9157f5eebde 100644
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: LGPL-2.1
-#ifndef BPF_STR_ERROR
-#define BPF_STR_ERROR
+#ifndef __LIBBPF_STR_ERROR_H
+#define __LIBBPF_STR_ERROR_H
 
 char *libbpf_strerror_r(int err, char *dst, int len);
-#endif // BPF_STR_ERROR
+#endif /* __LIBBPF_STR_ERROR_H */

Signature · 1 line

2.17.1

Andrey Ignatov

author

2018-10-04 05:17 UTC #6 lore raw

PATCH 6/6 libbpf: Use __u32 instead of u32 in bpf_program__load

In reply to Andrey Ignatov

Make bpf_program__load consistent with other interfaces: use __u32 instead of u32. That in turn fixes build of samples:

In file included from ./samples/bpf/trace_output_user.c:21:0:
./tools/lib/bpf/libbpf.h:132:9: error: unknown type name ‘u32’

         u32 kern_version);
         ^

Fixes commit 29cd77f41620d ("libbpf: Support loading individual progs")

Signed-off-by Andrey Ignatov <[email protected]>

Acked-by Alexei Starovoitov <[email protected]>

---
 tools/lib/bpf/libbpf.c | 2 +-
 tools/lib/bpf/libbpf.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

tools/lib/bpf/libbpf.c +1 -1

index 02888d36b805..85de1ebd4cb0 100644
@@ -1379,7 +1379,7 @@ load_program(enum bpf_prog_type type, enum bpf_attach_type expected_attach_type,
 
 int
 bpf_program__load(struct bpf_program *prog,
-		  char *license, u32 kern_version)
+		  char *license, __u32 kern_version)
 {
 	int err = 0, fd, i;

tools/lib/bpf/libbpf.h +1 -1

index 28f83dd6022b..fbfc2aec0f0d 100644
@@ -129,7 +129,7 @@ void bpf_program__set_ifindex(struct bpf_program *prog, __u32 ifindex);
 const char *bpf_program__title(struct bpf_program *prog, bool needs_copy);
 
 int bpf_program__load(struct bpf_program *prog, char *license,
-		      u32 kern_version);
+		      __u32 kern_version);
 int bpf_program__fd(struct bpf_program *prog);
 int bpf_program__pin_instance(struct bpf_program *prog, const char *path,
 			      int instance);

Signature · 1 line

2.17.1

Daniel Borkmann

maintainer

2018-10-04 21:15 UTC #7 lore raw

In reply to Andrey Ignatov

[ +Yonghong ]

Quoted 56 lines · Andrey Ignatov

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

Quoted 12 lines

 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

Yonghong Song

2018-10-09 13:59 UTC #8 lore raw

Quoted 75 lines · Daniel Borkmann

[ +Yonghong ]
Quoted 56 lines · Andrey Ignatov
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
Quoted 12 lines
  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!

Quoted 8 lines

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.

Quoted 4 lines

Thanks,
Daniel

Daniel Borkmann

maintainer

2018-10-10 05:37 UTC #9 lore raw

Quoted 85 lines · Yonghong Song

Quoted 75 lines · Daniel Borkmann
[ +Yonghong ]
Quoted 56 lines · Andrey Ignatov
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
Quoted 12 lines
  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!
Quoted 8 lines
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.

Quoted 3 lines

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!