| Message ID | 20230711095323.121131-1-arkadiusz.kubalewski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] tools: ynl-gen: fix parse multi-attr enum attribute | expand |
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?
>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
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?
>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
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):
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(-)