diff mbox series

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

Message ID 1548774737-16579-2-git-send-email-magnus.karlsson@intel.com
State Changes Requested
Delegated to: BPF Maintainers
Headers show
Series libbpf: adding AF_XDP support | expand

Commit Message

Magnus Karlsson Jan. 29, 2019, 3:12 p.m. UTC 
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

Comments

Daniel Borkmann Jan. 31, 2019, 10:04 a.m. UTC | #1 
On 01/29/2019 04:12 PM, 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__)
> -
>  void libbpf_set_print(libbpf_print_fn_t warn,
>  		      libbpf_print_fn_t info,
>  		      libbpf_print_fn_t debug)
> @@ -96,7 +69,6 @@ void libbpf_set_print(libbpf_print_fn_t warn,
>  		goto out;		\
>  } while(0)
>  
> -
>  /* Copied from tools/perf/util/util.h */
>  #ifndef zfree
>  # define zfree(ptr) ({ free(*ptr); *ptr = NULL; })
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> new file mode 100644
> index 0000000..951c078
> --- /dev/null
> +++ b/tools/lib/bpf/libbpf_internal.h

Just really minor nit: lets name the header utils.h or such. It would also make
sense to move the zfree() and zclose() to this and make use of it.

> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> +
> +/*
> + * Common internal libbpf functions and definitions.
> + *
> + * Copyright (C) 2013-2015 Alexei Starovoitov <ast@kernel.org>
> + * Copyright (C) 2015 Wang Nan <wangnan0@huawei.com>
> + * Copyright (C) 2015 Huawei Inc.
> + */
> +#ifndef __LIBBPF_LIBBPF_INTERNAL_H
> +#define __LIBBPF_LIBBPF_INTERNAL_H
> +
> +#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 __maybe_unused __printf(1, 2) libbpf_print_fn_t __pr_warning = __base_pr;
> +static __maybe_unused __printf(1, 2) libbpf_print_fn_t __pr_info = __base_pr;
> +static __maybe_unused __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__)
> +
> +#endif /* __LIBBPF_LIBBPF_INTERNAL_H */
>
Magnus Karlsson Jan. 31, 2019, 10:42 a.m. UTC | #2 
On Thu, Jan 31, 2019 at 11:07 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 01/29/2019 04:12 PM, 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__)
> > -
> >  void libbpf_set_print(libbpf_print_fn_t warn,
> >                     libbpf_print_fn_t info,
> >                     libbpf_print_fn_t debug)
> > @@ -96,7 +69,6 @@ void libbpf_set_print(libbpf_print_fn_t warn,
> >               goto out;               \
> >  } while(0)
> >
> > -
> >  /* Copied from tools/perf/util/util.h */
> >  #ifndef zfree
> >  # define zfree(ptr) ({ free(*ptr); *ptr = NULL; })
> > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> > new file mode 100644
> > index 0000000..951c078
> > --- /dev/null
> > +++ b/tools/lib/bpf/libbpf_internal.h
>
> Just really minor nit: lets name the header utils.h or such. It would also make
> sense to move the zfree() and zclose() to this and make use of it.

Will fix.

Thanks: Magnus

> > @@ -0,0 +1,41 @@
> > +/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> > +
> > +/*
> > + * Common internal libbpf functions and definitions.
> > + *
> > + * Copyright (C) 2013-2015 Alexei Starovoitov <ast@kernel.org>
> > + * Copyright (C) 2015 Wang Nan <wangnan0@huawei.com>
> > + * Copyright (C) 2015 Huawei Inc.
> > + */
> > +#ifndef __LIBBPF_LIBBPF_INTERNAL_H
> > +#define __LIBBPF_LIBBPF_INTERNAL_H
> > +
> > +#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 __maybe_unused __printf(1, 2) libbpf_print_fn_t __pr_warning = __base_pr;
> > +static __maybe_unused __printf(1, 2) libbpf_print_fn_t __pr_info = __base_pr;
> > +static __maybe_unused __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__)
> > +
> > +#endif /* __LIBBPF_LIBBPF_INTERNAL_H */
> >
>
Alexei Starovoitov Jan. 31, 2019, 6:52 p.m. UTC | #3 
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 ?
Yonghong Song Jan. 31, 2019, 7:12 p.m. UTC | #4 
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 ?

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?
Y Song Jan. 31, 2019, 7:35 p.m. UTC | #5 
On Tue, Jan 29, 2019 at 7:13 AM Magnus Karlsson
<magnus.karlsson@intel.com> 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__)
> -
>  void libbpf_set_print(libbpf_print_fn_t warn,
>                       libbpf_print_fn_t info,
>                       libbpf_print_fn_t debug)
> @@ -96,7 +69,6 @@ void libbpf_set_print(libbpf_print_fn_t warn,
>                 goto out;               \
>  } while(0)
>
> -
>  /* Copied from tools/perf/util/util.h */
>  #ifndef zfree
>  # define zfree(ptr) ({ free(*ptr); *ptr = NULL; })
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> new file mode 100644
> index 0000000..951c078
> --- /dev/null
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> +
> +/*
> + * Common internal libbpf functions and definitions.
> + *
> + * Copyright (C) 2013-2015 Alexei Starovoitov <ast@kernel.org>
> + * Copyright (C) 2015 Wang Nan <wangnan0@huawei.com>
> + * Copyright (C) 2015 Huawei Inc.
> + */
> +#ifndef __LIBBPF_LIBBPF_INTERNAL_H
> +#define __LIBBPF_LIBBPF_INTERNAL_H
> +
> +#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;
> +}

Hmm. This will introduce this static function into every .c file.

Maybe we could define a global function in libbpf.c like
  libbpf_debug_pr(level, ...)
This function will be internal to the library and implemented in libbpf.c.
All files can call this function to print out the debug information
based on level?

> +
> +static __maybe_unused __printf(1, 2) libbpf_print_fn_t __pr_warning = __base_pr;
> +static __maybe_unused __printf(1, 2) libbpf_print_fn_t __pr_info = __base_pr;
> +static __maybe_unused __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__)
> +
> +#endif /* __LIBBPF_LIBBPF_INTERNAL_H */
> --
> 2.7.4
>
Arnaldo Carvalho de Melo Jan. 31, 2019, 8:47 p.m. UTC | #6 
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
diff mbox series

Patch

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__)
-
 void libbpf_set_print(libbpf_print_fn_t warn,
 		      libbpf_print_fn_t info,
 		      libbpf_print_fn_t debug)
@@ -96,7 +69,6 @@  void libbpf_set_print(libbpf_print_fn_t warn,
 		goto out;		\
 } while(0)
 
-
 /* Copied from tools/perf/util/util.h */
 #ifndef zfree
 # define zfree(ptr) ({ free(*ptr); *ptr = NULL; })
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
new file mode 100644
index 0000000..951c078
--- /dev/null
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -0,0 +1,41 @@ 
+/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
+
+/*
+ * Common internal libbpf functions and definitions.
+ *
+ * Copyright (C) 2013-2015 Alexei Starovoitov <ast@kernel.org>
+ * Copyright (C) 2015 Wang Nan <wangnan0@huawei.com>
+ * Copyright (C) 2015 Huawei Inc.
+ */
+#ifndef __LIBBPF_LIBBPF_INTERNAL_H
+#define __LIBBPF_LIBBPF_INTERNAL_H
+
+#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 __maybe_unused __printf(1, 2) libbpf_print_fn_t __pr_warning = __base_pr;
+static __maybe_unused __printf(1, 2) libbpf_print_fn_t __pr_info = __base_pr;
+static __maybe_unused __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__)
+
+#endif /* __LIBBPF_LIBBPF_INTERNAL_H */