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 12:29 PM Xin Long <lucien.xin@gmail.com> wrote:
>
> 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.
Hi, Stephen and David A.

This patchset is in "deferred" status for a long time.
What should we do about this one?
should I improve something then repost or the lastest one will be fine.

Thanks.