[patch net-next v2 0/3] tools: ynl: couple of cmdline enhancements

[patch net-next v2 0/3] tools: ynl: couple of cmdline enhancements

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

This is part of the original "netlink: specs: devlink: add the rest of
missing attribute definitions" set which was rejected [1]. These three
patches enhances the cmdline user comfort, allowing to pass flag
attribute with bool values and enum names instead of scalars.

[1] https://lore.kernel.org/all/20240220181004.639af931@kernel.org/

---
v1->v2:
- only first 3 patches left, the rest it cut out
- see changelog of individual patches

Jiri Pirko (3):
  tools: ynl: allow user to specify flag attr with bool values
  tools: ynl: process all scalar types encoding in single elif statement
  tools: ynl: allow user to pass enum string instead of scalar value

 tools/net/ynl/lib/ynl.py | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

-- 
2.43.2

[patch net-next v2 1/3] tools: ynl: allow user to specify flag attr with bool values

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

The flag attr presence in Netlink message indicates value "true",
if it is missing in the message it means "false".

Allow user to specify attrname with value "true"/"false"
in json for flag attrs, treat "false" value properly.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v1->v2:
- accept other values than "False"
---
 tools/net/ynl/lib/ynl.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index f45ee5f29bed..4a44840bab68 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -459,6 +459,8 @@ class YnlFamily(SpecFamily):
                 attr_payload += self._add_attr(attr['nested-attributes'],
                                                subname, subvalue, sub_attrs)
         elif attr["type"] == 'flag':
+            if not value:
+                return b''
             attr_payload = b''
         elif attr["type"] == 'string':
             attr_payload = str(value).encode('ascii') + b'\x00'
-- 
2.43.2

Re: [patch net-next v2 1/3] tools: ynl: allow user to specify flag attr with bool values

[Donald Hunter]

On Wed, 21 Feb 2024 at 15:54, Jiri Pirko <jiri@resnulli.us> wrote:
>
> From: Jiri Pirko <jiri@nvidia.com>
>
> The flag attr presence in Netlink message indicates value "true",
> if it is missing in the message it means "false".
>
> Allow user to specify attrname with value "true"/"false"
> in json for flag attrs, treat "false" value properly.
>
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> ---
> v1->v2:
> - accept other values than "False"
> ---
>  tools/net/ynl/lib/ynl.py | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
> index f45ee5f29bed..4a44840bab68 100644
> --- a/tools/net/ynl/lib/ynl.py
> +++ b/tools/net/ynl/lib/ynl.py
> @@ -459,6 +459,8 @@ class YnlFamily(SpecFamily):
>                  attr_payload += self._add_attr(attr['nested-attributes'],
>                                                 subname, subvalue, sub_attrs)
>          elif attr["type"] == 'flag':
> +            if not value:
> +                return b''

Minor nit: It took me a moment to realise that by returning here, this
skips attribute creation. A comment to this effect would be helpful:

# If value is absent or false then skip attribute creation.

>              attr_payload = b''
>          elif attr["type"] == 'string':
>              attr_payload = str(value).encode('ascii') + b'\x00'
> --
> 2.43.2
>

Re: [patch net-next v2 1/3] tools: ynl: allow user to specify flag attr with bool values

[Jiri Pirko]

Wed, Feb 21, 2024 at 07:07:40PM CET, donald.hunter@gmail.com wrote:
>On Wed, 21 Feb 2024 at 15:54, Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> From: Jiri Pirko <jiri@nvidia.com>
>>
>> The flag attr presence in Netlink message indicates value "true",
>> if it is missing in the message it means "false".
>>
>> Allow user to specify attrname with value "true"/"false"
>> in json for flag attrs, treat "false" value properly.
>>
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>> ---
>> v1->v2:
>> - accept other values than "False"
>> ---
>>  tools/net/ynl/lib/ynl.py | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
>> index f45ee5f29bed..4a44840bab68 100644
>> --- a/tools/net/ynl/lib/ynl.py
>> +++ b/tools/net/ynl/lib/ynl.py
>> @@ -459,6 +459,8 @@ class YnlFamily(SpecFamily):
>>                  attr_payload += self._add_attr(attr['nested-attributes'],
>>                                                 subname, subvalue, sub_attrs)
>>          elif attr["type"] == 'flag':
>> +            if not value:
>> +                return b''
>
>Minor nit: It took me a moment to realise that by returning here, this
>skips attribute creation. A comment to this effect would be helpful:
>
># If value is absent or false then skip attribute creation.

Sure, will add.

>
>>              attr_payload = b''
>>          elif attr["type"] == 'string':
>>              attr_payload = str(value).encode('ascii') + b'\x00'
>> --
>> 2.43.2
>>

[patch net-next v2 2/3] tools: ynl: process all scalar types encoding in single elif statement

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

As a preparation to handle enums for scalar values, unify the processing
of all scalar types in a single elif statement.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 tools/net/ynl/lib/ynl.py | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index 4a44840bab68..38244aff1ec7 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -473,14 +473,14 @@ class YnlFamily(SpecFamily):
                 attr_payload = self._encode_struct(attr.struct_name, value)
             else:
                 raise Exception(f'Unknown type for binary attribute, value: {value}')
-        elif attr.is_auto_scalar:
+        elif attr['type'] in NlAttr.type_formats or attr.is_auto_scalar:
             scalar = int(value)
-            real_type = attr["type"][0] + ('32' if scalar.bit_length() <= 32 else '64')
-            format = NlAttr.get_format(real_type, attr.byte_order)
-            attr_payload = format.pack(int(value))
-        elif attr['type'] in NlAttr.type_formats:
-            format = NlAttr.get_format(attr['type'], attr.byte_order)
-            attr_payload = format.pack(int(value))
+            if attr.is_auto_scalar:
+                attr_type = attr["type"][0] + ('32' if scalar.bit_length() <= 32 else '64')
+            else:
+                attr_type = attr["type"]
+            format = NlAttr.get_format(attr_type, attr.byte_order)
+            attr_payload = format.pack(scalar)
         elif attr['type'] in "bitfield32":
             attr_payload = struct.pack("II", int(value["value"]), int(value["selector"]))
         elif attr['type'] == 'sub-message':
-- 
2.43.2

Re: [patch net-next v2 2/3] tools: ynl: process all scalar types encoding in single elif statement

[Donald Hunter]

On Wed, 21 Feb 2024 at 15:54, Jiri Pirko <jiri@resnulli.us> wrote:
>
> From: Jiri Pirko <jiri@nvidia.com>
>
> As a preparation to handle enums for scalar values, unify the processing
> of all scalar types in a single elif statement.

LGTM.

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>

> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> ---
>  tools/net/ynl/lib/ynl.py | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
> index 4a44840bab68..38244aff1ec7 100644
> --- a/tools/net/ynl/lib/ynl.py
> +++ b/tools/net/ynl/lib/ynl.py
> @@ -473,14 +473,14 @@ class YnlFamily(SpecFamily):
>                  attr_payload = self._encode_struct(attr.struct_name, value)
>              else:
>                  raise Exception(f'Unknown type for binary attribute, value: {value}')
> -        elif attr.is_auto_scalar:
> +        elif attr['type'] in NlAttr.type_formats or attr.is_auto_scalar:
>              scalar = int(value)
> -            real_type = attr["type"][0] + ('32' if scalar.bit_length() <= 32 else '64')
> -            format = NlAttr.get_format(real_type, attr.byte_order)
> -            attr_payload = format.pack(int(value))
> -        elif attr['type'] in NlAttr.type_formats:
> -            format = NlAttr.get_format(attr['type'], attr.byte_order)
> -            attr_payload = format.pack(int(value))
> +            if attr.is_auto_scalar:
> +                attr_type = attr["type"][0] + ('32' if scalar.bit_length() <= 32 else '64')
> +            else:
> +                attr_type = attr["type"]
> +            format = NlAttr.get_format(attr_type, attr.byte_order)
> +            attr_payload = format.pack(scalar)
>          elif attr['type'] in "bitfield32":
>              attr_payload = struct.pack("II", int(value["value"]), int(value["selector"]))
>          elif attr['type'] == 'sub-message':
> --
> 2.43.2
>

[patch net-next v2 3/3] tools: ynl: allow user to pass enum string instead of scalar value

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

During decoding of messages coming from kernel, attribute values are
converted to enum names in case the attribute type is enum of bitfield32.

However, when user constructs json message, he has to pass plain scalar
values. See "state" "selector" and "value" attributes in following
examples:

$ sudo ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml --do pin-set --json '{"id": 0, "parent-device": {"parent-id": 0, "state": 1}}'
$ sudo ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/devlink.yaml --do port-set --json '{"bus-name": "pci", "dev-name": "0000:08:00.1", "port-index": 98304, "port-function": {"caps": {"selector": 1, "value": 1 }}}'

Allow user to pass strings containing enum names, convert them to scalar
values to be encoded into Netlink message:

$ sudo ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml --do pin-set --json '{"id": 0, "parent-device": {"parent-id": 0, "state": "connected"}}'
$ sudo ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/devlink.yaml --do port-set --json '{"bus-name": "pci", "dev-name": "0000:08:00.1", "port-index": 98304, "port-function": {"caps": {"selector": ["roce-bit"], "value": ["roce-bit"] }}}'

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v1->v2:
- s/_get_scalar/_encode_enum/
- accept flat name not in a list
---
 tools/net/ynl/lib/ynl.py | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index 38244aff1ec7..14ae30db984a 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -438,6 +438,23 @@ class YnlFamily(SpecFamily):
         self.sock.setsockopt(Netlink.SOL_NETLINK, Netlink.NETLINK_ADD_MEMBERSHIP,
                              mcast_id)
 
+    def _encode_enum(self, attr_spec, value):
+        try:
+            return int(value)
+        except (ValueError, TypeError) as e:
+            if 'enum' not in attr_spec:
+                raise e
+        enum = self.consts[attr_spec['enum']]
+        if enum.type == 'flags' or attr_spec.get('enum-as-flags', False):
+            scalar = 0
+            if isinstance(value, str):
+                value = [value]
+            for single_value in value:
+                scalar += enum.entries[single_value].user_value(as_flags = True)
+            return scalar
+        else:
+            return enum.entries[value].user_value()
+
     def _add_attr(self, space, name, value, search_attrs):
         try:
             attr = self.attr_sets[space][name]
@@ -474,7 +491,7 @@ class YnlFamily(SpecFamily):
             else:
                 raise Exception(f'Unknown type for binary attribute, value: {value}')
         elif attr['type'] in NlAttr.type_formats or attr.is_auto_scalar:
-            scalar = int(value)
+            scalar = self._encode_enum(attr, value)
             if attr.is_auto_scalar:
                 attr_type = attr["type"][0] + ('32' if scalar.bit_length() <= 32 else '64')
             else:
@@ -482,7 +499,9 @@ class YnlFamily(SpecFamily):
             format = NlAttr.get_format(attr_type, attr.byte_order)
             attr_payload = format.pack(scalar)
         elif attr['type'] in "bitfield32":
-            attr_payload = struct.pack("II", int(value["value"]), int(value["selector"]))
+            scalar_value = self._encode_enum(attr, value["value"])
+            scalar_selector = self._encode_enum(attr, value["selector"])
+            attr_payload = struct.pack("II", scalar_value, scalar_selector)
         elif attr['type'] == 'sub-message':
             msg_format = self._resolve_selector(attr, search_attrs)
             attr_payload = b''
-- 
2.43.2

Re: [patch net-next v2 3/3] tools: ynl: allow user to pass enum string instead of scalar value

[Donald Hunter]

On Wed, 21 Feb 2024 at 15:54, Jiri Pirko <jiri@resnulli.us> wrote:
>
> From: Jiri Pirko <jiri@nvidia.com>
>
> During decoding of messages coming from kernel, attribute values are
> converted to enum names in case the attribute type is enum of bitfield32.
>
> However, when user constructs json message, he has to pass plain scalar
> values. See "state" "selector" and "value" attributes in following
> examples:
>
> $ sudo ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml --do pin-set --json '{"id": 0, "parent-device": {"parent-id": 0, "state": 1}}'
> $ sudo ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/devlink.yaml --do port-set --json '{"bus-name": "pci", "dev-name": "0000:08:00.1", "port-index": 98304, "port-function": {"caps": {"selector": 1, "value": 1 }}}'
>
> Allow user to pass strings containing enum names, convert them to scalar
> values to be encoded into Netlink message:
>
> $ sudo ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml --do pin-set --json '{"id": 0, "parent-device": {"parent-id": 0, "state": "connected"}}'
> $ sudo ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/devlink.yaml --do port-set --json '{"bus-name": "pci", "dev-name": "0000:08:00.1", "port-index": 98304, "port-function": {"caps": {"selector": ["roce-bit"], "value": ["roce-bit"] }}}'
>
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>

Re: [patch net-next v2 3/3] tools: ynl: allow user to pass enum string instead of scalar value

[Jakub Kicinski]

On Wed, 21 Feb 2024 16:54:15 +0100 Jiri Pirko wrote:
> +    def _encode_enum(self, attr_spec, value):
> +        try:
> +            return int(value)
> +        except (ValueError, TypeError) as e:
> +            if 'enum' not in attr_spec:
> +                raise e
> +        enum = self.consts[attr_spec['enum']]
> +        if enum.type == 'flags' or attr_spec.get('enum-as-flags', False):
> +            scalar = 0
> +            if isinstance(value, str):
> +                value = [value]
> +            for single_value in value:
> +                scalar += enum.entries[single_value].user_value(as_flags = True)
> +            return scalar
> +        else:
> +            return enum.entries[value].user_value()

That's not symmetric with _decode_enum(), I said:

                     vvvvvvvvvvvvvvvvvvvvvv
  It'd be cleaner to make it more symmetric with _decode_enum(), and
  call it _encode_enum().

How about you go back to your _get_scalar name and only factor out the
parts which encode the enum to be a _encode_enum() ?

Re: [patch net-next v2 3/3] tools: ynl: allow user to pass enum string instead of scalar value

[Jiri Pirko]

Wed, Feb 21, 2024 at 07:49:36PM CET, kuba@kernel.org wrote:
>On Wed, 21 Feb 2024 16:54:15 +0100 Jiri Pirko wrote:
>> +    def _encode_enum(self, attr_spec, value):
>> +        try:
>> +            return int(value)
>> +        except (ValueError, TypeError) as e:
>> +            if 'enum' not in attr_spec:
>> +                raise e
>> +        enum = self.consts[attr_spec['enum']]
>> +        if enum.type == 'flags' or attr_spec.get('enum-as-flags', False):
>> +            scalar = 0
>> +            if isinstance(value, str):
>> +                value = [value]
>> +            for single_value in value:
>> +                scalar += enum.entries[single_value].user_value(as_flags = True)
>> +            return scalar
>> +        else:
>> +            return enum.entries[value].user_value()
>
>That's not symmetric with _decode_enum(), I said:
>
>                     vvvvvvvvvvvvvvvvvvvvvv
>  It'd be cleaner to make it more symmetric with _decode_enum(), and
>  call it _encode_enum().
>
>How about you go back to your _get_scalar name and only factor out the
>parts which encode the enum to be a _encode_enum() ?

Okay.

RE: [patch net-next v2 0/3] tools: ynl: couple of cmdline enhancements

[Keller, Jacob E]

> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Wednesday, February 21, 2024 7:54 AM
> To: netdev@vger.kernel.org
> Cc: kuba@kernel.org; pabeni@redhat.com; davem@davemloft.net;
> edumazet@google.com; Keller, Jacob E <jacob.e.keller@intel.com>;
> swarupkotikalapudi@gmail.com; donald.hunter@gmail.com; sdf@google.com;
> lorenzo@kernel.org; alessandromarcolini99@gmail.com
> Subject: [patch net-next v2 0/3] tools: ynl: couple of cmdline enhancements
> 
> From: Jiri Pirko <jiri@nvidia.com>
> 
> This is part of the original "netlink: specs: devlink: add the rest of
> missing attribute definitions" set which was rejected [1]. These three
> patches enhances the cmdline user comfort, allowing to pass flag
> attribute with bool values and enum names instead of scalars.
> 
> [1] https://lore.kernel.org/all/20240220181004.639af931@kernel.org/
> 

This series looks straight forward to me.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> ---
> v1->v2:
> - only first 3 patches left, the rest it cut out
> - see changelog of individual patches
> 
> Jiri Pirko (3):
>   tools: ynl: allow user to specify flag attr with bool values
>   tools: ynl: process all scalar types encoding in single elif statement
>   tools: ynl: allow user to pass enum string instead of scalar value
> 
>  tools/net/ynl/lib/ynl.py | 39 ++++++++++++++++++++++++++++++---------
>  1 file changed, 30 insertions(+), 9 deletions(-)
> 
> --
> 2.43.2