Re: [PATCH net-next] tools: ynl-gen: fix parse multi-attr enum attribute

[Jakub Kicinski]

On Tue, 11 Jul 2023 11:53:23 +0200 Arkadiusz Kubalewski wrote:
> diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
> index 3b343d6cbbc0..553d82dd6382 100644
> --- a/tools/net/ynl/lib/ynl.py
> +++ b/tools/net/ynl/lib/ynl.py
> @@ -407,7 +407,14 @@ class YnlFamily(SpecFamily):
>                  raw >>= 1
>                  i += 1
>          else:
> -            value = enum.entries_by_val[raw - i].name
> +            if attr_spec.is_multi:
> +                for index in range(len(raw)):
> +                    if (type(raw[index]) == int):
> +                        enum_name = enum.entries_by_val[raw[index] - i].name
> +                        rsp[attr_spec['name']][index] = enum_name
> +                return
> +            else:
> +                value = enum.entries_by_val[raw - i].name
>          rsp[attr_spec['name']] = value

Two asks:

First this function stupidly looks at value-start. Best I can tell this
is a leftover from when enum set was an array, but potentially "indexed
with an offset" (ie. if value start = 10, first elem would have value
11, second 12 etc.). When we added support for sparse enums this was
carried forward, but it's actually incorrect. entries_by_val is indexed
with the real value, we should not subtract the start-value. So please
send a patch to set i to 0 at the start and ignore start-value here
(or LMK if I should send one).

Second, instead of fixing the value up here, after already putting it
in the rsp - can we call this function to decode the enum before?
A bit hard to explain, let me show you the diff of what I have in mind
for the call site:

diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index 1b3a36fbb1c3..e2e8a8c5fb6b 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -466,15 +466,15 @@ genl_family_name_to_id = None
             else:
                 raise Exception(f'Unknown {attr_spec["type"]} with name {attr_spec["name"]}')
 
+            if 'enum' in attr_spec:
+                decoded = self._decode_enum(rsp, attr_spec)
+
             if not attr_spec.is_multi:
                 rsp[attr_spec['name']] = decoded
             elif attr_spec.name in rsp:
                 rsp[attr_spec.name].append(decoded)
             else:
                 rsp[attr_spec.name] = [decoded]
-
-            if 'enum' in attr_spec:
-                self._decode_enum(rsp, attr_spec)
         return rsp
 
     def _decode_extack_path(self, attrs, attr_set, offset, target):

Then _decode_enum() only has to ever deal with single values,
and the caller will take care of mutli_attr like it would for any other
type?
-- 
pw-bot: cr