* [PATCH bpf-next] libbpf: fix libbpf_print
@ 2019-02-05 0:20 Stanislav Fomichev
2019-02-05 0:37 ` Yonghong Song
0 siblings, 1 reply; 4+ messages in thread
From: Stanislav Fomichev @ 2019-02-05 0:20 UTC (permalink / raw)
To: netdev; +Cc: yhs, davem, ast, daniel, Stanislav Fomichev
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>
---
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()
--
2.20.1.611.gfbb209baf1-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] libbpf: fix libbpf_print
2019-02-05 0:20 [PATCH bpf-next] libbpf: fix libbpf_print Stanislav Fomichev
@ 2019-02-05 0:37 ` Yonghong Song
2019-02-05 1:51 ` Alexei Starovoitov
0 siblings, 1 reply; 4+ messages in thread
From: Yonghong Song @ 2019-02-05 0:37 UTC (permalink / raw)
To: Stanislav Fomichev, netdev; +Cc: davem, ast, daniel
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()
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] libbpf: fix libbpf_print
2019-02-05 0:37 ` Yonghong Song
@ 2019-02-05 1:51 ` Alexei Starovoitov
2019-02-05 3:23 ` Yonghong Song
0 siblings, 1 reply; 4+ messages in thread
From: Alexei Starovoitov @ 2019-02-05 1:51 UTC (permalink / raw)
To: Yonghong Song; +Cc: Stanislav Fomichev, netdev, davem, ast, daniel
On Tue, Feb 05, 2019 at 12:37:29AM +0000, Yonghong Song wrote:
>
>
> 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>
argh.
Applied. Thanks for the fix.
Yonghong, how was the earlier patch set tested?
It sounds that nothing should have worked.
How perf changes were tested?
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] libbpf: fix libbpf_print
2019-02-05 1:51 ` Alexei Starovoitov
@ 2019-02-05 3:23 ` Yonghong Song
0 siblings, 0 replies; 4+ messages in thread
From: Yonghong Song @ 2019-02-05 3:23 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: Stanislav Fomichev, netdev, davem, ast, daniel
On 2/4/19 5:51 PM, Alexei Starovoitov wrote:
> On Tue, Feb 05, 2019 at 12:37:29AM +0000, Yonghong Song wrote:
>>
>>
>> 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>
>
> argh.
> Applied. Thanks for the fix.
> Yonghong, how was the earlier patch set tested?
Before the global function patch set, I have a global variable version,
which I tested and worked. Later on when changing to global
libbpf_print approach, I tested it through test_btf. That is why
I found a missing check for LIBBPF_DEBUG level in default __base_pr.
But I have to admit that I probably did not pay attention to contents
somehow so I missed the garbled output.
> It sounds that nothing should have worked.
> How perf changes were tested?
I only tested compilation. The context is similar to a few other
bpf selftests programs and I assume with similar implementation, the
result should be similar. But really badly, they are all incorrect :-(
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-02-05 3:24 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-05 0:20 [PATCH bpf-next] libbpf: fix libbpf_print Stanislav Fomichev
2019-02-05 0:37 ` Yonghong Song
2019-02-05 1:51 ` Alexei Starovoitov
2019-02-05 3:23 ` Yonghong Song
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.