Re: [PATCHv3 iproute2-next 3/7] iproute_lwtunnel: add options support for erspan metadata

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 17 Feb 2020 12:53:14 -0700
David Ahern <dsahern@gmail.com> wrote:

> On 2/15/20 11:38 PM, Xin Long wrote:
> > On Sun, Feb 16, 2020 at 12:51 AM David Ahern <dsahern@gmail.com> wrote:  
> >>
> >> On 2/14/20 9:18 PM, Xin Long wrote:  
> >>> On Sat, Feb 15, 2020 at 8:21 AM Stephen Hemminger
> >>> <stephen@networkplumber.org> wrote:  
> >>>>
> >>>> On Sat, 15 Feb 2020 01:40:27 +0800
> >>>> Xin Long <lucien.xin@gmail.com> wrote:
> >>>>  
> >>>>> This's not gonna work. as the output will be:
> >>>>> {"ver":"0x2","idx":"0","dir":"0x1","hwid":"0x2"}  (string)
> >>>>> instead of
> >>>>> {"ver":2,"index":0,"dir":1,"hwid":2} (number)  
> >>>>
> >>>> JSON is typeless. Lots of values are already printed in hex  
> >>> You may mean JSON data itself is typeless.
> >>> But JSON objects are typed when parsing JSON data, which includes
> >>> string, number, array, boolean. So it matters how to define the
> >>> members' 'type' in JSON data.
> >>>
> >>> For example, in python's 'json' module:
> >>>
> >>> #!/usr/bin/python2
> >>> import json
> >>> json_data_1 = '{"ver":"0x2","idx":"0","dir":"0x1","hwid":"0x2"}'
> >>> json_data_2 = '{"ver":2,"index":0,"dir":1,"hwid":2}'
> >>> parsed_json_1 = (json.loads(json_data_1))
> >>> parsed_json_2 = (json.loads(json_data_2))
> >>> print type(parsed_json_1["hwid"])
> >>> print type(parsed_json_2["hwid"])
> >>>
> >>> The output is:
> >>> <type 'unicode'>
> >>> <type 'int'>
> >>>
> >>> Also, '{"result": true}' is different from '{"result": "true"}' when
> >>> loading it in a 3rd-party lib.
> >>>
> >>> I think the JSON data coming from iproute2 is designed to be used by
> >>> a 3rd-party lib to parse, not just to show to users. To keep these
> >>> members' original type (numbers) is more appropriate, IMO.
> >>>  
> >>
> >> Stephen: why do you think all of the numbers should be in hex?
> >>
> >> It seems like consistency with existing output should matter more.
> >> ip/link_gre.c for instance prints index as an int, version as an int,
> >> direction as a string and only hwid in hex.
> >>
> >> Xin: any reason you did not follow the output of the existingg netdev
> >> based solutions?  
> > Hi David,
> > 
> > Option is expressed as "version:index:dir:hwid", I made all fields
> > in this string of hex, just like "class:type:data" in:
> > 
> > commit 0ed5269f9e41f495c8e9020c85f5e1644c1afc57
> > Author: Simon Horman <simon.horman@netronome.com>
> > Date:   Tue Jun 26 21:39:37 2018 -0700
> > 
> >     net/sched: add tunnel option support to act_tunnel_key
> > 
> > I'm not sure if it's good to mix multiple types in this string. wdyt?
> > 
> > but for the JSON data, of course, these are all numbers(not string).
> >   
> 
> I don't understand why Stephen is pushing for hex; it does not make
> sense for version, index or direction. I don't have a clear
> understanding of hwid to know uint vs hex, so your current JSON prints
> seem fine.
> 
> As for the stdout print and hex fields, staring at the tc and lwtunnel
> code, it seems like those 2 have a lot of parallels in expressing
> options for encoding vs lwtunnel and netdev based code. ie., I think
> this latest set is correct.
> 
> Stephen?

I just wanted:
1. The parse and print functions should have the same formats.
I.e. if you take the output and do a little massaging of the ifindex
it should be accepted as an input set of parameters.

2. As much as possible, the JSON and non-JSON output should be similar.
If non-JSON prints in hex, then JSON should display hex and vice/versa.

Ideally all inputs would be human format (not machine formats like hex).
But I guess the mistake was already made with some of the other tunnels.