| Message ID | 290ab5d2dc06b183159d293ab216962a3cc0df6d.1581676056.git.lucien.xin@gmail.com |
|---|---|
| State | Deferred |
| Delegated to: | David Ahern |
| Headers | show |
| Series | iproute2: fully support for geneve/vxlan/erspan options | expand |
On Fri, 14 Feb 2020 18:30:47 +0800 Xin Long <lucien.xin@gmail.com> wrote: > + > + open_json_array(PRINT_JSON, name); > + open_json_object(NULL); > + print_uint(PRINT_JSON, "ver", NULL, ver); > + print_uint(PRINT_JSON, "index", NULL, idx); > + print_uint(PRINT_JSON, "dir", NULL, dir); > + print_uint(PRINT_JSON, "hwid", NULL, hwid); > + close_json_object(); > + close_json_array(PRINT_JSON, name); > + > + print_nl(); > + print_string(PRINT_FP, name, "\t%s ", name); > + sprintf(strbuf, "%02x:%08x:%02x:%02x", ver, idx, dir, hwid); > + print_string(PRINT_FP, NULL, "%s ", strbuf); > +} Instead of having two sets of prints, is it possible to do this print_nl(); print_string(PRINT_FP, NULL, "\t", NULL); open_json_array(PRINT_ANY, name); open_json_object(NULL); print_0xhex(PRINT_ANY, "ver", " %02x", ver); print_0xhex(PRINT_ANY, "idx", ":%08x", idx); print_0xhex(PRINT_ANY, "dir", ":%02x", dir); print_0xhex(PRINT_ANY, "hwid", ":%02x", hwid) close_json_object(); close_json_array(PRINT_ANY, " "); Also, you seem to not hear the request to not use opaque hex values in the iproute2 interface. The version, index, etc should be distinct parameter values not a hex string. I think this still needs more work before merging.
On Sat, Feb 15, 2020 at 12:13 AM Stephen Hemminger <stephen@networkplumber.org> wrote: > > On Fri, 14 Feb 2020 18:30:47 +0800 > Xin Long <lucien.xin@gmail.com> wrote: > > > + > > + open_json_array(PRINT_JSON, name); > > + open_json_object(NULL); > > + print_uint(PRINT_JSON, "ver", NULL, ver); > > + print_uint(PRINT_JSON, "index", NULL, idx); > > + print_uint(PRINT_JSON, "dir", NULL, dir); > > + print_uint(PRINT_JSON, "hwid", NULL, hwid); > > + close_json_object(); > > + close_json_array(PRINT_JSON, name); > > + > > + print_nl(); > > + print_string(PRINT_FP, name, "\t%s ", name); > > + sprintf(strbuf, "%02x:%08x:%02x:%02x", ver, idx, dir, hwid); > > + print_string(PRINT_FP, NULL, "%s ", strbuf); > > +} > > Instead of having two sets of prints, is it possible to do this > print_nl(); > print_string(PRINT_FP, NULL, "\t", NULL); > > open_json_array(PRINT_ANY, name); > open_json_object(NULL); > print_0xhex(PRINT_ANY, "ver", " %02x", ver); > print_0xhex(PRINT_ANY, "idx", ":%08x", idx); > print_0xhex(PRINT_ANY, "dir", ":%02x", dir); > print_0xhex(PRINT_ANY, "hwid", ":%02x", hwid) > close_json_object(); > close_json_array(PRINT_ANY, " "); Hi Stephen, 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) > > Also, you seem to not hear the request to not use opaque hex values > in the iproute2 interface. The version, index, etc should be distinct > parameter values not a hex string. The opts STRING, especially these like "XX:YY:ZZ" are represented as hex string on both adding and dumping. It is to keep consistent with geneve_opts in m_tunnel_key and f_flower, see commit 6217917a382682d8e8a7ecdeb0c6626f701a0933 Author: Simon Horman <simon.horman@netronome.com> Date: Thu Jul 5 17:12:00 2018 -0700 tc: m_tunnel_key: Add tunnel option support to act_tunnel_key and commit 56155d4df86d489c4207444c8a90ce4e0e22e49f Author: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com> Date: Fri Sep 28 16:03:39 2018 +0200 tc: f_flower: add geneve option match support to flower and actually, the code type I'm using is exactly from these 2 patches. please take a look. I don't think this patchset should go to another type of format for opts. Thanks. > > I think this still needs more work before merging.
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
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. Thanks.
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 existing netdev based solutions?
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). Thanks.
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?
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.
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.
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.
On 4/19/20 2:39 AM, Xin Long wrote: > 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. I am fine with this set as is; Stephen had some concerns.
Hi, Stephen, any update? This is your last reply: """ 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. """ Do you still hope to change things like that (just note it won't be consistent with:) 6217917a tc: m_tunnel_key: Add tunnel option support to act_tunnel_key 56155d4d tc: f_flower: add geneve option match support to flower or go with the current set? I'm now okay to do either of them. Thanks. On Mon, Apr 20, 2020 at 6:28 AM David Ahern <dsahern@gmail.com> wrote: > > On 4/19/20 2:39 AM, Xin Long wrote: > > 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. > > I am fine with this set as is; Stephen had some concerns.
On Sat, 15 Feb 2020 01:40:27 +0800 Xin Long <lucien.xin@gmail.com> wrote: > On Sat, Feb 15, 2020 at 12:13 AM Stephen Hemminger > <stephen@networkplumber.org> wrote: > > > > On Fri, 14 Feb 2020 18:30:47 +0800 > > Xin Long <lucien.xin@gmail.com> wrote: > > > > > + > > > + open_json_array(PRINT_JSON, name); > > > + open_json_object(NULL); > > > + print_uint(PRINT_JSON, "ver", NULL, ver); > > > + print_uint(PRINT_JSON, "index", NULL, idx); > > > + print_uint(PRINT_JSON, "dir", NULL, dir); > > > + print_uint(PRINT_JSON, "hwid", NULL, hwid); > > > + close_json_object(); > > > + close_json_array(PRINT_JSON, name); > > > + > > > + print_nl(); > > > + print_string(PRINT_FP, name, "\t%s ", name); > > > + sprintf(strbuf, "%02x:%08x:%02x:%02x", ver, idx, dir, hwid); > > > + print_string(PRINT_FP, NULL, "%s ", strbuf); > > > +} > > > > Instead of having two sets of prints, is it possible to do this > > print_nl(); > > print_string(PRINT_FP, NULL, "\t", NULL); > > > > open_json_array(PRINT_ANY, name); > > open_json_object(NULL); > > print_0xhex(PRINT_ANY, "ver", " %02x", ver); > > print_0xhex(PRINT_ANY, "idx", ":%08x", idx); > > print_0xhex(PRINT_ANY, "dir", ":%02x", dir); > > print_0xhex(PRINT_ANY, "hwid", ":%02x", hwid) > > close_json_object(); > > close_json_array(PRINT_ANY, " "); > Hi Stephen, > > 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) > > > > > Also, you seem to not hear the request to not use opaque hex values > > in the iproute2 interface. The version, index, etc should be distinct > > parameter values not a hex string. > The opts STRING, especially these like "XX:YY:ZZ" are represented > as hex string on both adding and dumping. It is to keep consistent with > geneve_opts in m_tunnel_key and f_flower, see There are several different requests. 1. The format of the output must match the input. 2. Printing values in hex would be nice if they are bit fields 3. If non json uses hex, then json should use hex json is type less so { "ver":2 } and { "ver":"0x2" } are the same
On Thu, 23 Apr 2020 08:23:51 -0700 Stephen Hemminger wrote: > 3. If non json uses hex, then json should use hex > json is type less so { "ver":2 } and { "ver":"0x2" } are the same I may be missing something or misunderstanding you, but in my humble experience that's emphatically not true: $ echo '{ "a" : 2 }' | python -c 'import sys, json; print(json.load(sys.stdin)["a"] + 1)' 3 $ echo '{ "a" : "2" }' | python -c 'import sys, json; print(json.load(sys.stdin)["a"] + 1)' Traceback (most recent call last): File "<string>", line 1, in <module> TypeError: can only concatenate str (not "int") to str
On 4/19/20 4:28 PM, David Ahern wrote: > On 4/19/20 2:39 AM, Xin Long wrote: >> 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. > > I am fine with this set as is; Stephen had some concerns. > Please re-send the patches.
On Mon, Apr 27, 2020 at 2:29 AM David Ahern <dsahern@gmail.com> wrote: > > On 4/19/20 4:28 PM, David Ahern wrote: > > On 4/19/20 2:39 AM, Xin Long wrote: > >> 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. > > > > I am fine with this set as is; Stephen had some concerns. > > > > Please re-send the patches. OK, will post v4. thanks.
On 4/23/20 12:03 PM, Jakub Kicinski wrote: > On Thu, 23 Apr 2020 08:23:51 -0700 Stephen Hemminger wrote: >> 3. If non json uses hex, then json should use hex >> json is type less so { "ver":2 } and { "ver":"0x2" } are the same > > I may be missing something or misunderstanding you, but in my humble > experience that's emphatically not true: > > $ echo '{ "a" : 2 }' | python -c 'import sys, json; print(json.load(sys.stdin)["a"] + 1)' > 3 > $ echo '{ "a" : "2" }' | python -c 'import sys, json; print(json.load(sys.stdin)["a"] + 1)' > Traceback (most recent call last): > File "<string>", line 1, in <module> > TypeError: can only concatenate str (not "int") to str > I don't know which site is the definitive source for json, but several do state json has several types - strings, number, true / false / null, object, array.
On Mon, 27 Apr 2020 06:38:03 -0600 David Ahern <dsahern@gmail.com> wrote: > On 4/23/20 12:03 PM, Jakub Kicinski wrote: > > On Thu, 23 Apr 2020 08:23:51 -0700 Stephen Hemminger wrote: > >> 3. If non json uses hex, then json should use hex > >> json is type less so { "ver":2 } and { "ver":"0x2" } are the same > > > > I may be missing something or misunderstanding you, but in my humble > > experience that's emphatically not true: > > > > $ echo '{ "a" : 2 }' | python -c 'import sys, json; print(json.load(sys.stdin)["a"] + 1)' > > 3 > > $ echo '{ "a" : "2" }' | python -c 'import sys, json; print(json.load(sys.stdin)["a"] + 1)' > > Traceback (most recent call last): > > File "<string>", line 1, in <module> > > TypeError: can only concatenate str (not "int") to str > > > > I don't know which site is the definitive source for json, but several > do state json has several types - strings, number, true / false / null, > object, array. Probably I got confused because Python tries to be helpful... JSON is ossified/standardized http://www.ecma-international.org/publications/files/ECMA-ST/ECMA-404.pdf Best answer on the web was https://stackoverflow.com/questions/52671719/can-hex-format-be-used-with-json-files-if-so-how "JSON does not support hexadecimal numbers but they are supported in JSON5. json5.org"
On Thu, Apr 23, 2020 at 11:24 PM Stephen Hemminger <stephen@networkplumber.org> wrote: > > On Sat, 15 Feb 2020 01:40:27 +0800 > Xin Long <lucien.xin@gmail.com> wrote: > > > On Sat, Feb 15, 2020 at 12:13 AM Stephen Hemminger > > <stephen@networkplumber.org> wrote: > > > > > > On Fri, 14 Feb 2020 18:30:47 +0800 > > > Xin Long <lucien.xin@gmail.com> wrote: > > > > > > > + > > > > + open_json_array(PRINT_JSON, name); > > > > + open_json_object(NULL); > > > > + print_uint(PRINT_JSON, "ver", NULL, ver); > > > > + print_uint(PRINT_JSON, "index", NULL, idx); > > > > + print_uint(PRINT_JSON, "dir", NULL, dir); > > > > + print_uint(PRINT_JSON, "hwid", NULL, hwid); > > > > + close_json_object(); > > > > + close_json_array(PRINT_JSON, name); > > > > + > > > > + print_nl(); > > > > + print_string(PRINT_FP, name, "\t%s ", name); > > > > + sprintf(strbuf, "%02x:%08x:%02x:%02x", ver, idx, dir, hwid); > > > > + print_string(PRINT_FP, NULL, "%s ", strbuf); > > > > +} > > > > > > Instead of having two sets of prints, is it possible to do this > > > print_nl(); > > > print_string(PRINT_FP, NULL, "\t", NULL); > > > > > > open_json_array(PRINT_ANY, name); > > > open_json_object(NULL); > > > print_0xhex(PRINT_ANY, "ver", " %02x", ver); > > > print_0xhex(PRINT_ANY, "idx", ":%08x", idx); > > > print_0xhex(PRINT_ANY, "dir", ":%02x", dir); > > > print_0xhex(PRINT_ANY, "hwid", ":%02x", hwid) > > > close_json_object(); > > > close_json_array(PRINT_ANY, " "); > > Hi Stephen, > > > > 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) > > > > > > > > Also, you seem to not hear the request to not use opaque hex values > > > in the iproute2 interface. The version, index, etc should be distinct > > > parameter values not a hex string. > > The opts STRING, especially these like "XX:YY:ZZ" are represented > > as hex string on both adding and dumping. It is to keep consistent with > > geneve_opts in m_tunnel_key and f_flower, see > > There are several different requests. > > 1. The format of the output must match the input. > 2. Printing values in hex would be nice if they are bit fields > 3. If non json uses hex, then json should use hex > json is type less so { "ver":2 } and { "ver":"0x2" } are the same > V4 has been posted, with respect to these 3 rules. And all numbers are in uint type, no problems about { "ver":2 } and { "ver":"0x2" } things. Thanks.
diff --git a/ip/iproute_lwtunnel.c b/ip/iproute_lwtunnel.c index 7691002..f64ccb1 100644 --- a/ip/iproute_lwtunnel.c +++ b/ip/iproute_lwtunnel.c @@ -364,6 +364,43 @@ static void lwtunnel_print_vxlan_opts(struct rtattr *attr) print_uint(PRINT_FP, NULL, "0x%x ", gbp); } +static void lwtunnel_print_erspan_opts(struct rtattr *attr) +{ + struct rtattr *tb[LWTUNNEL_IP_OPT_ERSPAN_MAX + 1]; + struct rtattr *i = RTA_DATA(attr); + int rem = RTA_PAYLOAD(attr); + char *name = "erspan_opts"; + char strbuf[rem * 2 + 1]; + __u8 ver, hwid, dir; + __u32 idx; + + parse_rtattr(tb, LWTUNNEL_IP_OPT_ERSPAN_MAX, i, RTA_PAYLOAD(attr)); + ver = rta_getattr_u8(tb[LWTUNNEL_IP_OPT_ERSPAN_VER]); + if (ver == 1) { + idx = rta_getattr_be32(tb[LWTUNNEL_IP_OPT_ERSPAN_INDEX]); + dir = 0; + hwid = 0; + } else { + idx = 0; + dir = rta_getattr_u8(tb[LWTUNNEL_IP_OPT_ERSPAN_DIR]); + hwid = rta_getattr_u8(tb[LWTUNNEL_IP_OPT_ERSPAN_HWID]); + } + + open_json_array(PRINT_JSON, name); + open_json_object(NULL); + print_uint(PRINT_JSON, "ver", NULL, ver); + print_uint(PRINT_JSON, "index", NULL, idx); + print_uint(PRINT_JSON, "dir", NULL, dir); + print_uint(PRINT_JSON, "hwid", NULL, hwid); + close_json_object(); + close_json_array(PRINT_JSON, name); + + print_nl(); + print_string(PRINT_FP, name, "\t%s ", name); + sprintf(strbuf, "%02x:%08x:%02x:%02x", ver, idx, dir, hwid); + print_string(PRINT_FP, NULL, "%s ", strbuf); +} + static void lwtunnel_print_opts(struct rtattr *attr) { struct rtattr *tb_opt[LWTUNNEL_IP_OPTS_MAX + 1]; @@ -373,6 +410,8 @@ static void lwtunnel_print_opts(struct rtattr *attr) lwtunnel_print_geneve_opts(tb_opt[LWTUNNEL_IP_OPTS_GENEVE]); else if (tb_opt[LWTUNNEL_IP_OPTS_VXLAN]) lwtunnel_print_vxlan_opts(tb_opt[LWTUNNEL_IP_OPTS_VXLAN]); + else if (tb_opt[LWTUNNEL_IP_OPTS_ERSPAN]) + lwtunnel_print_erspan_opts(tb_opt[LWTUNNEL_IP_OPTS_ERSPAN]); } static void print_encap_ip(FILE *fp, struct rtattr *encap) @@ -987,6 +1026,82 @@ static int lwtunnel_parse_vxlan_opts(char *str, size_t len, struct rtattr *rta) return 0; } +static int lwtunnel_parse_erspan_opts(char *str, size_t len, struct rtattr *rta) +{ + struct rtattr *nest; + char *token; + int i, err; + + nest = rta_nest(rta, len, LWTUNNEL_IP_OPTS_ERSPAN | NLA_F_NESTED); + i = 1; + token = strsep(&str, ":"); + while (token) { + switch (i) { + case LWTUNNEL_IP_OPT_ERSPAN_VER: + { + __u8 opt_type; + + if (!strlen(token)) + break; + err = get_u8(&opt_type, token, 16); + if (err) + return err; + + rta_addattr8(rta, len, i, opt_type); + break; + } + case LWTUNNEL_IP_OPT_ERSPAN_INDEX: + { + __be32 opt_class; + + if (!strlen(token)) + break; + err = get_be32(&opt_class, token, 16); + if (err) + return err; + + rta_addattr32(rta, len, i, opt_class); + break; + } + case LWTUNNEL_IP_OPT_ERSPAN_DIR: + { + __u8 opt_type; + + if (!strlen(token)) + break; + err = get_u8(&opt_type, token, 16); + if (err) + return err; + + rta_addattr8(rta, len, i, opt_type); + break; + } + case LWTUNNEL_IP_OPT_ERSPAN_HWID: + { + __u8 opt_type; + + if (!strlen(token)) + break; + err = get_u8(&opt_type, token, 16); + if (err) + return err; + + rta_addattr8(rta, len, i, opt_type); + break; + } + default: + fprintf(stderr, "Unknown \"geneve_opts\" type\n"); + return -1; + } + + token = strsep(&str, ":"); + i++; + } + + rta_nest_end(rta, nest); + return 0; +} + static int parse_encap_ip(struct rtattr *rta, size_t len, int *argcp, char ***argvp) { @@ -1073,6 +1188,21 @@ static int parse_encap_ip(struct rtattr *rta, size_t len, invarg("\"vxlan_opts\" value is invalid\n", *argv); rta_nest_end(rta, nest); + } else if (strcmp(*argv, "erspan_opts") == 0) { + struct rtattr *nest; + + if (opts_ok++) + duparg2("opts", *argv); + + NEXT_ARG(); + + nest = rta_nest(rta, len, + LWTUNNEL_IP_OPTS | NLA_F_NESTED); + ret = lwtunnel_parse_erspan_opts(*argv, len, rta); + if (ret) + invarg("\"erspan_opts\" value is invalid\n", + *argv); + rta_nest_end(rta, nest); } else if (strcmp(*argv, "key") == 0) { if (key_ok++) duparg2("key", *argv); @@ -1272,6 +1402,21 @@ static int parse_encap_ip6(struct rtattr *rta, size_t len, invarg("\"vxlan_opts\" value is invalid\n", *argv); rta_nest_end(rta, nest); + } else if (strcmp(*argv, "erspan_opts") == 0) { + struct rtattr *nest; + + if (opts_ok++) + duparg2("opts", *argv); + + NEXT_ARG(); + + nest = rta_nest(rta, len, + LWTUNNEL_IP_OPTS | NLA_F_NESTED); + ret = lwtunnel_parse_erspan_opts(*argv, len, rta); + if (ret) + invarg("\"erspan_opts\" value is invalid\n", + *argv); + rta_nest_end(rta, nest); } else if (strcmp(*argv, "key") == 0) { if (key_ok++) duparg2("key", *argv);
This patch is to add LWTUNNEL_IP_OPTS_ERSPAN's parse and print to implement erspan options support in iproute_lwtunnel. Option is expressed as version:index:dir:hwid, dir and hwid will be parsed when version is 2, while index will be parsed when version is 1. erspan doesn't support multiple options. With this patch, users can add and dump erspan options like: # ip netns add a # ip netns add b # ip -n a link add eth0 type veth peer name eth0 netns b # ip -n a link set eth0 up # ip -n b link set eth0 up # ip -n a addr add 10.1.0.1/24 dev eth0 # ip -n b addr add 10.1.0.2/24 dev eth0 # ip -n b link add erspan1 type erspan key 1 seq erspan 123 \ local 10.1.0.2 remote 10.1.0.1 # ip -n b addr add 1.1.1.1/24 dev erspan1 # ip -n b link set erspan1 up # ip -n b route add 2.1.1.0/24 dev erspan1 # ip -n a link add erspan1 type erspan key 1 seq local 10.1.0.1 external # ip -n a addr add 2.1.1.1/24 dev erspan1 # ip -n a link set erspan1 up # ip -n a route add 1.1.1.0/24 encap ip id 1 \ erspan_opts 2:123:1:2 dst 10.1.0.2 dev erspan1 # ip -n a route show # ip netns exec a ping 1.1.1.1 -c 1 1.1.1.0/24 encap ip id 1 src 0.0.0.0 dst 10.1.0.2 ttl 0 tos 0 erspan_opts 02:00000000:01:02 dev erspan1 scope link PING 1.1.1.1 (1.1.1.1) 56(84) bytes of data. 64 bytes from 1.1.1.1: icmp_seq=1 ttl=64 time=0.124 ms v1->v2: - improve the changelog. - use PRINT_ANY to support dumping with json format. v2->v3: - implement proper JSON object for opts instead of just bunch of strings. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- ip/iproute_lwtunnel.c | 145 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 145 insertions(+)