netdev - Re: [PATCH bpf-next v3 1/3] libbpf: move pr_*() functions to common header file

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 31 Jan 2019 21:47:10 +0100
From:   Arnaldo CArvalho de Melo <acme@...nel.org>
To:     Yonghong Song <yhs@...com>
Cc:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Magnus Karlsson <magnus.karlsson@...el.com>,
        "bjorn.topel@...el.com" <bjorn.topel@...el.com>,
        "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "jakub.kicinski@...ronome.com" <jakub.kicinski@...ronome.com>,
        "bjorn.topel@...il.com" <bjorn.topel@...il.com>,
        "qi.z.zhang@...el.com" <qi.z.zhang@...el.com>,
        "brouer@...hat.com" <brouer@...hat.com>
Subject: Re: [PATCH bpf-next v3 1/3] libbpf: move pr_*() functions to common
 header file

Em Thu, Jan 31, 2019 at 07:12:33PM +0000, Yonghong Song escreveu:
> 
> 
> On 1/31/19 10:52 AM, Alexei Starovoitov wrote:
> > On Tue, Jan 29, 2019 at 04:12:15PM +0100, Magnus Karlsson wrote:
> >> Move the pr_*() functions in libbpf.c to a common header file called
> >> libbpf_internal.h. This so that the later libbpf AF_XDP helper library
> >> code in xsk.c can use these printing functions too.
> >>
> >> Signed-off-by: Magnus Karlsson <magnus.karlsson@...el.com>
> >> ---
> >>   tools/lib/bpf/libbpf.c          | 30 +-----------------------------
> >>   tools/lib/bpf/libbpf_internal.h | 41 +++++++++++++++++++++++++++++++++++++++++
> >>   2 files changed, 42 insertions(+), 29 deletions(-)
> >>   create mode 100644 tools/lib/bpf/libbpf_internal.h
> >>
> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >> index 2ccde17..1d7fe26 100644
> >> --- a/tools/lib/bpf/libbpf.c
> >> +++ b/tools/lib/bpf/libbpf.c
> >> @@ -39,6 +39,7 @@
> >>   #include <gelf.h>
> >>   
> >>   #include "libbpf.h"
> >> +#include "libbpf_internal.h"
> >>   #include "bpf.h"
> >>   #include "btf.h"
> >>   #include "str_error.h"
> >> @@ -51,34 +52,6 @@
> >>   #define BPF_FS_MAGIC		0xcafe4a11
> >>   #endif
> >>   
> >> -#define __printf(a, b)	__attribute__((format(printf, a, b)))
> >> -
> >> -__printf(1, 2)
> >> -static int __base_pr(const char *format, ...)
> >> -{
> >> -	va_list args;
> >> -	int err;
> >> -
> >> -	va_start(args, format);
> >> -	err = vfprintf(stderr, format, args);
> >> -	va_end(args);
> >> -	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;
> >> -
> >> -#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__)
> > 
> > since these funcs are about to be used more widely
> > let's clean this api while we still can.
> > How about we convert it to single pr_log callback function
> > with verbosity flag instead of three callbacks ?

Probably. I just wanted to keep, in the source code, the
pr_{debug,warn,info} APIs, to reduce the learning curve for people used
to program in the kernel and in tools/perf/.

> Another possible change related to the API function
>    libbpf_set_print
> 
> Currently the function takes three parameters,
> 
> LIBBPF_API void libbpf_set_print(libbpf_print_fn_t warn,
>                                   libbpf_print_fn_t info,
>                                   libbpf_print_fn_t debug);
> 
> So it currently supports three level of debugging output.
> Is it possible in the future more debugging output level
> may be supported? if this is the case, maybe we could
> change the API function libbpf_set_print to something like
> the below before the library version bumps into 1.0.0?
> 
>   LIBBPF_API void libbpf_set_print(libbpf_print_fn_t dprint);
> and
>    typedef int (*libbpf_print_fn_t)(enum libbpf_debug_level level,
>                                     const char *, ...)
>          __attribute__((format(printf, 1, 2)));
>    enum libbpf_debug_level {
>      LIBBPF_WARN,
>      LIBBPF_INFO,
>      LIBBPF_DEBUG,
>    };
> 
> Basically, the user provided callback function must have
> the first parameters as the level.
> 
> Any comments? Arnaldo?

I think it is ok, as long as we can override the pr_log thing to allow
for using with TUI, stdio, GUI.

- Arnaldo