On Fri, Feb 1, 2019 at 10:16 AM Yonghong Song <y...@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 <y...@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
>
