[PATCH bpf-next v2 0/3] tools/bpf: changes of libbpf debug interfaces

[PATCH bpf-next v2 0/3] tools/bpf: changes of libbpf debug interfaces

[Yonghong Song]

These are patches responding to my comments for 
Magnus's patch (https://patchwork.ozlabs.org/patch/1032848/).
The goal is to make pr_* macros available to other C files
than libbpf.c, and to simplify API function libbpf_set_print().

Specifically, Patch #1 used global functions
to facilitate pr_* macros in the header files so they 
are available in different C files. 
Patch #2 removes the global function libbpf_dprint_level_available()
which is added in Patch 1.
Patch #3 simplified libbpf_set_print() which takes only one print
function with a debug level argument among others.

Changelogs:
 v1 -> v2:
   . Renamed global function libbpf_dprint() to libbpf_debug_print()
     to be more expressive.
   . Removed libbpf_dprint_level_available() as it is used only
     once in btf.c and we can remove it by optimizing for common cases.

Yonghong Song (3):
  tools/bpf: move libbpf pr_* debug print functions to headers
  tools/bpf: print out btf log at LIBBPF_WARN level
  tools/bpf: simplify libbpf API function libbpf_set_print()

 tools/lib/bpf/btf.c                           | 110 +++++++++---------
 tools/lib/bpf/btf.h                           |   7 +-
 tools/lib/bpf/libbpf.c                        |  42 +++----
 tools/lib/bpf/libbpf.h                        |  20 ++--
 tools/lib/bpf/test_libbpf.cpp                 |   4 +-
 tools/lib/bpf/util.h                          |  30 +++++
 tools/perf/util/bpf-loader.c                  |  32 ++---
 tools/testing/selftests/bpf/test_btf.c        |   7 +-
 .../testing/selftests/bpf/test_libbpf_open.c  |  36 +++---
 tools/testing/selftests/bpf/test_progs.c      |  20 +++-
 10 files changed, 166 insertions(+), 142 deletions(-)
 create mode 100644 tools/lib/bpf/util.h

-- 
2.17.1

[PATCH bpf-next v2 1/3] tools/bpf: move libbpf pr_* debug print functions to headers

[Yonghong Song]

A global function libbpf_debug_print, which is invisible
outside the shared library, is defined to print based
on levels. The pr_warning, pr_info and pr_debug
macros are moved into the newly created header
common.h. So any .c file including common.h can
use these macros directly.

Currently btf__new and btf_ext__new API has an argument getting
__pr_debug function pointer into btf.c so the debugging information
can be printed there. This patch removed this parameter
from btf__new and btf_ext__new and directly using pr_debug in btf.c.

Another global function libbpf_dprint_level_available, also
invisible outside the shared library, can test
whether a particular level debug printing is
available or not. It is used in btf.c to
test whether DEBUG level debug printing is availabl or not,
based on which the log buffer will be allocated when loading
btf to the kernel.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 tools/lib/bpf/btf.c           | 97 +++++++++++++++++------------------
 tools/lib/bpf/btf.h           |  7 +--
 tools/lib/bpf/libbpf.c        | 46 ++++++++++++-----
 tools/lib/bpf/libbpf.h        |  6 +++
 tools/lib/bpf/test_libbpf.cpp |  2 +-
 tools/lib/bpf/util.h          | 32 ++++++++++++
 6 files changed, 120 insertions(+), 70 deletions(-)
 create mode 100644 tools/lib/bpf/util.h

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index d682d3b8f7b9..159104ec31dc 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -9,8 +9,9 @@
 #include <linux/btf.h>
 #include "btf.h"
 #include "bpf.h"
+#include "libbpf.h"
+#include "util.h"
 
-#define elog(fmt, ...) { if (err_log) err_log(fmt, ##__VA_ARGS__); }
 #define max(a, b) ((a) > (b) ? (a) : (b))
 #define min(a, b) ((a) < (b) ? (a) : (b))
 
@@ -107,54 +108,54 @@ static int btf_add_type(struct btf *btf, struct btf_type *t)
 	return 0;
 }
 
-static int btf_parse_hdr(struct btf *btf, btf_print_fn_t err_log)
+static int btf_parse_hdr(struct btf *btf)
 {
 	const struct btf_header *hdr = btf->hdr;
 	__u32 meta_left;
 
 	if (btf->data_size < sizeof(struct btf_header)) {
-		elog("BTF header not found\n");
+		pr_debug("BTF header not found\n");
 		return -EINVAL;
 	}
 
 	if (hdr->magic != BTF_MAGIC) {
-		elog("Invalid BTF magic:%x\n", hdr->magic);
+		pr_debug("Invalid BTF magic:%x\n", hdr->magic);
 		return -EINVAL;
 	}
 
 	if (hdr->version != BTF_VERSION) {
-		elog("Unsupported BTF version:%u\n", hdr->version);
+		pr_debug("Unsupported BTF version:%u\n", hdr->version);
 		return -ENOTSUP;
 	}
 
 	if (hdr->flags) {
-		elog("Unsupported BTF flags:%x\n", hdr->flags);
+		pr_debug("Unsupported BTF flags:%x\n", hdr->flags);
 		return -ENOTSUP;
 	}
 
 	meta_left = btf->data_size - sizeof(*hdr);
 	if (!meta_left) {
-		elog("BTF has no data\n");
+		pr_debug("BTF has no data\n");
 		return -EINVAL;
 	}
 
 	if (meta_left < hdr->type_off) {
-		elog("Invalid BTF type section offset:%u\n", hdr->type_off);
+		pr_debug("Invalid BTF type section offset:%u\n", hdr->type_off);
 		return -EINVAL;
 	}
 
 	if (meta_left < hdr->str_off) {
-		elog("Invalid BTF string section offset:%u\n", hdr->str_off);
+		pr_debug("Invalid BTF string section offset:%u\n", hdr->str_off);
 		return -EINVAL;
 	}
 
 	if (hdr->type_off >= hdr->str_off) {
-		elog("BTF type section offset >= string section offset. No type?\n");
+		pr_debug("BTF type section offset >= string section offset. No type?\n");
 		return -EINVAL;
 	}
 
 	if (hdr->type_off & 0x02) {
-		elog("BTF type section is not aligned to 4 bytes\n");
+		pr_debug("BTF type section is not aligned to 4 bytes\n");
 		return -EINVAL;
 	}
 
@@ -163,7 +164,7 @@ static int btf_parse_hdr(struct btf *btf, btf_print_fn_t err_log)
 	return 0;
 }
 
-static int btf_parse_str_sec(struct btf *btf, btf_print_fn_t err_log)
+static int btf_parse_str_sec(struct btf *btf)
 {
 	const struct btf_header *hdr = btf->hdr;
 	const char *start = btf->nohdr_data + hdr->str_off;
@@ -171,7 +172,7 @@ static int btf_parse_str_sec(struct btf *btf, btf_print_fn_t err_log)
 
 	if (!hdr->str_len || hdr->str_len - 1 > BTF_MAX_NAME_OFFSET ||
 	    start[0] || end[-1]) {
-		elog("Invalid BTF string section\n");
+		pr_debug("Invalid BTF string section\n");
 		return -EINVAL;
 	}
 
@@ -180,7 +181,7 @@ static int btf_parse_str_sec(struct btf *btf, btf_print_fn_t err_log)
 	return 0;
 }
 
-static int btf_parse_type_sec(struct btf *btf, btf_print_fn_t err_log)
+static int btf_parse_type_sec(struct btf *btf)
 {
 	struct btf_header *hdr = btf->hdr;
 	void *nohdr_data = btf->nohdr_data;
@@ -219,7 +220,7 @@ static int btf_parse_type_sec(struct btf *btf, btf_print_fn_t err_log)
 		case BTF_KIND_RESTRICT:
 			break;
 		default:
-			elog("Unsupported BTF_KIND:%u\n",
+			pr_debug("Unsupported BTF_KIND:%u\n",
 			     BTF_INFO_KIND(t->info));
 			return -EINVAL;
 		}
@@ -363,7 +364,7 @@ void btf__free(struct btf *btf)
 	free(btf);
 }
 
-struct btf *btf__new(__u8 *data, __u32 size, btf_print_fn_t err_log)
+struct btf *btf__new(__u8 *data, __u32 size)
 {
 	__u32 log_buf_size = 0;
 	char *log_buf = NULL;
@@ -376,7 +377,7 @@ struct btf *btf__new(__u8 *data, __u32 size, btf_print_fn_t err_log)
 
 	btf->fd = -1;
 
-	if (err_log) {
+	if (libbpf_dprint_level_available(LIBBPF_DEBUG)) {
 		log_buf = malloc(BPF_LOG_BUF_SIZE);
 		if (!log_buf) {
 			err = -ENOMEM;
@@ -400,21 +401,21 @@ struct btf *btf__new(__u8 *data, __u32 size, btf_print_fn_t err_log)
 
 	if (btf->fd == -1) {
 		err = -errno;
-		elog("Error loading BTF: %s(%d)\n", strerror(errno), errno);
+		pr_debug("Error loading BTF: %s(%d)\n", strerror(errno), errno);
 		if (log_buf && *log_buf)
-			elog("%s\n", log_buf);
+			pr_debug("%s\n", log_buf);
 		goto done;
 	}
 
-	err = btf_parse_hdr(btf, err_log);
+	err = btf_parse_hdr(btf);
 	if (err)
 		goto done;
 
-	err = btf_parse_str_sec(btf, err_log);
+	err = btf_parse_str_sec(btf);
 	if (err)
 		goto done;
 
-	err = btf_parse_type_sec(btf, err_log);
+	err = btf_parse_type_sec(btf);
 
 done:
 	free(log_buf);
@@ -491,7 +492,7 @@ int btf__get_from_id(__u32 id, struct btf **btf)
 		goto exit_free;
 	}
 
-	*btf = btf__new((__u8 *)(long)btf_info.btf, btf_info.btf_size, NULL);
+	*btf = btf__new((__u8 *)(long)btf_info.btf, btf_info.btf_size);
 	if (IS_ERR(*btf)) {
 		err = PTR_ERR(*btf);
 		*btf = NULL;
@@ -514,8 +515,7 @@ struct btf_ext_sec_copy_param {
 
 static int btf_ext_copy_info(struct btf_ext *btf_ext,
 			     __u8 *data, __u32 data_size,
-			     struct btf_ext_sec_copy_param *ext_sec,
-			     btf_print_fn_t err_log)
+			     struct btf_ext_sec_copy_param *ext_sec)
 {
 	const struct btf_ext_header *hdr = (struct btf_ext_header *)data;
 	const struct btf_ext_info_sec *sinfo;
@@ -529,14 +529,14 @@ static int btf_ext_copy_info(struct btf_ext *btf_ext,
 	data_size -= hdr->hdr_len;
 
 	if (ext_sec->off & 0x03) {
-		elog(".BTF.ext %s section is not aligned to 4 bytes\n",
+		pr_debug(".BTF.ext %s section is not aligned to 4 bytes\n",
 		     ext_sec->desc);
 		return -EINVAL;
 	}
 
 	if (data_size < ext_sec->off ||
 	    ext_sec->len > data_size - ext_sec->off) {
-		elog("%s section (off:%u len:%u) is beyond the end of the ELF section .BTF.ext\n",
+		pr_debug("%s section (off:%u len:%u) is beyond the end of the ELF section .BTF.ext\n",
 		     ext_sec->desc, ext_sec->off, ext_sec->len);
 		return -EINVAL;
 	}
@@ -546,7 +546,7 @@ static int btf_ext_copy_info(struct btf_ext *btf_ext,
 
 	/* At least a record size */
 	if (info_left < sizeof(__u32)) {
-		elog(".BTF.ext %s record size not found\n", ext_sec->desc);
+		pr_debug(".BTF.ext %s record size not found\n", ext_sec->desc);
 		return -EINVAL;
 	}
 
@@ -554,7 +554,7 @@ static int btf_ext_copy_info(struct btf_ext *btf_ext,
 	record_size = *(__u32 *)info;
 	if (record_size < ext_sec->min_rec_size ||
 	    record_size & 0x03) {
-		elog("%s section in .BTF.ext has invalid record size %u\n",
+		pr_debug("%s section in .BTF.ext has invalid record size %u\n",
 		     ext_sec->desc, record_size);
 		return -EINVAL;
 	}
@@ -564,7 +564,7 @@ static int btf_ext_copy_info(struct btf_ext *btf_ext,
 
 	/* If no records, return failure now so .BTF.ext won't be used. */
 	if (!info_left) {
-		elog("%s section in .BTF.ext has no records", ext_sec->desc);
+		pr_debug("%s section in .BTF.ext has no records", ext_sec->desc);
 		return -EINVAL;
 	}
 
@@ -574,14 +574,14 @@ static int btf_ext_copy_info(struct btf_ext *btf_ext,
 		__u32 num_records;
 
 		if (info_left < sec_hdrlen) {
-			elog("%s section header is not found in .BTF.ext\n",
+			pr_debug("%s section header is not found in .BTF.ext\n",
 			     ext_sec->desc);
 			return -EINVAL;
 		}
 
 		num_records = sinfo->num_info;
 		if (num_records == 0) {
-			elog("%s section has incorrect num_records in .BTF.ext\n",
+			pr_debug("%s section has incorrect num_records in .BTF.ext\n",
 			     ext_sec->desc);
 			return -EINVAL;
 		}
@@ -589,7 +589,7 @@ static int btf_ext_copy_info(struct btf_ext *btf_ext,
 		total_record_size = sec_hdrlen +
 				    (__u64)num_records * record_size;
 		if (info_left < total_record_size) {
-			elog("%s section has incorrect num_records in .BTF.ext\n",
+			pr_debug("%s section has incorrect num_records in .BTF.ext\n",
 			     ext_sec->desc);
 			return -EINVAL;
 		}
@@ -610,8 +610,7 @@ static int btf_ext_copy_info(struct btf_ext *btf_ext,
 }
 
 static int btf_ext_copy_func_info(struct btf_ext *btf_ext,
-				  __u8 *data, __u32 data_size,
-				  btf_print_fn_t err_log)
+				  __u8 *data, __u32 data_size)
 {
 	const struct btf_ext_header *hdr = (struct btf_ext_header *)data;
 	struct btf_ext_sec_copy_param param = {
@@ -622,12 +621,11 @@ static int btf_ext_copy_func_info(struct btf_ext *btf_ext,
 		.desc = "func_info"
 	};
 
-	return btf_ext_copy_info(btf_ext, data, data_size, &param, err_log);
+	return btf_ext_copy_info(btf_ext, data, data_size, &param);
 }
 
 static int btf_ext_copy_line_info(struct btf_ext *btf_ext,
-				  __u8 *data, __u32 data_size,
-				  btf_print_fn_t err_log)
+				  __u8 *data, __u32 data_size)
 {
 	const struct btf_ext_header *hdr = (struct btf_ext_header *)data;
 	struct btf_ext_sec_copy_param param = {
@@ -638,37 +636,36 @@ static int btf_ext_copy_line_info(struct btf_ext *btf_ext,
 		.desc = "line_info",
 	};
 
-	return btf_ext_copy_info(btf_ext, data, data_size, &param, err_log);
+	return btf_ext_copy_info(btf_ext, data, data_size, &param);
 }
 
-static int btf_ext_parse_hdr(__u8 *data, __u32 data_size,
-			     btf_print_fn_t err_log)
+static int btf_ext_parse_hdr(__u8 *data, __u32 data_size)
 {
 	const struct btf_ext_header *hdr = (struct btf_ext_header *)data;
 
 	if (data_size < offsetof(struct btf_ext_header, func_info_off) ||
 	    data_size < hdr->hdr_len) {
-		elog("BTF.ext header not found");
+		pr_debug("BTF.ext header not found");
 		return -EINVAL;
 	}
 
 	if (hdr->magic != BTF_MAGIC) {
-		elog("Invalid BTF.ext magic:%x\n", hdr->magic);
+		pr_debug("Invalid BTF.ext magic:%x\n", hdr->magic);
 		return -EINVAL;
 	}
 
 	if (hdr->version != BTF_VERSION) {
-		elog("Unsupported BTF.ext version:%u\n", hdr->version);
+		pr_debug("Unsupported BTF.ext version:%u\n", hdr->version);
 		return -ENOTSUP;
 	}
 
 	if (hdr->flags) {
-		elog("Unsupported BTF.ext flags:%x\n", hdr->flags);
+		pr_debug("Unsupported BTF.ext flags:%x\n", hdr->flags);
 		return -ENOTSUP;
 	}
 
 	if (data_size == hdr->hdr_len) {
-		elog("BTF.ext has no data\n");
+		pr_debug("BTF.ext has no data\n");
 		return -EINVAL;
 	}
 
@@ -685,12 +682,12 @@ void btf_ext__free(struct btf_ext *btf_ext)
 	free(btf_ext);
 }
 
-struct btf_ext *btf_ext__new(__u8 *data, __u32 size, btf_print_fn_t err_log)
+struct btf_ext *btf_ext__new(__u8 *data, __u32 size)
 {
 	struct btf_ext *btf_ext;
 	int err;
 
-	err = btf_ext_parse_hdr(data, size, err_log);
+	err = btf_ext_parse_hdr(data, size);
 	if (err)
 		return ERR_PTR(err);
 
@@ -698,13 +695,13 @@ struct btf_ext *btf_ext__new(__u8 *data, __u32 size, btf_print_fn_t err_log)
 	if (!btf_ext)
 		return ERR_PTR(-ENOMEM);
 
-	err = btf_ext_copy_func_info(btf_ext, data, size, err_log);
+	err = btf_ext_copy_func_info(btf_ext, data, size);
 	if (err) {
 		btf_ext__free(btf_ext);
 		return ERR_PTR(err);
 	}
 
-	err = btf_ext_copy_line_info(btf_ext, data, size, err_log);
+	err = btf_ext_copy_line_info(btf_ext, data, size);
 	if (err) {
 		btf_ext__free(btf_ext);
 		return ERR_PTR(err);
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index b0610dcdae6b..b1e8e54cc21d 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -55,11 +55,8 @@ struct btf_ext_header {
 	__u32	line_info_len;
 };
 
-typedef int (*btf_print_fn_t)(const char *, ...)
-	__attribute__((format(printf, 1, 2)));
-
 LIBBPF_API void btf__free(struct btf *btf);
-LIBBPF_API struct btf *btf__new(__u8 *data, __u32 size, btf_print_fn_t err_log);
+LIBBPF_API struct btf *btf__new(__u8 *data, __u32 size);
 LIBBPF_API __s32 btf__find_by_name(const struct btf *btf,
 				   const char *type_name);
 LIBBPF_API const struct btf_type *btf__type_by_id(const struct btf *btf,
@@ -70,7 +67,7 @@ LIBBPF_API int btf__fd(const struct btf *btf);
 LIBBPF_API const char *btf__name_by_offset(const struct btf *btf, __u32 offset);
 LIBBPF_API int btf__get_from_id(__u32 id, struct btf **btf);
 
-struct btf_ext *btf_ext__new(__u8 *data, __u32 size, btf_print_fn_t err_log);
+struct btf_ext *btf_ext__new(__u8 *data, __u32 size);
 void btf_ext__free(struct btf_ext *btf_ext);
 int btf_ext__reloc_func_info(const struct btf *btf,
 			     const struct btf_ext *btf_ext,
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 2ccde17957e6..1ede6b93653e 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -42,6 +42,7 @@
 #include "bpf.h"
 #include "btf.h"
 #include "str_error.h"
+#include "util.h"
 
 #ifndef EM_BPF
 #define EM_BPF 247
@@ -69,16 +70,6 @@ static __printf(1, 2) libbpf_print_fn_t __pr_warning = __base_pr;
 static __printf(1, 2) libbpf_print_fn_t __pr_info = __base_pr;
 static __printf(1, 2) libbpf_print_fn_t __pr_debug;
 
-#define __pr(func, fmt, ...)	\
-do {				\
-	if ((func))		\
-		(func)("libbpf: " fmt, ##__VA_ARGS__); \
-} while (0)
-
-#define pr_warning(fmt, ...)	__pr(__pr_warning, fmt, ##__VA_ARGS__)
-#define pr_info(fmt, ...)	__pr(__pr_info, fmt, ##__VA_ARGS__)
-#define pr_debug(fmt, ...)	__pr(__pr_debug, fmt, ##__VA_ARGS__)
-
 void libbpf_set_print(libbpf_print_fn_t warn,
 		      libbpf_print_fn_t info,
 		      libbpf_print_fn_t debug)
@@ -88,6 +79,35 @@ void libbpf_set_print(libbpf_print_fn_t warn,
 	__pr_debug = debug;
 }
 
+__printf(2, 3)
+void libbpf_debug_print(enum libbpf_print_level level, const char *format, ...)
+{
+	va_list args;
+
+	va_start(args, format);
+	if (level == LIBBPF_WARN) {
+		if (__pr_warning)
+			__pr_warning(format, args);
+	} else if (level == LIBBPF_INFO) {
+		if (__pr_info)
+			__pr_info(format, args);
+	} else {
+		if (__pr_debug)
+			__pr_debug(format, args);
+	}
+	va_end(args);
+}
+
+bool libbpf_dprint_level_available(enum libbpf_print_level level)
+{
+	if (level == LIBBPF_WARN)
+		return !!__pr_warning;
+	else if (level == LIBBPF_INFO)
+		return !!__pr_info;
+	else
+		return !!__pr_debug;
+}
+
 #define STRERR_BUFSIZE  128
 
 #define CHECK_ERR(action, err, out) do {	\
@@ -839,8 +859,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
 		else if (strcmp(name, "maps") == 0)
 			obj->efile.maps_shndx = idx;
 		else if (strcmp(name, BTF_ELF_SEC) == 0) {
-			obj->btf = btf__new(data->d_buf, data->d_size,
-					    __pr_debug);
+			obj->btf = btf__new(data->d_buf, data->d_size);
 			if (IS_ERR(obj->btf)) {
 				pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n",
 					   BTF_ELF_SEC, PTR_ERR(obj->btf));
@@ -915,8 +934,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
 				 BTF_EXT_ELF_SEC, BTF_ELF_SEC);
 		} else {
 			obj->btf_ext = btf_ext__new(btf_ext_data->d_buf,
-						    btf_ext_data->d_size,
-						    __pr_debug);
+						    btf_ext_data->d_size);
 			if (IS_ERR(obj->btf_ext)) {
 				pr_warning("Error loading ELF section %s: %ld. Ignored and continue.\n",
 					   BTF_EXT_ELF_SEC,
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 62ae6cb93da1..4e21971101c9 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -47,6 +47,12 @@ enum libbpf_errno {
 
 LIBBPF_API int libbpf_strerror(int err, char *buf, size_t size);
 
+enum libbpf_print_level {
+        LIBBPF_WARN,
+        LIBBPF_INFO,
+        LIBBPF_DEBUG,
+};
+
 /*
  * __printf is defined in include/linux/compiler-gcc.h. However,
  * it would be better if libbpf.h didn't depend on Linux header files.
diff --git a/tools/lib/bpf/test_libbpf.cpp b/tools/lib/bpf/test_libbpf.cpp
index abf3fc25c9fa..be67f5ea2c19 100644
--- a/tools/lib/bpf/test_libbpf.cpp
+++ b/tools/lib/bpf/test_libbpf.cpp
@@ -14,5 +14,5 @@ int main(int argc, char *argv[])
     bpf_prog_get_fd_by_id(0);
 
     /* btf.h */
-    btf__new(NULL, 0, NULL);
+    btf__new(NULL, 0);
 }
diff --git a/tools/lib/bpf/util.h b/tools/lib/bpf/util.h
new file mode 100644
index 000000000000..3ae80f486875
--- /dev/null
+++ b/tools/lib/bpf/util.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
+/* Copyright (c) 2019 Facebook */
+
+#ifndef __LIBBPF_COMMON_H
+#define __LIBBPF_COMMON_H
+
+#include <stdbool.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+extern void libbpf_debug_print(enum libbpf_print_level level,
+			       const char *format, ...)
+	__attribute__((format(printf, 2, 3)));
+
+extern bool libbpf_dprint_level_available(enum libbpf_print_level level);
+
+#define __pr(level, fmt, ...)	\
+do {				\
+	libbpf_debug_print(level, "libbpf: " fmt, ##__VA_ARGS__);	\
+} while (0)
+
+#define pr_warning(fmt, ...)	__pr(LIBBPF_WARN, fmt, ##__VA_ARGS__)
+#define pr_info(fmt, ...)	__pr(LIBBPF_INFO, fmt, ##__VA_ARGS__)
+#define pr_debug(fmt, ...)	__pr(LIBBPF_DEBUG, fmt, ##__VA_ARGS__)
+
+#ifdef __cplusplus
+} /* extern "C" */
+#endif
+
+#endif
-- 
2.17.1

[PATCH bpf-next v2 2/3] tools/bpf: print out btf log at LIBBPF_WARN level

[Yonghong Song]

Currently, the btf log is allocated and printed out in case
of error at LIBBPF_DEBUG level.
Such logs from kernel are very important for debugging.
For example, bpf syscall BPF_PROG_LOAD command can get
verifier logs back to user space. In function load_program()
of libbpf.c, the log buffer is allocated unconditionally
and printed out at pr_warning() level.

Let us do the similar thing here for btf. Allocate buffer
unconditionally and print out error logs at pr_warning() level.
This is to reduce one global function and
optimize for common situations where pr_warning()
is activated either by default or by user supplied
debug output function.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 tools/lib/bpf/btf.c    | 19 +++++++++----------
 tools/lib/bpf/libbpf.c | 10 ----------
 tools/lib/bpf/util.h   |  2 --
 3 files changed, 9 insertions(+), 22 deletions(-)

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 159104ec31dc..46271bc19572 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -377,16 +377,15 @@ struct btf *btf__new(__u8 *data, __u32 size)
 
 	btf->fd = -1;
 
-	if (libbpf_dprint_level_available(LIBBPF_DEBUG)) {
-		log_buf = malloc(BPF_LOG_BUF_SIZE);
-		if (!log_buf) {
-			err = -ENOMEM;
-			goto done;
-		}
-		*log_buf = 0;
-		log_buf_size = BPF_LOG_BUF_SIZE;
+	log_buf = malloc(BPF_LOG_BUF_SIZE);
+	if (!log_buf) {
+		err = -ENOMEM;
+		goto done;
 	}
 
+	*log_buf = 0;
+	log_buf_size = BPF_LOG_BUF_SIZE;
+
 	btf->data = malloc(size);
 	if (!btf->data) {
 		err = -ENOMEM;
@@ -401,9 +400,9 @@ struct btf *btf__new(__u8 *data, __u32 size)
 
 	if (btf->fd == -1) {
 		err = -errno;
-		pr_debug("Error loading BTF: %s(%d)\n", strerror(errno), errno);
+		pr_warning("Error loading BTF: %s(%d)\n", strerror(errno), errno);
 		if (log_buf && *log_buf)
-			pr_debug("%s\n", log_buf);
+			pr_warning("%s\n", log_buf);
 		goto done;
 	}
 
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 1ede6b93653e..1b1c0b504d25 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -98,16 +98,6 @@ void libbpf_debug_print(enum libbpf_print_level level, const char *format, ...)
 	va_end(args);
 }
 
-bool libbpf_dprint_level_available(enum libbpf_print_level level)
-{
-	if (level == LIBBPF_WARN)
-		return !!__pr_warning;
-	else if (level == LIBBPF_INFO)
-		return !!__pr_info;
-	else
-		return !!__pr_debug;
-}
-
 #define STRERR_BUFSIZE  128
 
 #define CHECK_ERR(action, err, out) do {	\
diff --git a/tools/lib/bpf/util.h b/tools/lib/bpf/util.h
index 3ae80f486875..66d65b803095 100644
--- a/tools/lib/bpf/util.h
+++ b/tools/lib/bpf/util.h
@@ -14,8 +14,6 @@ extern void libbpf_debug_print(enum libbpf_print_level level,
 			       const char *format, ...)
 	__attribute__((format(printf, 2, 3)));
 
-extern bool libbpf_dprint_level_available(enum libbpf_print_level level);
-
 #define __pr(level, fmt, ...)	\
 do {				\
 	libbpf_debug_print(level, "libbpf: " fmt, ##__VA_ARGS__);	\
-- 
2.17.1

[PATCH bpf-next v2 3/3] tools/bpf: simplify libbpf API function libbpf_set_print()

[Yonghong Song]

Currently, the libbpf API function libbpf_set_print()
takes three function pointer parameters for warning, info
and debug printout respectively.

This patch changes the API to have just one function pointer
parameter and the function pointer has one additional
parameter "debugging level". So if in the future, if
the debug level is increased, the function signature
won't change.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 tools/lib/bpf/libbpf.c                        | 28 ++++-----------
 tools/lib/bpf/libbpf.h                        | 14 +++-----
 tools/lib/bpf/test_libbpf.cpp                 |  2 +-
 tools/perf/util/bpf-loader.c                  | 32 +++++++----------
 tools/testing/selftests/bpf/test_btf.c        |  7 ++--
 .../testing/selftests/bpf/test_libbpf_open.c  | 36 +++++++++----------
 tools/testing/selftests/bpf/test_progs.c      | 20 +++++++++--
 7 files changed, 63 insertions(+), 76 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 1b1c0b504d25..d2337a179837 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -54,8 +54,8 @@
 
 #define __printf(a, b)	__attribute__((format(printf, a, b)))
 
-__printf(1, 2)
-static int __base_pr(const char *format, ...)
+__printf(2, 3)
+static int __base_pr(enum libbpf_print_level level, const char *format, ...)
 {
 	va_list args;
 	int err;
@@ -66,17 +66,11 @@ static int __base_pr(const char *format, ...)
 	return err;
 }
 
-static __printf(1, 2) libbpf_print_fn_t __pr_warning = __base_pr;
-static __printf(1, 2) libbpf_print_fn_t __pr_info = __base_pr;
-static __printf(1, 2) libbpf_print_fn_t __pr_debug;
+static __printf(2, 3) libbpf_print_fn_t __libbpf_pr = __base_pr;
 
-void libbpf_set_print(libbpf_print_fn_t warn,
-		      libbpf_print_fn_t info,
-		      libbpf_print_fn_t debug)
+void libbpf_set_print(libbpf_print_fn_t fn)
 {
-	__pr_warning = warn;
-	__pr_info = info;
-	__pr_debug = debug;
+	__libbpf_pr = fn;
 }
 
 __printf(2, 3)
@@ -85,16 +79,8 @@ void libbpf_debug_print(enum libbpf_print_level level, const char *format, ...)
 	va_list args;
 
 	va_start(args, format);
-	if (level == LIBBPF_WARN) {
-		if (__pr_warning)
-			__pr_warning(format, args);
-	} else if (level == LIBBPF_INFO) {
-		if (__pr_info)
-			__pr_info(format, args);
-	} else {
-		if (__pr_debug)
-			__pr_debug(format, args);
-	}
+	if (__libbpf_pr)
+		__libbpf_pr(level, format, args);
 	va_end(args);
 }
 
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 4e21971101c9..f8f27f1bb6bf 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -53,17 +53,11 @@ enum libbpf_print_level {
         LIBBPF_DEBUG,
 };
 
-/*
- * __printf is defined in include/linux/compiler-gcc.h. However,
- * it would be better if libbpf.h didn't depend on Linux header files.
- * So instead of __printf, here we use gcc attribute directly.
- */
-typedef int (*libbpf_print_fn_t)(const char *, ...)
-	__attribute__((format(printf, 1, 2)));
+typedef int (*libbpf_print_fn_t)(enum libbpf_print_level level,
+				 const char *, ...)
+	__attribute__((format(printf, 2, 3)));
 
-LIBBPF_API void libbpf_set_print(libbpf_print_fn_t warn,
-				 libbpf_print_fn_t info,
-				 libbpf_print_fn_t debug);
+LIBBPF_API void libbpf_set_print(libbpf_print_fn_t fn);
 
 /* Hide internal to user */
 struct bpf_object;
diff --git a/tools/lib/bpf/test_libbpf.cpp b/tools/lib/bpf/test_libbpf.cpp
index be67f5ea2c19..fc134873bb6d 100644
--- a/tools/lib/bpf/test_libbpf.cpp
+++ b/tools/lib/bpf/test_libbpf.cpp
@@ -8,7 +8,7 @@
 int main(int argc, char *argv[])
 {
     /* libbpf.h */
-    libbpf_set_print(NULL, NULL, NULL);
+    libbpf_set_print(NULL);
 
     /* bpf.h */
     bpf_prog_get_fd_by_id(0);
diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
index 2f3eb6d293ee..c492f3a2acdc 100644
--- a/tools/perf/util/bpf-loader.c
+++ b/tools/perf/util/bpf-loader.c
@@ -24,21 +24,17 @@
 #include "llvm-utils.h"
 #include "c++/clang-c.h"
 
-#define DEFINE_PRINT_FN(name, level) \
-static int libbpf_##name(const char *fmt, ...)	\
-{						\
-	va_list args;				\
-	int ret;				\
-						\
-	va_start(args, fmt);			\
-	ret = veprintf(level, verbose, pr_fmt(fmt), args);\
-	va_end(args);				\
-	return ret;				\
-}
+static int libbpf_perf_dprint(enum libbpf_print_level level __attribute__((unused)),
+			      const char *fmt, ...)
+{
+	va_list args;
+	int ret;
 
-DEFINE_PRINT_FN(warning, 1)
-DEFINE_PRINT_FN(info, 1)
-DEFINE_PRINT_FN(debug, 1)
+	va_start(args, fmt);
+	ret = veprintf(1, verbose, pr_fmt(fmt), args);
+	va_end(args);
+	return ret;
+}
 
 struct bpf_prog_priv {
 	bool is_tp;
@@ -59,9 +55,7 @@ bpf__prepare_load_buffer(void *obj_buf, size_t obj_buf_sz, const char *name)
 	struct bpf_object *obj;
 
 	if (!libbpf_initialized) {
-		libbpf_set_print(libbpf_warning,
-				 libbpf_info,
-				 libbpf_debug);
+		libbpf_set_print(libbpf_perf_dprint);
 		libbpf_initialized = true;
 	}
 
@@ -79,9 +73,7 @@ struct bpf_object *bpf__prepare_load(const char *filename, bool source)
 	struct bpf_object *obj;
 
 	if (!libbpf_initialized) {
-		libbpf_set_print(libbpf_warning,
-				 libbpf_info,
-				 libbpf_debug);
+		libbpf_set_print(libbpf_perf_dprint);
 		libbpf_initialized = true;
 	}
 
diff --git a/tools/testing/selftests/bpf/test_btf.c b/tools/testing/selftests/bpf/test_btf.c
index 179f1d8ec5bf..aebaeff5a5a0 100644
--- a/tools/testing/selftests/bpf/test_btf.c
+++ b/tools/testing/selftests/bpf/test_btf.c
@@ -54,8 +54,9 @@ static int count_result(int err)
 
 #define __printf(a, b)	__attribute__((format(printf, a, b)))
 
-__printf(1, 2)
-static int __base_pr(const char *format, ...)
+__printf(2, 3)
+static int __base_pr(enum libbpf_print_level level __attribute__((unused)),
+		     const char *format, ...)
 {
 	va_list args;
 	int err;
@@ -5650,7 +5651,7 @@ int main(int argc, char **argv)
 		return err;
 
 	if (args.always_log)
-		libbpf_set_print(__base_pr, __base_pr, __base_pr);
+		libbpf_set_print(__base_pr);
 
 	if (args.raw_test)
 		err |= test_raw();
diff --git a/tools/testing/selftests/bpf/test_libbpf_open.c b/tools/testing/selftests/bpf/test_libbpf_open.c
index 8fcd1c076add..3fe258520e4b 100644
--- a/tools/testing/selftests/bpf/test_libbpf_open.c
+++ b/tools/testing/selftests/bpf/test_libbpf_open.c
@@ -34,23 +34,22 @@ static void usage(char *argv[])
 	printf("\n");
 }
 
-#define DEFINE_PRINT_FN(name, enabled) \
-static int libbpf_##name(const char *fmt, ...)  	\
-{							\
-        va_list args;					\
-        int ret;					\
-							\
-        va_start(args, fmt);				\
-	if (enabled) {					\
-		fprintf(stderr, "[" #name "] ");	\
-		ret = vfprintf(stderr, fmt, args);	\
-	}						\
-        va_end(args);					\
-        return ret;					\
+static bool debug = 0;
+static int libbpf_print(enum libbpf_print_level level,
+			const char *fmt, ...)
+{
+	va_list args;
+	int ret;
+
+	if (level == LIBBPF_DEBUG && !debug)
+		return 0;
+
+	va_start(args, fmt);
+	fprintf(stderr, "[%d] ", level);
+	ret = vfprintf(stderr, fmt, args);
+	va_end(args);
+	return ret;
 }
-DEFINE_PRINT_FN(warning, 1)
-DEFINE_PRINT_FN(info, 1)
-DEFINE_PRINT_FN(debug, 1)
 
 #define EXIT_FAIL_LIBBPF EXIT_FAILURE
 #define EXIT_FAIL_OPTION 2
@@ -120,15 +119,14 @@ int main(int argc, char **argv)
 	int longindex = 0;
 	int opt;
 
-	libbpf_set_print(libbpf_warning, libbpf_info, NULL);
+	libbpf_set_print(libbpf_print);
 
 	/* Parse commands line args */
 	while ((opt = getopt_long(argc, argv, "hDq",
 				  long_options, &longindex)) != -1) {
 		switch (opt) {
 		case 'D':
-			libbpf_set_print(libbpf_warning, libbpf_info,
-					 libbpf_debug);
+			debug = 1;
 			break;
 		case 'q': /* Use in scripting mode */
 			verbose = 0;
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index d8940b8b2f8d..5eff68ab2c1c 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -10,6 +10,7 @@
 #include <string.h>
 #include <assert.h>
 #include <stdlib.h>
+#include <stdarg.h>
 #include <time.h>
 
 #include <linux/types.h>
@@ -1783,6 +1784,21 @@ static void test_task_fd_query_tp(void)
 				   "sys_enter_read");
 }
 
+static int libbpf_print(enum libbpf_print_level level,
+			const char *format, ...)
+{
+	va_list args;
+	int ret;
+
+	if (level == LIBBPF_DEBUG)
+		return 0;
+
+	va_start(args, format);
+	ret = vfprintf(stderr, format, args);
+	va_end(args);
+	return ret;
+}
+
 static void test_reference_tracking()
 {
 	const char *file = "./test_sk_lookup_kern.o";
@@ -1809,9 +1825,9 @@ static void test_reference_tracking()
 
 		/* Expect verifier failure if test name has 'fail' */
 		if (strstr(title, "fail") != NULL) {
-			libbpf_set_print(NULL, NULL, NULL);
+			libbpf_set_print(NULL);
 			err = !bpf_program__load(prog, "GPL", 0);
-			libbpf_set_print(printf, printf, NULL);
+			libbpf_set_print(libbpf_print);
 		} else {
 			err = bpf_program__load(prog, "GPL", 0);
 		}
-- 
2.17.1

Re: [PATCH bpf-next v2 3/3] tools/bpf: simplify libbpf API function libbpf_set_print()

[Andrii Nakryiko]

On Fri, Feb 1, 2019 at 10:16 AM Yonghong Song <yhs@fb.com> wrote:
>
> Currently, the libbpf API function libbpf_set_print()
> takes three function pointer parameters for warning, info
> and debug printout respectively.
>
> This patch changes the API to have just one function pointer
> parameter and the function pointer has one additional
> parameter "debugging level". So if in the future, if
> the debug level is increased, the function signature
> won't change.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>  tools/lib/bpf/libbpf.c                        | 28 ++++-----------
>  tools/lib/bpf/libbpf.h                        | 14 +++-----
>  tools/lib/bpf/test_libbpf.cpp                 |  2 +-
>  tools/perf/util/bpf-loader.c                  | 32 +++++++----------
>  tools/testing/selftests/bpf/test_btf.c        |  7 ++--
>  .../testing/selftests/bpf/test_libbpf_open.c  | 36 +++++++++----------
>  tools/testing/selftests/bpf/test_progs.c      | 20 +++++++++--
>  7 files changed, 63 insertions(+), 76 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 1b1c0b504d25..d2337a179837 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -54,8 +54,8 @@
>
>  #define __printf(a, b) __attribute__((format(printf, a, b)))
>
> -__printf(1, 2)
> -static int __base_pr(const char *format, ...)
> +__printf(2, 3)
> +static int __base_pr(enum libbpf_print_level level, const char *format, ...)
>  {
>         va_list args;
>         int err;
> @@ -66,17 +66,11 @@ static int __base_pr(const char *format, ...)
>         return err;
>  }
>
> -static __printf(1, 2) libbpf_print_fn_t __pr_warning = __base_pr;
> -static __printf(1, 2) libbpf_print_fn_t __pr_info = __base_pr;
> -static __printf(1, 2) libbpf_print_fn_t __pr_debug;
> +static __printf(2, 3) libbpf_print_fn_t __libbpf_pr = __base_pr;
>
> -void libbpf_set_print(libbpf_print_fn_t warn,
> -                     libbpf_print_fn_t info,
> -                     libbpf_print_fn_t debug)
> +void libbpf_set_print(libbpf_print_fn_t fn)
>  {
> -       __pr_warning = warn;
> -       __pr_info = info;
> -       __pr_debug = debug;
> +       __libbpf_pr = fn;
>  }
>
>  __printf(2, 3)
> @@ -85,16 +79,8 @@ void libbpf_debug_print(enum libbpf_print_level level, const char *format, ...)
>         va_list args;
>
>         va_start(args, format);
> -       if (level == LIBBPF_WARN) {
> -               if (__pr_warning)
> -                       __pr_warning(format, args);
> -       } else if (level == LIBBPF_INFO) {
> -               if (__pr_info)
> -                       __pr_info(format, args);
> -       } else {
> -               if (__pr_debug)
> -                       __pr_debug(format, args);
> -       }
> +       if (__libbpf_pr)

If __libbpf_pr is NULL, is there a need to call va_start/va_end? If
not, should they be moved inside if's body?

> +               __libbpf_pr(level, format, args);
>         va_end(args);
>  }
>
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 4e21971101c9..f8f27f1bb6bf 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -53,17 +53,11 @@ enum libbpf_print_level {
>          LIBBPF_DEBUG,
>  };
>
> -/*
> - * __printf is defined in include/linux/compiler-gcc.h. However,
> - * it would be better if libbpf.h didn't depend on Linux header files.
> - * So instead of __printf, here we use gcc attribute directly.
> - */
> -typedef int (*libbpf_print_fn_t)(const char *, ...)
> -       __attribute__((format(printf, 1, 2)));
> +typedef int (*libbpf_print_fn_t)(enum libbpf_print_level level,
> +                                const char *, ...)
> +       __attribute__((format(printf, 2, 3)));
>
> -LIBBPF_API void libbpf_set_print(libbpf_print_fn_t warn,
> -                                libbpf_print_fn_t info,
> -                                libbpf_print_fn_t debug);
> +LIBBPF_API void libbpf_set_print(libbpf_print_fn_t fn);
>
>  /* Hide internal to user */
>  struct bpf_object;
> diff --git a/tools/lib/bpf/test_libbpf.cpp b/tools/lib/bpf/test_libbpf.cpp
> index be67f5ea2c19..fc134873bb6d 100644
> --- a/tools/lib/bpf/test_libbpf.cpp
> +++ b/tools/lib/bpf/test_libbpf.cpp
> @@ -8,7 +8,7 @@
>  int main(int argc, char *argv[])
>  {
>      /* libbpf.h */
> -    libbpf_set_print(NULL, NULL, NULL);
> +    libbpf_set_print(NULL);
>
>      /* bpf.h */
>      bpf_prog_get_fd_by_id(0);
> diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> index 2f3eb6d293ee..c492f3a2acdc 100644
> --- a/tools/perf/util/bpf-loader.c
> +++ b/tools/perf/util/bpf-loader.c
> @@ -24,21 +24,17 @@
>  #include "llvm-utils.h"
>  #include "c++/clang-c.h"
>
> -#define DEFINE_PRINT_FN(name, level) \
> -static int libbpf_##name(const char *fmt, ...) \
> -{                                              \
> -       va_list args;                           \
> -       int ret;                                \
> -                                               \
> -       va_start(args, fmt);                    \
> -       ret = veprintf(level, verbose, pr_fmt(fmt), args);\
> -       va_end(args);                           \
> -       return ret;                             \
> -}
> +static int libbpf_perf_dprint(enum libbpf_print_level level __attribute__((unused)),
> +                             const char *fmt, ...)
> +{
> +       va_list args;
> +       int ret;
>
> -DEFINE_PRINT_FN(warning, 1)
> -DEFINE_PRINT_FN(info, 1)
> -DEFINE_PRINT_FN(debug, 1)
> +       va_start(args, fmt);
> +       ret = veprintf(1, verbose, pr_fmt(fmt), args);
> +       va_end(args);
> +       return ret;
> +}
>
>  struct bpf_prog_priv {
>         bool is_tp;
> @@ -59,9 +55,7 @@ bpf__prepare_load_buffer(void *obj_buf, size_t obj_buf_sz, const char *name)
>         struct bpf_object *obj;
>
>         if (!libbpf_initialized) {
> -               libbpf_set_print(libbpf_warning,
> -                                libbpf_info,
> -                                libbpf_debug);
> +               libbpf_set_print(libbpf_perf_dprint);
>                 libbpf_initialized = true;
>         }
>
> @@ -79,9 +73,7 @@ struct bpf_object *bpf__prepare_load(const char *filename, bool source)
>         struct bpf_object *obj;
>
>         if (!libbpf_initialized) {
> -               libbpf_set_print(libbpf_warning,
> -                                libbpf_info,
> -                                libbpf_debug);
> +               libbpf_set_print(libbpf_perf_dprint);
>                 libbpf_initialized = true;
>         }
>
> diff --git a/tools/testing/selftests/bpf/test_btf.c b/tools/testing/selftests/bpf/test_btf.c
> index 179f1d8ec5bf..aebaeff5a5a0 100644
> --- a/tools/testing/selftests/bpf/test_btf.c
> +++ b/tools/testing/selftests/bpf/test_btf.c
> @@ -54,8 +54,9 @@ static int count_result(int err)
>
>  #define __printf(a, b) __attribute__((format(printf, a, b)))
>
> -__printf(1, 2)
> -static int __base_pr(const char *format, ...)
> +__printf(2, 3)
> +static int __base_pr(enum libbpf_print_level level __attribute__((unused)),
> +                    const char *format, ...)
>  {
>         va_list args;
>         int err;
> @@ -5650,7 +5651,7 @@ int main(int argc, char **argv)
>                 return err;
>
>         if (args.always_log)
> -               libbpf_set_print(__base_pr, __base_pr, __base_pr);
> +               libbpf_set_print(__base_pr);
>
>         if (args.raw_test)
>                 err |= test_raw();
> diff --git a/tools/testing/selftests/bpf/test_libbpf_open.c b/tools/testing/selftests/bpf/test_libbpf_open.c
> index 8fcd1c076add..3fe258520e4b 100644
> --- a/tools/testing/selftests/bpf/test_libbpf_open.c
> +++ b/tools/testing/selftests/bpf/test_libbpf_open.c
> @@ -34,23 +34,22 @@ static void usage(char *argv[])
>         printf("\n");
>  }
>
> -#define DEFINE_PRINT_FN(name, enabled) \
> -static int libbpf_##name(const char *fmt, ...)         \
> -{                                                      \
> -        va_list args;                                  \
> -        int ret;                                       \
> -                                                       \
> -        va_start(args, fmt);                           \
> -       if (enabled) {                                  \
> -               fprintf(stderr, "[" #name "] ");        \
> -               ret = vfprintf(stderr, fmt, args);      \
> -       }                                               \
> -        va_end(args);                                  \
> -        return ret;                                    \
> +static bool debug = 0;
> +static int libbpf_print(enum libbpf_print_level level,
> +                       const char *fmt, ...)
> +{
> +       va_list args;
> +       int ret;
> +
> +       if (level == LIBBPF_DEBUG && !debug)
> +               return 0;
> +
> +       va_start(args, fmt);
> +       fprintf(stderr, "[%d] ", level);
> +       ret = vfprintf(stderr, fmt, args);
> +       va_end(args);
> +       return ret;
>  }
> -DEFINE_PRINT_FN(warning, 1)
> -DEFINE_PRINT_FN(info, 1)
> -DEFINE_PRINT_FN(debug, 1)
>
>  #define EXIT_FAIL_LIBBPF EXIT_FAILURE
>  #define EXIT_FAIL_OPTION 2
> @@ -120,15 +119,14 @@ int main(int argc, char **argv)
>         int longindex = 0;
>         int opt;
>
> -       libbpf_set_print(libbpf_warning, libbpf_info, NULL);
> +       libbpf_set_print(libbpf_print);
>
>         /* Parse commands line args */
>         while ((opt = getopt_long(argc, argv, "hDq",
>                                   long_options, &longindex)) != -1) {
>                 switch (opt) {
>                 case 'D':
> -                       libbpf_set_print(libbpf_warning, libbpf_info,
> -                                        libbpf_debug);
> +                       debug = 1;
>                         break;
>                 case 'q': /* Use in scripting mode */
>                         verbose = 0;
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index d8940b8b2f8d..5eff68ab2c1c 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -10,6 +10,7 @@
>  #include <string.h>
>  #include <assert.h>
>  #include <stdlib.h>
> +#include <stdarg.h>
>  #include <time.h>
>
>  #include <linux/types.h>
> @@ -1783,6 +1784,21 @@ static void test_task_fd_query_tp(void)
>                                    "sys_enter_read");
>  }
>
> +static int libbpf_print(enum libbpf_print_level level,
> +                       const char *format, ...)
> +{
> +       va_list args;
> +       int ret;
> +
> +       if (level == LIBBPF_DEBUG)
> +               return 0;
> +
> +       va_start(args, format);
> +       ret = vfprintf(stderr, format, args);
> +       va_end(args);
> +       return ret;
> +}
> +
>  static void test_reference_tracking()
>  {
>         const char *file = "./test_sk_lookup_kern.o";
> @@ -1809,9 +1825,9 @@ static void test_reference_tracking()
>
>                 /* Expect verifier failure if test name has 'fail' */
>                 if (strstr(title, "fail") != NULL) {
> -                       libbpf_set_print(NULL, NULL, NULL);
> +                       libbpf_set_print(NULL);
>                         err = !bpf_program__load(prog, "GPL", 0);
> -                       libbpf_set_print(printf, printf, NULL);
> +                       libbpf_set_print(libbpf_print);
>                 } else {
>                         err = bpf_program__load(prog, "GPL", 0);
>                 }
> --
> 2.17.1
>

Re: [PATCH bpf-next v2 3/3] tools/bpf: simplify libbpf API function libbpf_set_print()

[Yonghong Song]

On 2/1/19 11:02 AM, Andrii Nakryiko wrote:
> On Fri, Feb 1, 2019 at 10:16 AM Yonghong Song <yhs@fb.com> wrote:
>>
>> Currently, the libbpf API function libbpf_set_print()
>> takes three function pointer parameters for warning, info
>> and debug printout respectively.
>>
>> This patch changes the API to have just one function pointer
>> parameter and the function pointer has one additional
>> parameter "debugging level". So if in the future, if
>> the debug level is increased, the function signature
>> won't change.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   tools/lib/bpf/libbpf.c                        | 28 ++++-----------
>>   tools/lib/bpf/libbpf.h                        | 14 +++-----
>>   tools/lib/bpf/test_libbpf.cpp                 |  2 +-
>>   tools/perf/util/bpf-loader.c                  | 32 +++++++----------
>>   tools/testing/selftests/bpf/test_btf.c        |  7 ++--
>>   .../testing/selftests/bpf/test_libbpf_open.c  | 36 +++++++++----------
>>   tools/testing/selftests/bpf/test_progs.c      | 20 +++++++++--
>>   7 files changed, 63 insertions(+), 76 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 1b1c0b504d25..d2337a179837 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -54,8 +54,8 @@
>>
>>   #define __printf(a, b) __attribute__((format(printf, a, b)))
>>
>> -__printf(1, 2)
>> -static int __base_pr(const char *format, ...)
>> +__printf(2, 3)
>> +static int __base_pr(enum libbpf_print_level level, const char *format, ...)
>>   {
>>          va_list args;
>>          int err;
>> @@ -66,17 +66,11 @@ static int __base_pr(const char *format, ...)
>>          return err;
>>   }
>>
>> -static __printf(1, 2) libbpf_print_fn_t __pr_warning = __base_pr;
>> -static __printf(1, 2) libbpf_print_fn_t __pr_info = __base_pr;
>> -static __printf(1, 2) libbpf_print_fn_t __pr_debug;
>> +static __printf(2, 3) libbpf_print_fn_t __libbpf_pr = __base_pr;
>>
>> -void libbpf_set_print(libbpf_print_fn_t warn,
>> -                     libbpf_print_fn_t info,
>> -                     libbpf_print_fn_t debug)
>> +void libbpf_set_print(libbpf_print_fn_t fn)
>>   {
>> -       __pr_warning = warn;
>> -       __pr_info = info;
>> -       __pr_debug = debug;
>> +       __libbpf_pr = fn;
>>   }
>>
>>   __printf(2, 3)
>> @@ -85,16 +79,8 @@ void libbpf_debug_print(enum libbpf_print_level level, const char *format, ...)
>>          va_list args;
>>
>>          va_start(args, format);
>> -       if (level == LIBBPF_WARN) {
>> -               if (__pr_warning)
>> -                       __pr_warning(format, args);
>> -       } else if (level == LIBBPF_INFO) {
>> -               if (__pr_info)
>> -                       __pr_info(format, args);
>> -       } else {
>> -               if (__pr_debug)
>> -                       __pr_debug(format, args);
>> -       }
>> +       if (__libbpf_pr)
> 
> If __libbpf_pr is NULL, is there a need to call va_start/va_end? If
> not, should they be moved inside if's body?

You are right. Will fix this in the next version.

> 
>> +               __libbpf_pr(level, format, args);
>>          va_end(args);
>>   }
>>

Re: [PATCH bpf-next v2 1/3] tools/bpf: move libbpf pr_* debug print functions to headers

[Arnaldo Carvalho de Melo]

Em Fri, Feb 01, 2019 at 09:47:31AM -0800, Yonghong Song escreveu:
> @@ -698,13 +695,13 @@ struct btf_ext *btf_ext__new(__u8 *data, __u32 size, btf_print_fn_t err_log)
>  	if (!btf_ext)
>  		return ERR_PTR(-ENOMEM);
>  
> -	err = btf_ext_copy_func_info(btf_ext, data, size, err_log);
> +	err = btf_ext_copy_func_info(btf_ext, data, size);
>  	if (err) {
>  		btf_ext__free(btf_ext);

One thing I noticed whas that the class + __ + method is not being
consistently followed, will this be dealt with in a followup patch, i.e.
to make this consistently use the format used in the
btf_ext__free(btf_ext) case?

>  		return ERR_PTR(err);
>  	}
>  
> -	err = btf_ext_copy_line_info(btf_ext, data, size, err_log);
> +	err = btf_ext_copy_line_info(btf_ext, data, size);
>  	if (err) {
>  		btf_ext__free(btf_ext);
>  		return ERR_PTR(err);

- Arnaldo

Re: [PATCH bpf-next v2 1/3] tools/bpf: move libbpf pr_* debug print functions to headers

[Yonghong Song]

On 2/1/19 3:10 PM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Feb 01, 2019 at 09:47:31AM -0800, Yonghong Song escreveu:
>> @@ -698,13 +695,13 @@ struct btf_ext *btf_ext__new(__u8 *data, __u32 size, btf_print_fn_t err_log)
>>   	if (!btf_ext)
>>   		return ERR_PTR(-ENOMEM);
>>   
>> -	err = btf_ext_copy_func_info(btf_ext, data, size, err_log);
>> +	err = btf_ext_copy_func_info(btf_ext, data, size);
>>   	if (err) {
>>   		btf_ext__free(btf_ext);
> 
> One thing I noticed whas that the class + __ + method is not being
> consistently followed, will this be dealt with in a followup patch, i.e.
> to make this consistently use the format used in the
> btf_ext__free(btf_ext) case?

Currently, the API functions have both +_+ and +__+ method names.
The +_+ is for APIs having a close one-to-one mapping
to system calls. The +__+ is for APIs a little bit high level.
Did you find any particular method whose format is not quite right?

> 
>>   		return ERR_PTR(err);
>>   	}
>>   
>> -	err = btf_ext_copy_line_info(btf_ext, data, size, err_log);
>> +	err = btf_ext_copy_line_info(btf_ext, data, size);
>>   	if (err) {
>>   		btf_ext__free(btf_ext);
>>   		return ERR_PTR(err);
> 
> - Arnaldo
> 

Re: [PATCH bpf-next v2 1/3] tools/bpf: move libbpf pr_* debug print functions to headers

[Arnaldo Carvalho de Melo]

Em Fri, Feb 01, 2019 at 11:20:44PM +0000, Yonghong Song escreveu:
> On 2/1/19 3:10 PM, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Feb 01, 2019 at 09:47:31AM -0800, Yonghong Song escreveu:
> >> @@ -698,13 +695,13 @@ struct btf_ext *btf_ext__new(__u8 *data, __u32 size, btf_print_fn_t err_log)
> >>   	if (!btf_ext)
> >>   		return ERR_PTR(-ENOMEM);
> >>   
> >> -	err = btf_ext_copy_func_info(btf_ext, data, size, err_log);
> >> +	err = btf_ext_copy_func_info(btf_ext, data, size);
> >>   	if (err) {
> >>   		btf_ext__free(btf_ext);
> > 
> > One thing I noticed whas that the class + __ + method is not being
> > consistently followed, will this be dealt with in a followup patch, i.e.
> > to make this consistently use the format used in the
> > btf_ext__free(btf_ext) case?
> 
> Currently, the API functions have both +_+ and +__+ method names.
> The +_+ is for APIs having a close one-to-one mapping
> to system calls. The +__+ is for APIs a little bit high level.
> Did you find any particular method whose format is not quite right?

Nope, and thanks for the clarification, if I find something else I'll
get in touch,

- Arnaldo