netdev - Re: [PATCH bpf-next v2 3/3] tools/bpf: simplify libbpf API function libbpf_set

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <7e98554c-da96-107e-fff9-721753ce6655@fb.com>
Date:   Fri, 1 Feb 2019 20:37:08 +0000
From:   Yonghong Song <yhs@...com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
CC:     Arnaldo Carvalho de Melo <acme@...hat.com>,
        Magnus Karlsson <magnus.karlsson@...el.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next v2 3/3] tools/bpf: simplify libbpf API function
 libbpf_set_print()



On 2/1/19 11:02 AM, Andrii Nakryiko wrote:
> On Fri, Feb 1, 2019 at 10:16 AM Yonghong Song <yhs@...com> wrote:
>>
>> Currently, the libbpf API function libbpf_set_print()
>> takes three function pointer parameters for warning, info
>> and debug printout respectively.
>>
>> This patch changes the API to have just one function pointer
>> parameter and the function pointer has one additional
>> parameter "debugging level". So if in the future, if
>> the debug level is increased, the function signature
>> won't change.
>>
>> Signed-off-by: Yonghong Song <yhs@...com>
>> ---
>>   tools/lib/bpf/libbpf.c                        | 28 ++++-----------
>>   tools/lib/bpf/libbpf.h                        | 14 +++-----
>>   tools/lib/bpf/test_libbpf.cpp                 |  2 +-
>>   tools/perf/util/bpf-loader.c                  | 32 +++++++----------
>>   tools/testing/selftests/bpf/test_btf.c        |  7 ++--
>>   .../testing/selftests/bpf/test_libbpf_open.c  | 36 +++++++++----------
>>   tools/testing/selftests/bpf/test_progs.c      | 20 +++++++++--
>>   7 files changed, 63 insertions(+), 76 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 1b1c0b504d25..d2337a179837 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -54,8 +54,8 @@
>>
>>   #define __printf(a, b) __attribute__((format(printf, a, b)))
>>
>> -__printf(1, 2)
>> -static int __base_pr(const char *format, ...)
>> +__printf(2, 3)
>> +static int __base_pr(enum libbpf_print_level level, const char *format, ...)
>>   {
>>          va_list args;
>>          int err;
>> @@ -66,17 +66,11 @@ static int __base_pr(const char *format, ...)
>>          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;
>> +static __printf(2, 3) libbpf_print_fn_t __libbpf_pr = __base_pr;
>>
>> -void libbpf_set_print(libbpf_print_fn_t warn,
>> -                     libbpf_print_fn_t info,
>> -                     libbpf_print_fn_t debug)
>> +void libbpf_set_print(libbpf_print_fn_t fn)
>>   {
>> -       __pr_warning = warn;
>> -       __pr_info = info;
>> -       __pr_debug = debug;
>> +       __libbpf_pr = fn;
>>   }
>>
>>   __printf(2, 3)
>> @@ -85,16 +79,8 @@ void libbpf_debug_print(enum libbpf_print_level level, const char *format, ...)
>>          va_list args;
>>
>>          va_start(args, format);
>> -       if (level == LIBBPF_WARN) {
>> -               if (__pr_warning)
>> -                       __pr_warning(format, args);
>> -       } else if (level == LIBBPF_INFO) {
>> -               if (__pr_info)
>> -                       __pr_info(format, args);
>> -       } else {
>> -               if (__pr_debug)
>> -                       __pr_debug(format, args);
>> -       }
>> +       if (__libbpf_pr)
> 
> If __libbpf_pr is NULL, is there a need to call va_start/va_end? If
> not, should they be moved inside if's body?

You are right. Will fix this in the next version.

> 
>> +               __libbpf_pr(level, format, args);
>>          va_end(args);
>>   }
>>