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

[Xin Long <lucien.xin@gmail.com>]

On Tue, Feb 18, 2020 at 5:03 AM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> 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.
I guess we can't 'fix' these in other tunnels in tc.

So I'm thinking we can either use the latest patchset,
or keep the geneve opts format in lwtunnel consistent
with the geneve opts in tc only and parse all with
unint in the new erspan/vxlan tunnels opts.