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

[Yonghong Song <yhs@fb.com>]

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?