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

From: Quentin Monnet <quentin.monnet@netronome.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Andrii Nakryiko <andriin@fb.com>,
	Networking <netdev@vger.kernel.org>,
	bpf@vger.kernel.org, Alexei Starovoitov <ast@fb.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH v2 bpf-next 10/12] bpftool: add C output format option to btf dump subcommand
Date: Fri, 24 May 2019 20:42:01 +0100	[thread overview]
Message-ID: <72fbdb59-4b3b-0e7f-20e1-2ced103fdc46@netronome.com> (raw)
In-Reply-To: <CAEf4Bza9ikV+SnBOE-h8J7ggw--1M3L8ak-VQ6-RxO71x0YUhw@mail.gmail.com>

2019-05-24 10:14 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com>
> On Fri, May 24, 2019 at 2:14 AM Quentin Monnet
> <quentin.monnet@netronome.com> wrote:
>>
>> Hi Andrii,
>>
>> Some nits inline, nothing blocking though.
>>
>> 2019-05-23 13:42 UTC-0700 ~ Andrii Nakryiko <andriin@fb.com>
>>> Utilize new libbpf's btf_dump API to emit BTF as a C definitions.
>>>
>>> Signed-off-by: Andrii Nakryiko <andriin@fb.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!