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