Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | Date | Tue, 11 Jul 2023 20:59:53 -0700 | From | Jakub Kicinski <> | Subject | Re: [PATCH net-next] tools: ynl-gen: fix parse multi-attr enum attribute |
| |
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
| ![\](/images/icornerr.gif) |