[PATCHv3,iproute2-next,3/7] iproute_lwtunnel: add options support for erspan metadata
diff mbox series

Message ID 290ab5d2dc06b183159d293ab216962a3cc0df6d.1581676056.git.lucien.xin@gmail.com
State New
Delegated to: David Ahern
Headers show
Series
  • iproute2: fully support for geneve/vxlan/erspan options
Related show

Commit Message

Xin Long Feb. 14, 2020, 10:30 a.m. UTC
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(+)

Comments

Stephen Hemminger Feb. 14, 2020, 4:13 p.m. UTC | #1
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.
Xin Long Feb. 14, 2020, 5:40 p.m. UTC | #2
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.
Stephen Hemminger Feb. 15, 2020, 12:21 a.m. UTC | #3
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
Xin Long Feb. 15, 2020, 4:18 a.m. UTC | #4
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.
David Ahern Feb. 15, 2020, 4:51 p.m. UTC | #5
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?
Xin Long Feb. 16, 2020, 6:38 a.m. UTC | #6
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.
David Ahern Feb. 17, 2020, 7:53 p.m. UTC | #7
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?
Stephen Hemminger Feb. 17, 2020, 9:02 p.m. UTC | #8
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.
Xin Long Feb. 18, 2020, 4:29 a.m. UTC | #9
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.

Patch
diff mbox series

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);