Re: [PATCH bpf-next 0/4] libbpf: add raw BTF type dumping

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, Sep 29, 2020 at 8:18 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Sep 29, 2020 at 05:44:48PM -0700, Andrii Nakryiko wrote:
> > On Tue, Sep 29, 2020 at 5:03 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Tue, Sep 29, 2020 at 04:28:39PM -0700, Andrii Nakryiko wrote:
> > > > Add btf_dump__dump_type_raw() API that emits human-readable low-level BTF type
> > > > information, same as bpftool output. bpftool is not switched to this API
> > > > because bpftool still needs to perform all the same BTF type processing logic
> > > > to do JSON output, so benefits are pretty much zero.
> > >
> > > If the only existing user cannot actually use such api it speaks heavily
> > > against adding such api to libbpf. Comparing strings in tests is nice, but
> > > could be done with C output just as well.
> >
> > It certainly can, it just won't save much code, because bpftool would
> > still need to have a big switch over BTF type kinds to do JSON output.
>
> So you're saying that most of the dump_btf_type() in bpftool/btf.c will stay as-is.
> Only 'if (json_output)' will become unconditional? Hmm.

Yes.

> I know you don't want json in libbpf, but I think it's the point of
> making a call on such things. Either libbpf gets to dump both
> json and text dump_btf_type()-like output or it stays with C only.

Right, I don't think JSON belongs in libbpf. But I fail to see why
this is the point where we need to make such a decision.

> Doing C and this text and not doing json is inconsistent.

Inconsistent with what? I've never found bpftool's raw BTF dump in
JSON format useful. At all. Saying raw BTF dump is useful and
consistent (?) only if it's both human-readable text and JSON makes no
sense to me. Libbpf doesn't have to re-implement entire bpftool
functionality.

> Either libbpf can print btf in many different ways or it stays with C.
> 2nd format is not special in any way.

I don't understand your point. With my patch it now can dump it as
valid C language definition or as a textual low-level BTF
representation.

If you are saying it should emit it in Go format, Rust format, or
other language-specific way, then sure, maybe, but it sure won't
re-use C-specific logic of btf_dump__dump_type() as is, because it is
C language specific. For Go there would be different logic, just as
for any other language. And someone will have to implement it (and
there would need to be a compelling use case for that, of course). And
it will be a different API, or at least a generic API with some enum
specifying "format" (which is the same thing, really, but inferior for
customizability reasons).

But JSON is different from that. It's just a more machine-friendly
output of textual low-level BTF dump. It could have been BSON or YAML,
but I hope you don't suggest to emit in those formats as well.

> I don't think that text and json formats bring much value comparing to C,
> so I would be fine with C only.

Noted. I disagree and find it very useful all the time, it's pretty
much the only way I look at BTF. C output is not complete: it doesn't
show functions, data sections and variables. It's not a replacement
for raw BTF dump. I don't even consider it as a different "format".
It's an entirely different and complementary (not alternative) view
(interpretation) of BTF.

> But if we allow 2nd format we should
> do json at the same time too to save bpftool the hassle.

There is no hassle for bpftool, code is written and working. Libbpf's
goal is not to minimize bpftool code either. So I hear you, but I
don't think about this the same way.

> And in the future we should allow 4th and 5th formats.

Ok, but there is no contradiction with what I'm doing here.



Regardless, feel free to drop patches #2 and #3, but patch #1 fixes
real issue, so would be nice to land it anyways. Patch #4 adds test
for changes in patch #1. Let me know if you want me to respin with
just those 2 patches.