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

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

[Arkadiusz Kubalewski]

Fix the issues with parsing enums in ynl.py script.

v2->v3:
- Restore i iterator helper for decoding 'enum-as-flag' attributes in
_decode_enum(..) function.
- Fix _decode_enum(..) usage in case of binary & struct type attributes
by moving it to as_struct() funtion while calling _decode_enum(..) for
individual struct members.

v1->v2:
Initially this was one patch, but review shown there is need to fix also
leftover issues with indexing in _decode_enum(..) function
("tools: ynl-gen: fix enum index in _decode_enum(..)").

Arkadiusz Kubalewski (2):
  tools: ynl-gen: fix enum index in _decode_enum(..)
  tools: ynl-gen: fix parse multi-attr enum attribute

 tools/net/ynl/lib/ynl.py | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

-- 
2.38.1

[PATCH net-next v3 1/2] tools: ynl-gen: fix enum index in _decode_enum(..)

[Arkadiusz Kubalewski]

Remove wrong index adjustement, which is leftover from adding
support for sparse enums.
enum.entries_by_val() function shall not subtract the start-value, as
it is indexed with real enum value.

Fixes: c311aaa74ca1 ("tools: ynl: fix enum-as-flags in the generic CLI")
Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
---
 tools/net/ynl/lib/ynl.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index 1b3a36fbb1c3..5db7d47067f9 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -420,8 +420,8 @@ class YnlFamily(SpecFamily):
     def _decode_enum(self, rsp, attr_spec):
         raw = rsp[attr_spec['name']]
         enum = self.consts[attr_spec['enum']]
-        i = attr_spec.get('value-start', 0)
         if 'enum-as-flags' in attr_spec and attr_spec['enum-as-flags']:
+            i = attr_spec.get('value-start', 0)
             value = set()
             while raw:
                 if raw & 1:
@@ -429,7 +429,7 @@ class YnlFamily(SpecFamily):
                 raw >>= 1
                 i += 1
         else:
-            value = enum.entries_by_val[raw - i].name
+            value = enum.entries_by_val[raw].name
         rsp[attr_spec['name']] = value
 
     def _decode_binary(self, attr, attr_spec):
-- 
2.38.1

Re: [PATCH net-next v3 1/2] tools: ynl-gen: fix enum index in _decode_enum(..)

[Jakub Kicinski]

On Tue, 18 Jul 2023 18:22:24 +0200 Arkadiusz Kubalewski wrote:
> -        i = attr_spec.get('value-start', 0)
>          if 'enum-as-flags' in attr_spec and attr_spec['enum-as-flags']:
> +            i = attr_spec.get('value-start', 0)

Just:
		i = 0
-- 
pw-bot: cr

RE: [PATCH net-next v3 1/2] tools: ynl-gen: fix enum index in _decode_enum(..)

[Kubalewski, Arkadiusz]

>From: Jakub Kicinski <kuba@kernel.org>
>Sent: Wednesday, July 19, 2023 5:17 AM
>
>On Tue, 18 Jul 2023 18:22:24 +0200 Arkadiusz Kubalewski wrote:
>> -        i = attr_spec.get('value-start', 0)
>>          if 'enum-as-flags' in attr_spec and attr_spec['enum-as-flags']:
>> +            i = attr_spec.get('value-start', 0)
>
>Just:
>		i = 0

Sure, will do.

Thank you!
Arkadiusz

>--
>pw-bot: cr

Re: [PATCH net-next v3 1/2] tools: ynl-gen: fix enum index in _decode_enum(..)

[Simon Horman]

On Tue, Jul 18, 2023 at 06:22:24PM +0200, Arkadiusz Kubalewski wrote:
> Remove wrong index adjustement, which is leftover from adding

nit: adjustement -> adjustment

...

RE: [PATCH net-next v3 1/2] tools: ynl-gen: fix enum index in _decode_enum(..)

[Kubalewski, Arkadiusz]

>From: Simon Horman <simon.horman@corigine.com>
>Sent: Friday, July 21, 2023 10:55 AM
>
>On Tue, Jul 18, 2023 at 06:22:24PM +0200, Arkadiusz Kubalewski wrote:
>> Remove wrong index adjustement, which is leftover from adding
>
>nit: adjustement -> adjustment
>

Yes, fixed.

Thank you!
Arkadiusz

>...

[PATCH net-next v3 2/2] 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:
Traceback (most recent call last):
  File "/net-next/tools/net/ynl/./test.py", line 520, in <module>
    main()
  File "/net-next/tools/net/ynl/./test.py", line 488, in main
    dplls=dplls_get(282574471561216)
  File "/net-next/tools/net/ynl/./test.py", line 48, in dplls_get
    reply=act(args)
  File "/net-next/tools/net/ynl/./test.py", line 41, in act
    reply = ynl.dump(args.dump, attrs)
  File "/net-next/tools/net/ynl/lib/ynl.py", line 598, in dump
    return self._op(method, vals, dump=True)
  File "/net-next/tools/net/ynl/lib/ynl.py", line 584, in _op
    rsp_msg = self._decode(gm.raw_attrs, op.attr_set.name)
  File "/net-next/tools/net/ynl/lib/ynl.py", line 451, in _decode
    self._decode_enum(rsp, attr_spec)
  File "/net-next/tools/net/ynl/lib/ynl.py", line 408, in _decode_enum
    value = enum.entries_by_val[raw].name
TypeError: unhashable type: 'list'
error: 1

Redesign _decode_enum(..) to take a enum int value and translate
it to either a bitmask or enum name as expected.

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

diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index 5db7d47067f9..671ef4b5eaa6 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -135,7 +135,7 @@ class NlAttr:
         format = self.get_format(type)
         return [ x[0] for x in format.iter_unpack(self.raw) ]
 
-    def as_struct(self, members):
+    def as_struct(self, members, attr_spec):
         value = dict()
         offset = 0
         for m in members:
@@ -147,6 +147,9 @@ class NlAttr:
                 format = self.get_format(m.type, m.byte_order)
                 [ decoded ] = format.unpack_from(self.raw, offset)
                 offset += format.size
+
+            if m.enum:
+                decoded = self._decode_enum(decoded, attr_spec)
             if m.display_hint:
                 decoded = self.formatted_string(decoded, m.display_hint)
             value[m.name] = decoded
@@ -417,8 +420,7 @@ class YnlFamily(SpecFamily):
         pad = b'\x00' * ((4 - len(attr_payload) % 4) % 4)
         return struct.pack('HH', len(attr_payload) + 4, nl_type) + attr_payload + pad
 
-    def _decode_enum(self, rsp, attr_spec):
-        raw = rsp[attr_spec['name']]
+    def _decode_enum(self, raw, attr_spec):
         enum = self.consts[attr_spec['enum']]
         if 'enum-as-flags' in attr_spec and attr_spec['enum-as-flags']:
             i = attr_spec.get('value-start', 0)
@@ -430,15 +432,12 @@ class YnlFamily(SpecFamily):
                 i += 1
         else:
             value = enum.entries_by_val[raw].name
-        rsp[attr_spec['name']] = value
+        return value
 
     def _decode_binary(self, attr, attr_spec):
         if attr_spec.struct_name:
             members = self.consts[attr_spec.struct_name]
-            decoded = attr.as_struct(members)
-            for m in members:
-                if m.enum:
-                    self._decode_enum(decoded, m)
+            decoded = attr.as_struct(members, attr_spec)
         elif attr_spec.sub_type:
             decoded = attr.as_c_array(attr_spec.sub_type)
         else:
@@ -466,6 +465,9 @@ class YnlFamily(SpecFamily):
             else:
                 raise Exception(f'Unknown {attr_spec["type"]} with name {attr_spec["name"]}')
 
+            if 'enum' in attr_spec:
+                decoded = self._decode_enum(int.from_bytes(attr.raw, "big"), attr_spec)
+
             if not attr_spec.is_multi:
                 rsp[attr_spec['name']] = decoded
             elif attr_spec.name in rsp:
@@ -473,8 +475,6 @@ class YnlFamily(SpecFamily):
             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):
-- 
2.38.1

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

[Jakub Kicinski]

On Tue, 18 Jul 2023 18:22:25 +0200 Arkadiusz Kubalewski wrote:
> +            if 'enum' in attr_spec:
> +                decoded = self._decode_enum(int.from_bytes(attr.raw, "big"), attr_spec)

why int.from_bytes(attr.raw, "big")?
'decoded' already holds the integer at this point

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

[Kubalewski, Arkadiusz]

>From: Jakub Kicinski <kuba@kernel.org>
>Sent: Wednesday, July 19, 2023 5:19 AM
>
>On Tue, 18 Jul 2023 18:22:25 +0200 Arkadiusz Kubalewski wrote:
>> +            if 'enum' in attr_spec:
>> +                decoded = self._decode_enum(int.from_bytes(attr.raw,
>"big"), attr_spec)
>
>why int.from_bytes(attr.raw, "big")?
>'decoded' already holds the integer at this point

Yes, will fix.

Thank you!
Arkadiusz

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

[Donald Hunter]

On Tue, 18 Jul 2023 at 17:24, Arkadiusz Kubalewski
<arkadiusz.kubalewski@intel.com> wrote:
>
>  tools/net/ynl/lib/ynl.py | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
> index 5db7d47067f9..671ef4b5eaa6 100644
> --- a/tools/net/ynl/lib/ynl.py
> +++ b/tools/net/ynl/lib/ynl.py
> @@ -135,7 +135,7 @@ class NlAttr:
>          format = self.get_format(type)
>          return [ x[0] for x in format.iter_unpack(self.raw) ]
>
> -    def as_struct(self, members):
> +    def as_struct(self, members, attr_spec):

No need to pass attr_spec - it's the spec for the struct, not for the members.

>          value = dict()
>          offset = 0
>          for m in members:
> @@ -147,6 +147,9 @@ class NlAttr:
>                  format = self.get_format(m.type, m.byte_order)
>                  [ decoded ] = format.unpack_from(self.raw, offset)
>                  offset += format.size
> +
> +            if m.enum:
> +                decoded = self._decode_enum(decoded, attr_spec)

_decode_enum is not a method on NlAttr so I'm pretty sure this will
fail. Looks like we need to move _decode_enum() into NlAttr?

The second param to _decode_enum should be 'm' which is the attr spec
for the member.

>              if m.display_hint:
>                  decoded = self.formatted_string(decoded, m.display_hint)
>              value[m.name] = decoded
> @@ -417,8 +420,7 @@ class YnlFamily(SpecFamily):
>          pad = b'\x00' * ((4 - len(attr_payload) % 4) % 4)
>          return struct.pack('HH', len(attr_payload) + 4, nl_type) + attr_payload + pad
>
> -    def _decode_enum(self, rsp, attr_spec):
> -        raw = rsp[attr_spec['name']]
> +    def _decode_enum(self, raw, attr_spec):
>          enum = self.consts[attr_spec['enum']]
>          if 'enum-as-flags' in attr_spec and attr_spec['enum-as-flags']:
>              i = attr_spec.get('value-start', 0)
> @@ -430,15 +432,12 @@ class YnlFamily(SpecFamily):
>                  i += 1
>          else:
>              value = enum.entries_by_val[raw].name
> -        rsp[attr_spec['name']] = value
> +        return value
>
>      def _decode_binary(self, attr, attr_spec):
>          if attr_spec.struct_name:
>              members = self.consts[attr_spec.struct_name]
> -            decoded = attr.as_struct(members)
> -            for m in members:
> -                if m.enum:
> -                    self._decode_enum(decoded, m)
> +            decoded = attr.as_struct(members, attr_spec)
>          elif attr_spec.sub_type:
>              decoded = attr.as_c_array(attr_spec.sub_type)
>          else:
> @@ -466,6 +465,9 @@ class YnlFamily(SpecFamily):
>              else:
>                  raise Exception(f'Unknown {attr_spec["type"]} with name {attr_spec["name"]}')
>
> +            if 'enum' in attr_spec:
> +                decoded = self._decode_enum(int.from_bytes(attr.raw, "big"), attr_spec)

As Jakub said, this should just be self._decode_enum(decoded, attr_spec)

> +
>              if not attr_spec.is_multi:
>                  rsp[attr_spec['name']] = decoded
>              elif attr_spec.name in rsp:
> @@ -473,8 +475,6 @@ class YnlFamily(SpecFamily):
>              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):
> --
> 2.38.1

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

[Kubalewski, Arkadiusz]

>From: Donald Hunter <donald.hunter@gmail.com>
>Sent: Wednesday, July 19, 2023 1:18 PM
>
>On Tue, 18 Jul 2023 at 17:24, Arkadiusz Kubalewski
><arkadiusz.kubalewski@intel.com> wrote:
>>
>>  tools/net/ynl/lib/ynl.py | 20 ++++++++++----------
>>  1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
>> index 5db7d47067f9..671ef4b5eaa6 100644
>> --- a/tools/net/ynl/lib/ynl.py
>> +++ b/tools/net/ynl/lib/ynl.py
>> @@ -135,7 +135,7 @@ class NlAttr:
>>          format = self.get_format(type)
>>          return [ x[0] for x in format.iter_unpack(self.raw) ]
>>
>> -    def as_struct(self, members):
>> +    def as_struct(self, members, attr_spec):
>
>No need to pass attr_spec - it's the spec for the struct, not for the
>members.

Yep, that what I was thinking in previous version thread.

>
>>          value = dict()
>>          offset = 0
>>          for m in members:
>> @@ -147,6 +147,9 @@ class NlAttr:
>>                  format = self.get_format(m.type, m.byte_order)
>>                  [ decoded ] = format.unpack_from(self.raw, offset)
>>                  offset += format.size
>> +
>> +            if m.enum:
>> +                decoded = self._decode_enum(decoded, attr_spec)
>
>_decode_enum is not a method on NlAttr so I'm pretty sure this will
>fail. Looks like we need to move _decode_enum() into NlAttr?
>

Are you sure about moving it? it seems all the decode function are members of
YnlFamily.

>The second param to _decode_enum should be 'm' which is the attr spec
>for the member.

Ok, will do. Altough I think of moving back _decode_enum() to the
_decode_binary(), where it was initially, but with proper usage like:
  for m in members:
    if m.enum:
      decoded[m.name] = self._decode_enum(decoded[m.name], m)

This way all the decode function would be kept in YnlFamily.
Does it make sense?

>
>>              if m.display_hint:
>>                  decoded = self.formatted_string(decoded, m.display_hint)
>>              value[m.name] = decoded
>> @@ -417,8 +420,7 @@ class YnlFamily(SpecFamily):
>>          pad = b'\x00' * ((4 - len(attr_payload) % 4) % 4)
>>          return struct.pack('HH', len(attr_payload) + 4, nl_type) +
>>attr_payload + pad
>>
>> -    def _decode_enum(self, rsp, attr_spec):
>> -        raw = rsp[attr_spec['name']]
>> +    def _decode_enum(self, raw, attr_spec):
>>          enum = self.consts[attr_spec['enum']]
>>          if 'enum-as-flags' in attr_spec and attr_spec['enum-as-flags']:
>>              i = attr_spec.get('value-start', 0)
>> @@ -430,15 +432,12 @@ class YnlFamily(SpecFamily):
>>                  i += 1
>>          else:
>>              value = enum.entries_by_val[raw].name
>> -        rsp[attr_spec['name']] = value
>> +        return value
>>
>>      def _decode_binary(self, attr, attr_spec):
>>          if attr_spec.struct_name:
>>              members = self.consts[attr_spec.struct_name]
>> -            decoded = attr.as_struct(members)
>> -            for m in members:
>> -                if m.enum:
>> -                    self._decode_enum(decoded, m)
>> +            decoded = attr.as_struct(members, attr_spec)
>>          elif attr_spec.sub_type:
>>              decoded = attr.as_c_array(attr_spec.sub_type)
>>          else:
>> @@ -466,6 +465,9 @@ class YnlFamily(SpecFamily):
>>              else:
>>                  raise Exception(f'Unknown {attr_spec["type"]} with name
>>{attr_spec["name"]}')
>>
>> +            if 'enum' in attr_spec:
>> +                decoded = self._decode_enum(int.from_bytes(attr.raw,
>>"big"), attr_spec)
>
>As Jakub said, this should just be self._decode_enum(decoded, attr_spec)
>

Yes, will fix.

Thank you!
Arkadiusz

>> +
>>              if not attr_spec.is_multi:
>>                  rsp[attr_spec['name']] = decoded
>>              elif attr_spec.name in rsp:
>> @@ -473,8 +475,6 @@ class YnlFamily(SpecFamily):
>>              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):
>> --
>> 2.38.1