netdev - Re: [PATCH v2 bpf-next 10/12] bpftool: add C output format option to btf dump subcommand

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: <72fbdb59-4b3b-0e7f-20e1-2ced103fdc46@netronome.com>
Date:   Fri, 24 May 2019 20:42:01 +0100
From:   Quentin Monnet <quentin.monnet@...ronome.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Andrii Nakryiko <andriin@...com>,
        Networking <netdev@...r.kernel.org>, bpf@...r.kernel.org,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 10/12] bpftool: add C output format option to
 btf dump subcommand

2019-05-24 10:14 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@...il.com>
> On Fri, May 24, 2019 at 2:14 AM Quentin Monnet
> <quentin.monnet@...ronome.com> wrote:
>>
>> Hi Andrii,
>>
>> Some nits inline, nothing blocking though.
>>
>> 2019-05-23 13:42 UTC-0700 ~ Andrii Nakryiko <andriin@...com>
>>> Utilize new libbpf's btf_dump API to emit BTF as a C definitions.
>>>
>>> Signed-off-by: Andrii Nakryiko <andriin@...com>
>>> ---
>>>  tools/bpf/bpftool/btf.c | 74 +++++++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 72 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
>>> index a22ef6587ebe..1cdbfad42b38 100644
>>> --- a/tools/bpf/bpftool/btf.c
>>> +++ b/tools/bpf/bpftool/btf.c
>>> @@ -340,11 +340,48 @@ static int dump_btf_raw(const struct btf *btf,
>>>       return 0;
>>>  }
>>>
>>> +static void btf_dump_printf(void *ctx, const char *fmt, va_list args)
>>
>> Nit: This function could have a printf attribute ("__printf(2, 0)").
> 
> added, though I don't think it matters as it's only used as a callback function.

Thanks. Yes, true... But the attribute does not hurt, and we have it
case it changes in the future and the function is reused. Ok, unlikely,
but...

> 
>>
>>> +{
>>> +     vfprintf(stdout, fmt, args);
>>> +}
>>> +


>>> @@ -431,6 +468,29 @@ static int do_dump(int argc, char **argv)
>>>               goto done;
>>>       }
>>>
>>> +     while (argc) {
>>> +             if (is_prefix(*argv, "format")) {
>>> +                     NEXT_ARG();
>>> +                     if (argc < 1) {
>>> +                             p_err("expecting value for 'format' option\n");
>>> +                             goto done;
>>> +                     }
>>> +                     if (strcmp(*argv, "c") == 0) {
>>> +                             dump_c = true;
>>> +                     } else if (strcmp(*argv, "raw") == 0) {
>>
>> Do you think we could use is_prefix() instead of strcmp() here?
> 
> So I considered it, and then decided against it, though I can still be
> convinced otherwise. Right now we have raw and c, but let's say we add
> rust as an option. r will become ambiguous, but actually will be
> resolved to whatever we check first: either raw or rust, which is not
> great. So given that those format specifiers will tend to be short, I
> decided it's ok to require to specify them fully. Does it make sense?

It does make sense. I thought about that too. I think I would add prefix
handling anyway, especially because "raw" is the default so it makes
sense defaulting to it in case there is a collision in the future. This
is what happens already between "bpftool prog" and "bpftool perf". But I
don't really mind, so ok, let's keep the full keyword for now if you prefer.

> 
>>
>>> +                             dump_c = false;
>>> +                     } else {
>>> +                             p_err("unrecognized format specifier: '%s'",
>>> +                                   *argv);
>>
>> Would it be worth reminding the user about the valid specifiers in that
>> message? (But then we already have it in do_help(), so maybe not.)
> 
> Added possible options to the message.

Cool, thanks!