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

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

[Arkadiusz Kubalewski]

When attribute is enum type and marked as multi-attr, the netlink respond
is not parsed, fails with stack trace:
File "/root/arek/linux-dpll/tools/net/ynl/lib/ynl.py", line 600, in dump
  return self._op(method, vals, dump=True)
File "/root/arek/linux-dpll/tools/net/ynl/lib/ynl.py", line 586, in _op
  rsp_msg = self._decode(gm.raw_attrs, op.attr_set.name)
File "/root/arek/linux-dpll/tools/net/ynl/lib/ynl.py", line 453, in _decode
  self._decode_enum(rsp, attr_spec)
File "/root/arek/linux-dpll/tools/net/ynl/lib/ynl.py", line 410, in _decode_enum
  value = enum.entries_by_val[raw - i].name
TypeError: unsupported operand type(s) for -: 'list' and 'int'
error: 1

Allow succesfull parse of multi-attr enums by decoding and assigning their
names into response in the _decode_enum(..) function.

Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
 tools/net/ynl/lib/ynl.py | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

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
 
     def _decode_binary(self, attr, attr_spec):
-- 
2.37.3

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

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

[Kubalewski, Arkadiusz]

>From: Jakub Kicinski <kuba@kernel.org>
>Sent: Wednesday, July 12, 2023 6:00 AM
>
>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?

Sure, I will try to implement your proposal and send update here.

Thank you!
Arkadiusz

>--
>pw-bot: cr

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

[Jakub Kicinski]

On Wed, 12 Jul 2023 09:47:43 +0000 Kubalewski, Arkadiusz wrote:
> >+            if 'enum' in attr_spec:
> >+                decoded = self._decode_enum(rsp, attr_spec)

To be clear - this is just a quick mock up, you'll need to change 
the arguments here, obviously. Probably to decoded and attr_spec?

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

[Kubalewski, Arkadiusz]

>From: Jakub Kicinski <kuba@kernel.org>
>Sent: Wednesday, July 12, 2023 6:24 PM
>To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>
>Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>davem@davemloft.net; pabeni@redhat.com; edumazet@google.com;
>chuck.lever@oracle.com
>Subject: Re: [PATCH net-next] tools: ynl-gen: fix parse multi-attr enum
>attribute
>
>On Wed, 12 Jul 2023 09:47:43 +0000 Kubalewski, Arkadiusz wrote:
>> >+            if 'enum' in attr_spec:
>> >+                decoded = self._decode_enum(rsp, attr_spec)
>
>To be clear - this is just a quick mock up, you'll need to change
>the arguments here, obviously. Probably to decoded and attr_spec?

Well I did something that works for me:
("[PATCH net-next 0/2] tools: ynl-gen: fix parse multi-attr enum attribute")

But I am pretty sure it could break the other _decode_enum call
(from _decode_binary(..)), wasn't able to test it yet, as it seems to be used
only with ovs_flow.yaml spec (binary + struct type attr).

If you could take a look would be great.

Thank you!
Arkadiusz