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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d0ec991a-77fc-84dd-b4cc-9ae649f7a0ac@gmail.com>
Date:   Mon, 17 Feb 2020 12:53:14 -0700
From:   David Ahern <dsahern@...il.com>
To:     Xin Long <lucien.xin@...il.com>,
        Stephen Hemminger <stephen@...workplumber.org>
Cc:     network dev <netdev@...r.kernel.org>,
        Simon Horman <simon.horman@...ronome.com>
Subject: Re: [PATCHv3 iproute2-next 3/7] iproute_lwtunnel: add options support
 for erspan metadata

On 2/15/20 11:38 PM, Xin Long wrote:
> On Sun, Feb 16, 2020 at 12:51 AM David Ahern <dsahern@...il.com> wrote:
>>
>> On 2/14/20 9:18 PM, Xin Long wrote:
>>> On Sat, Feb 15, 2020 at 8:21 AM Stephen Hemminger
>>> <stephen@...workplumber.org> wrote:
>>>>
>>>> On Sat, 15 Feb 2020 01:40:27 +0800
>>>> Xin Long <lucien.xin@...il.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@...ronome.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?