Re: [PATCH bpf-next] libbpf: fix libbpf_print

[Yonghong Song <yhs@fb.com>]

On 2/4/19 4:20 PM, Stanislav Fomichev wrote:
> With the recent print rework we now have the following problem:
> pr_{warning,info,debug} expand to __pr which calls libbpf_print.
> libbpf_print does va_start and calls __libbpf_pr with va_list argument.
> In __base_pr we again do va_start. Because the next argument is a
> va_list, we don't get correct pointer to the argument (and print noting
> in my case, I don't know why it doesn't crash tbh).
> 
> Fix this by changing libbpf_print_fn_t signature to accept va_list and
> remove unneeded calls to va_start in the existing users.
> 
> Alternatively, this can we solved by exporting __libbpf_pr and
> changing __pr macro to (and killing libbpf_print):
> {
> 	if (__libbpf_pr)
> 		__libbpf_pr(level, "libbpf: " fmt, ##__VA_ARGS__)
> }
> 
> Signed-off-by: Stanislav Fomichev <sdf@google.com>

It is my mistake. My early version did passed correctly and later
on I made some changes and did not test properly. Thanks for the fix!

Acked-by: Yonghong Song <yhs@fb.com>


> ---
>   tools/lib/bpf/libbpf.c                         | 14 ++++----------
>   tools/lib/bpf/libbpf.h                         |  3 +--
>   tools/perf/util/bpf-loader.c                   | 10 ++--------
>   tools/testing/selftests/bpf/test_btf.c         | 13 ++-----------
>   tools/testing/selftests/bpf/test_libbpf_open.c | 10 ++--------
>   tools/testing/selftests/bpf/test_progs.c       | 10 ++--------
>   6 files changed, 13 insertions(+), 47 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 84ca6c2bea91..47969aa0faf8 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -54,22 +54,16 @@
>   
>   #define __printf(a, b)	__attribute__((format(printf, a, b)))
>   
> -__printf(2, 3)
> -static int __base_pr(enum libbpf_print_level level, const char *format, ...)
> +static int __base_pr(enum libbpf_print_level level, const char *format,
> +		     va_list args)
>   {
> -	va_list args;
> -	int err;
> -
>   	if (level == LIBBPF_DEBUG)
>   		return 0;
>   
> -	va_start(args, format);
> -	err = vfprintf(stderr, format, args);
> -	va_end(args);
> -	return err;
> +	return vfprintf(stderr, format, args);
>   }
>   
> -static __printf(2, 3) libbpf_print_fn_t __libbpf_pr = __base_pr;
> +static libbpf_print_fn_t __libbpf_pr = __base_pr;
>   
>   void libbpf_set_print(libbpf_print_fn_t fn)
>   {
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 19dbc1bed960..69a7c25eaccc 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -54,8 +54,7 @@ enum libbpf_print_level {
>   };
>   
>   typedef int (*libbpf_print_fn_t)(enum libbpf_print_level level,
> -				 const char *, ...)
> -	__attribute__((format(printf, 2, 3)));
> +				 const char *, va_list ap);
>   
>   LIBBPF_API void libbpf_set_print(libbpf_print_fn_t fn);
>   
> diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> index 38afdbe6a9e0..037d8ff6a634 100644
> --- a/tools/perf/util/bpf-loader.c
> +++ b/tools/perf/util/bpf-loader.c
> @@ -25,15 +25,9 @@
>   #include "c++/clang-c.h"
>   
>   static int libbpf_perf_print(enum libbpf_print_level level __attribute__((unused)),
> -			      const char *fmt, ...)
> +			      const char *fmt, va_list args)
>   {
> -	va_list args;
> -	int ret;
> -
> -	va_start(args, fmt);
> -	ret = veprintf(1, verbose, pr_fmt(fmt), args);
> -	va_end(args);
> -	return ret;
> +	return veprintf(1, verbose, pr_fmt(fmt), args);
>   }
>   
>   struct bpf_prog_priv {
> diff --git a/tools/testing/selftests/bpf/test_btf.c b/tools/testing/selftests/bpf/test_btf.c
> index aebaeff5a5a0..5afab823ffbe 100644
> --- a/tools/testing/selftests/bpf/test_btf.c
> +++ b/tools/testing/selftests/bpf/test_btf.c
> @@ -52,19 +52,10 @@ static int count_result(int err)
>   	return err;
>   }
>   
> -#define __printf(a, b)	__attribute__((format(printf, a, b)))
> -
> -__printf(2, 3)
>   static int __base_pr(enum libbpf_print_level level __attribute__((unused)),
> -		     const char *format, ...)
> +		     const char *format, va_list args)
>   {
> -	va_list args;
> -	int err;
> -
> -	va_start(args, format);
> -	err = vfprintf(stderr, format, args);
> -	va_end(args);
> -	return err;
> +	return vfprintf(stderr, format, args);
>   }
>   
>   #define BTF_INFO_ENC(kind, kind_flag, vlen)			\
> diff --git a/tools/testing/selftests/bpf/test_libbpf_open.c b/tools/testing/selftests/bpf/test_libbpf_open.c
> index b9ff3bf76544..1909ecf4d999 100644
> --- a/tools/testing/selftests/bpf/test_libbpf_open.c
> +++ b/tools/testing/selftests/bpf/test_libbpf_open.c
> @@ -36,19 +36,13 @@ static void usage(char *argv[])
>   
>   static bool debug = 0;
>   static int libbpf_debug_print(enum libbpf_print_level level,
> -			      const char *fmt, ...)
> +			      const char *fmt, va_list args)
>   {
> -	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;
> +	return vfprintf(stderr, fmt, args);
>   }
>   
>   #define EXIT_FAIL_LIBBPF EXIT_FAILURE
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 55d05102e7bf..c52bd90fbb34 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -1785,18 +1785,12 @@ static void test_task_fd_query_tp(void)
>   }
>   
>   static int libbpf_debug_print(enum libbpf_print_level level,
> -			      const char *format, ...)
> +			      const char *format, va_list args)
>   {
> -	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;
> +	return vfprintf(stderr, format, args);
>   }
>   
>   static void test_reference_tracking()
>