From: Arnaldo CArvalho de Melo <acme@kernel•org>
To: Yonghong Song <yhs@fb•com>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail•com>,
Magnus Karlsson <magnus.karlsson@intel•com>,
"bjorn.topel@intel•com" <bjorn.topel@intel•com>,
"ast@kernel•org" <ast@kernel•org>,
"daniel@iogearbox•net" <daniel@iogearbox•net>,
"netdev@vger•kernel.org" <netdev@vger•kernel.org>,
"jakub.kicinski@netronome•com" <jakub.kicinski@netronome•com>,
"bjorn.topel@gmail•com" <bjorn.topel@gmail•com>,
"qi.z.zhang@intel•com" <qi.z.zhang@intel•com>,
"brouer@redhat•com" <brouer@redhat•com>
Subject: Re: [PATCH bpf-next v3 1/3] libbpf: move pr_*() functions to common header file
Date: Thu, 31 Jan 2019 21:47:10 +0100 [thread overview]
Message-ID: <20190131204710.GA4519@kernel.org> (raw)
In-Reply-To: <44beb7c1-9299-1c43-bd1b-38f13e87cf80@fb.com>
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@intel•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
next prev parent reply other threads:[~2019-01-31 20:47 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-29 15:12 [PATCH bpf-next v3 0/3] libbpf: adding AF_XDP support Magnus Karlsson
2019-01-29 15:12 ` [PATCH bpf-next v3 1/3] libbpf: move pr_*() functions to common header file Magnus Karlsson
2019-01-31 10:04 ` Daniel Borkmann
2019-01-31 10:42 ` Magnus Karlsson
2019-01-31 18:52 ` Alexei Starovoitov
2019-01-31 19:12 ` Yonghong Song
2019-01-31 20:47 ` Arnaldo CArvalho de Melo [this message]
2019-01-31 19:35 ` Y Song
2019-01-29 15:12 ` [PATCH bpf-next v3 2/3] libbpf: add support for using AF_XDP sockets Magnus Karlsson
2019-01-31 10:05 ` Daniel Borkmann
2019-01-31 10:44 ` Magnus Karlsson
2019-01-31 10:29 ` Daniel Borkmann
2019-01-31 13:55 ` Magnus Karlsson
2019-01-31 14:05 ` Maciej Fijalkowski
2019-01-31 14:23 ` Magnus Karlsson
2019-01-29 15:12 ` [PATCH bpf-next v3 3/3] samples/bpf: convert xdpsock to use libbpf for AF_XDP access Magnus Karlsson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190131204710.GA4519@kernel.org \
--to=acme@kernel$(echo .)org \
--cc=alexei.starovoitov@gmail$(echo .)com \
--cc=ast@kernel$(echo .)org \
--cc=bjorn.topel@gmail$(echo .)com \
--cc=bjorn.topel@intel$(echo .)com \
--cc=brouer@redhat$(echo .)com \
--cc=daniel@iogearbox$(echo .)net \
--cc=jakub.kicinski@netronome$(echo .)com \
--cc=magnus.karlsson@intel$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=qi.z.zhang@intel$(echo .)com \
--cc=yhs@fb$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox