diff mbox series

[net-next,03/13] tools: ynl: allow user to pass enum string instead of scalar value

Message ID 20240219172525.71406-4-jiri@resnulli.us (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series netlink: specs: devlink: add the rest of missing attribute definitions | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl fail Generated files up to date; build failed; build has 3 warnings/errors; GEN HAS DIFF 2 files changed, 12648 deletions(-);
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 39 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-02-19--21-00 (tests: 1357)

Commit Message

Jiri Pirko Feb. 19, 2024, 5:25 p.m. UTC
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>
---
 tools/net/ynl/lib/ynl.py | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

Comments

Jakub Kicinski Feb. 19, 2024, 8:49 p.m. UTC | #1
On Mon, 19 Feb 2024 18:25:19 +0100 Jiri Pirko wrote:
> +    def _get_scalar(self, attr_spec, value):

It'd be cleaner to make it more symmetric with _decode_enum(), and call
it _encode_enum().
Jakub Kicinski Feb. 19, 2024, 8:51 p.m. UTC | #2
On Mon, 19 Feb 2024 18:25:19 +0100 Jiri Pirko wrote:
> +        if enum.type == 'flags' or attr_spec.get('enum-as-flags', False):
> +            scalar = 0
> +            for single_value in value:
> +                scalar += enum.entries[single_value].user_value(as_flags = True)

If the user mistakenly passes a single value for a flag, rather than 
a set, this is going to generate a hard to understand error.
How about we check isinstance(, str) and handle that directly,
whether a flag or not.
Jiri Pirko Feb. 20, 2024, 7:25 a.m. UTC | #3
Mon, Feb 19, 2024 at 09:49:14PM CET, kuba@kernel.org wrote:
>On Mon, 19 Feb 2024 18:25:19 +0100 Jiri Pirko wrote:
>> +    def _get_scalar(self, attr_spec, value):
>
>It'd be cleaner to make it more symmetric with _decode_enum(), and call
>it _encode_enum().

That is misleading name, as it does not have to be enum.
Jiri Pirko Feb. 20, 2024, 7:27 a.m. UTC | #4
Mon, Feb 19, 2024 at 09:51:00PM CET, kuba@kernel.org wrote:
>On Mon, 19 Feb 2024 18:25:19 +0100 Jiri Pirko wrote:
>> +        if enum.type == 'flags' or attr_spec.get('enum-as-flags', False):
>> +            scalar = 0
>> +            for single_value in value:
>> +                scalar += enum.entries[single_value].user_value(as_flags = True)
>
>If the user mistakenly passes a single value for a flag, rather than 
>a set, this is going to generate a hard to understand error.
>How about we check isinstance(, str) and handle that directly,
>whether a flag or not.

Yeah, I was thinking about that as well. But as the flag output is
always list, here we expect also always list. I can either do what you
suggest of Errout with some sane message in case of the variable is not
a list. I didn't find ynl to be particularly forgiving in case of input
and error messages, that is why I didn't bother here.
Jakub Kicinski Feb. 21, 2024, 1:55 a.m. UTC | #5
On Tue, 20 Feb 2024 08:25:14 +0100 Jiri Pirko wrote:
> >It'd be cleaner to make it more symmetric with _decode_enum(), and call
> >it _encode_enum().  
> 
> That is misleading name, as it does not have to be enum.

It's a variant of a encode function using enum metadata.
Jakub Kicinski Feb. 21, 2024, 1:59 a.m. UTC | #6
On Tue, 20 Feb 2024 08:27:57 +0100 Jiri Pirko wrote:
> >If the user mistakenly passes a single value for a flag, rather than 
> >a set, this is going to generate a hard to understand error.
> >How about we check isinstance(, str) and handle that directly,
> >whether a flag or not.  
> 
> Yeah, I was thinking about that as well. But as the flag output is
> always list, here we expect also always list. I can either do what you
> suggest of Errout with some sane message in case of the variable is not
> a list. I didn't find ynl to be particularly forgiving in case of input
> and error messages, that is why I didn't bother here.

It's not the same thing, but (without looking at the code IIRC)
for multi-attr we do accept both a list and direct value.
Here we don't have to be lenient in what we accept.
Clear error message would be good enough.

Some of the sharp edges in Python YNL are because I very much
anticipated the pyroute2 maintainer to do a proper implementation, 
and this tool was just a very crude PoC :D
Donald Hunter Feb. 21, 2024, 11:40 a.m. UTC | #7
On Wed, 21 Feb 2024 at 01:59, Jakub Kicinski <kuba@kernel.org> wrote:
>
> Some of the sharp edges in Python YNL are because I very much
> anticipated the pyroute2 maintainer to do a proper implementation,
> and this tool was just a very crude PoC :D

Hah yeah, I looked at pyroute2 a while back and thought there was a
bit of an impedance mismatch between the dynamic schema driven
approach of ynl and the declarative / procedural code in iproute2. I
think a code generator would be the way to target iproute2.

https://github.com/svinota/pyroute2/blob/34d0768f89fd232126c49e2f7c94e6da6582795b/pyroute2/netlink/rtnl/rtmsg.py#L102-L139

I find ynl to be a very useful tool when writing and testing spec
files and have been happy to contribute to it for that purpose. I
think we have started to remove some of the sharp edges, but there is
more to do :-)
Jiri Pirko Feb. 21, 2024, 2:31 p.m. UTC | #8
Wed, Feb 21, 2024 at 02:55:56AM CET, kuba@kernel.org wrote:
>On Tue, 20 Feb 2024 08:25:14 +0100 Jiri Pirko wrote:
>> >It'd be cleaner to make it more symmetric with _decode_enum(), and call
>> >it _encode_enum().  
>> 
>> That is misleading name, as it does not have to be enum.
>
>It's a variant of a encode function using enum metadata.

Not really. "decode_enum()" is exactly that, it only works with enum
case. Here, the enum is optional. If user passes scalar directly or if
the attribute does not have "enum" defined, this has nothing to do with
enum. But, as you wish.
diff mbox series

Patch

diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index ccdb2f1e7379..d2ea1571d00c 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -438,6 +438,21 @@  class YnlFamily(SpecFamily):
         self.sock.setsockopt(Netlink.SOL_NETLINK, Netlink.NETLINK_ADD_MEMBERSHIP,
                              mcast_id)
 
+    def _get_scalar(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
+            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 +489,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._get_scalar(attr, value)
             if attr.is_auto_scalar:
                 attr_type = attr["type"][0] + ('32' if scalar.bit_length() <= 32 else '64')
             else:
@@ -482,7 +497,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._get_scalar(attr, value["value"])
+            scalar_selector = self._get_scalar(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''