Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 21 Jun 2018 15:51:17 -0700
From:   Martin KaFai Lau <kafai@fb.com>
To:     Jakub Kicinski <jakub.kicinski@netronome.com>
CC:     Okash Khawaja <osk@fb.com>, Daniel Borkmann <daniel@iogearbox.net>,
        "Alexei Starovoitov" <ast@kernel.org>, Yonghong Song <yhs@fb.com>,
        Quentin Monnet <quentin.monnet@netronome.com>,
        "David S. Miller" <davem@davemloft.net>, <netdev@vger.kernel.org>,
        <kernel-team@fb.com>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH bpf-next 2/3] bpf: btf: add btf json print functionality
Message-ID: <20180621225117.dhrkrtmkfbeihbe4@kafai-mbp.dhcp.thefacebook.com>
References: <20180620203051.223156973@fb.com>
 <20180620203703.101156292@fb.com>
 <20180621145935.41ff8974@cakuba.netronome.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20180621145935.41ff8974@cakuba.netronome.com>
User-Agent: NeoMutt/20180512
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jun 21, 2018 at 02:59:35PM -0700, Jakub Kicinski wrote:
> On Wed, 20 Jun 2018 13:30:53 -0700, Okash Khawaja wrote:
> > $ sudo bpftool map dump -p id 14
> > [{
> >         "key": 0
> >     },{
> >         "value": {
> >             "m": 1,
> >             "n": 2,
> >             "o": "c",
> >             "p": [15,16,17,18,15,16,17,18
> >             ],
> >             "q": [[25,26,27,28,25,26,27,28
> >                 ],[35,36,37,38,35,36,37,38
> >                 ],[45,46,47,48,45,46,47,48
> >                 ],[55,56,57,58,55,56,57,58
> >                 ]
> >             ],
> >             "r": 1,
> >             "s": 0x7ffff6f70568,
> >             "t": {
> >                 "x": 5,
> >                 "y": 10
> >             },
> >             "u": 100,
> >             "v": 20,
> >             "w1": 0x7,
> >             "w2": 0x3
> >         }
> >     }
> > ]
> 
> I don't think this format is okay, JSON output is an API you shouldn't
> break.  You can change the non-JSON output whatever way you like, but
> JSON must remain backwards compatible.
> 
> The dump today has object per entry, e.g.:
> 
> {
>         "key":["0x00","0x00","0x00","0x00",
>         ],
>         "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
>         ]
> }
> 
> This format must remain, you may only augment it with new fields.  E.g.:
> 
> {
>         "key":["0x00","0x00","0x00","0x00",
>         ],
> 	"key_struct":{
> 		"index":0
> 	},
>         "value": ["0x02","0x00","0x00","0x00","0x00","0x00","0x00","0x00"
>         ],
> 	"value_struct":{
> 		"src_ip":2,
> 		"dst_ip:0
> 	}
> }
I am not sure how useful to have both "key|value" and "(key|value)_struct"
while most people would prefer "key_struct"/"value_struct" if it is
available.

How about introducing a new option, like "-b", to print the
map with BTF (if available) such that it won't break the existing
one (-j or -p) while the "-b" output can keep using the "key"
and "value".

The existing json can be kept as is.

> 
> The name XYZ_struct may not be the best, perhaps you can come up with a
> better one?  
> 
> Does that make sense?  Am I missing what you're doing here?
> 
> One process note - please make sure you run checkpatch.pl --strict on
> bpftool patches before posting.
> 
> Thanks for working on this!