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:   Sun, 1 Jul 2018 11:31:47 +0100
From:   Okash Khawaja <osk@fb.com>
To:     Jakub Kicinski <jakub.kicinski@netronome.com>
CC:     Daniel Borkmann <daniel@iogearbox.net>,
        Martin KaFai Lau <kafai@fb.com>,
        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: <20180701103146.GA1388@w1t1fb>
References: <20180622225408.jor7lpvsksnwiiec@kafai-mbp.dhcp.thefacebook.com>
 <20180622163200.20564ec4@cakuba.netronome.com>
 <20180623002639.h4qxy7aakypi6t7b@kafai-mbp.dhcp.thefacebook.com>
 <20180626164820.GA12981@w1t1fb>
 <20180626133133.618af1d3@cakuba.netronome.com>
 <20180626222709.fsznwqauxojhhx7v@kafai-mbp.dhcp.thefacebook.com>
 <20180626153559.0511f709@cakuba.netronome.com>
 <23968d53-2895-f0bc-a38c-5bc99e1846ab@iogearbox.net>
 <20180627114737.GA1580@w1t1fb>
 <0360a47f-1056-3117-31ec-7c3f5d6dbccb@iogearbox.net>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <0360a47f-1056-3117-31ec-7c3f5d6dbccb@iogearbox.net>
User-Agent: Mutt/1.9.5 (2018-04-13)
Received-SPF: None (protection.outlook.com: fb.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Jul 2018 09:31:43.8299 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: cee3b16c-3cd9-415b-e9e3-08d5df35762c
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR15MB2508
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-07-01_03:,,
 signatures=0
X-Proofpoint-Spam-Reason: safe
X-FB-Internal: Safe
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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,
                        ...
                }
        }
}]

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.

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?

thanks,
okash