Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality

[Jakub Kicinski]

On Sun, 1 Jul 2018 11:31:47 +0100, Okash Khawaja wrote:
> On Wed, Jun 27, 2018 at 02:56:49PM +0200, Daniel Borkmann wrote:
> > On 06/27/2018 01:47 PM, Okash Khawaja wrote:  
> > > On Wed, Jun 27, 2018 at 12:34:35PM +0200, Daniel Borkmann wrote:  
> > >> On 06/27/2018 12:35 AM, Jakub Kicinski wrote:  
> > >>> On Tue, 26 Jun 2018 15:27:09 -0700, Martin KaFai Lau wrote:  
> > >>>> On Tue, Jun 26, 2018 at 01:31:33PM -0700, Jakub Kicinski wrote:  
> > >> [...]  
> > >>>>> Implementing both outputs in one series will help you structure your
> > >>>>> code to best suit both of the formats up front.    
> > >>>> hex and "formatted" are the only things missing?  As always, things
> > >>>> can be refactored when new use case comes up.  Lets wait for
> > >>>> Okash input.
> > >>>>
> > >>>> Regardless, plaintext is our current use case.  Having the current
> > >>>> patchset in does not stop us or others from contributing other use
> > >>>> cases (json, "bpftool map find"...etc),  and IMO it is actually
> > >>>> the opposite.  Others may help us get there faster than us alone.
> > >>>> We should not stop making forward progress and take this patch
> > >>>> as hostage because "abc" and "xyz" are not done together.  
> > >>>
> > >>> Parity between JSON and plain text output is non negotiable.  
> > >>
> > >> Longish discussion and some confusion in this thread. :-) First of all
> > >> thanks a lot for working on it, very useful!   
> > > Thanks :)
> > >   
> > >> My $0.02 on it is that so far
> > >> great care has been taken in bpftool to indeed have feature parity between
> > >> JSON and plain text, so it would be highly desirable to keep continuing
> > >> this practice if the consensus is that it indeed is feasible and makes
> > >> sense wrt BTF data. There has been mentioned that given BTF data can be
> > >> dynamic depending on what the user loads via bpf(2) so a potential JSON
> > >> output may look different/break each time anyway. This however could all be
> > >> embedded under a container object that has a fixed key like 'formatted'
> > >> where tools like jq(1) can query into it. I think this would be fine since
> > >> the rest of the (non-dynamic) output is still retained as-is and then
> > >> wouldn't confuse or collide with existing users, and anyone programmatically
> > >> parsing deeper into the BTF data under such JSON container object needs
> > >> to have awareness of what specific data it wants to query from it; so
> > >> there's no conflict wrt breaking anything here. Imho, both outputs would
> > >> be very valuable.  
> > > Okay I can add "formatted" object under json output.
> > > 
> > > One thing to note here is that the fixed output will change if the map
> > > itself changes. So someone writing a program that consumes that fixed
> > > output will have to account for his program breaking in future, thus  
> > 
> > Yes, that aspect is fine though, any program/script parsing this would need
> > to be aware of the underlying map type to make sense of it (e.g. per-cpu vs
> > non per-cpu maps to name one). But that info it could query/verify already
> > beforehand via bpftool as well (via normal map info dump for a given id).
> >   
> > > breaking backward compatibility anyway as far as the developer is
> > > concerned :)
> > > 
> > > I will go ahead with work on "formatted" object.  
> > 
> > Cool, thanks,
> > Daniel  
> 
> 
> hi,
> 
> couple of questions:
> 
> 1. just to be sure, formatted section will be on the same level as "key"
> and "value"? so something like following:
> 
> 
> $ bpftool map dump -p id 8
> [{
>         "key": ["0x00","0x00","0x00","0x00"
>         ],
>         "value": [...
>         ],
>         "formatted": {
>                 "key": 0,
>                 "value": {
>                         "int_field":  3,
>                         "pointerfield": 2152930552,
>                         ...
>                 }
>         }
> }]

Looks good, yes!

> 2. i noticed that the ouput in v1 has all the keys and values on the
> same level. in v2, i'll change them so that each key-value pair is a
> separate object. let me know what you think.

For non-JSON output?  No preference, whatever looks better :)  Empty
line between key/value pairs to visually separate them could also
work.  But up to you.

> finally, i noticed there is a map lookup command which also prints map
> entries. do want that to also be btf-printed in this patchset?

It would be nice to share the printing code for the two, yes.