| Message ID | 0eedc19860e9b84f105c57d17219b3d0af3100d2.1705950652.git.alessandromarcolini99@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tools: ynl: Add sub-message and multi-attr encoding support | expand |
On Mon, Jan 22, 2024 at 08:19:41PM +0100, Alessandro Marcolini wrote: > Add encoding support for 'sub-message' attribute and for resolving > sub-message selectors at different nesting level from the key > attribute. > > Also, add encoding support for multi-attr attributes. > > Signed-off-by: Alessandro Marcolini <alessandromarcolini99@gmail.com> > --- > tools/net/ynl/lib/ynl.py | 54 +++++++++++++++++++++++++++++++++++----- > 1 file changed, 48 insertions(+), 6 deletions(-) > > diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py > index 1e10512b2117..f8c56944f7e7 100644 > --- a/tools/net/ynl/lib/ynl.py > +++ b/tools/net/ynl/lib/ynl.py > @@ -449,7 +449,7 @@ class YnlFamily(SpecFamily): > self.sock.setsockopt(Netlink.SOL_NETLINK, Netlink.NETLINK_ADD_MEMBERSHIP, > mcast_id) > > - def _add_attr(self, space, name, value): > + def _add_attr(self, space, name, value, vals): > try: > attr = self.attr_sets[space][name] > except KeyError: > @@ -458,8 +458,13 @@ class YnlFamily(SpecFamily): > if attr["type"] == 'nest': > nl_type |= Netlink.NLA_F_NESTED > attr_payload = b'' > - for subname, subvalue in value.items(): > - attr_payload += self._add_attr(attr['nested-attributes'], subname, subvalue) > + # Check if it's a list of values (i.e. it contains multi-attr elements) > + for subname, subvalue in ( > + ((k, v) for item in value for k, v in item.items()) > + if isinstance(value, list) > + else value.items() > + ): This is a bit hard to read. Is it possible to make it a bit easier to read?
On 1/22/24 20:46, Breno Leitao wrote: > This is a bit hard to read. > > Is it possible to make it a bit easier to read? Hi! Yes, an easier to read version could be: diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py index f8c56944f7e7..d837e769c5bf 100644 --- a/tools/net/ynl/lib/ynl.py +++ b/tools/net/ynl/lib/ynl.py @@ -459,12 +459,13 @@ class YnlFamily(SpecFamily): nl_type |= Netlink.NLA_F_NESTED attr_payload = b'' # Check if it's a list of values (i.e. it contains multi-attr elements) - for subname, subvalue in ( - ((k, v) for item in value for k, v in item.items()) - if isinstance(value, list) - else value.items() - ): - attr_payload += self._add_attr(attr['nested-attributes'], subname, subvalue, vals) + if isinstance(value, list): + for item in value: + for subname, subvalue in item.items(): + attr_payload += self._add_attr(attr['nested-attributes'], subname, subvalue, vals) + else: + for subname, subvalue in value.items(): + attr_payload += self._add_attr(attr['nested-attributes'], subname, subvalue, vals) elif attr["type"] == 'flag': attr_payload = b'' elif attr["type"] == 'string':
Alessandro Marcolini <alessandromarcolini99@gmail.com> writes: > Add encoding support for 'sub-message' attribute and for resolving > sub-message selectors at different nesting level from the key > attribute. I think the relevant patches in my series are: https://lore.kernel.org/netdev/20240123160538.172-3-donald.hunter@gmail.com/T/#u https://lore.kernel.org/netdev/20240123160538.172-5-donald.hunter@gmail.com/T/#u > > Also, add encoding support for multi-attr attributes. This would be better as a separate patch since it is unrelated to the other changes. > Signed-off-by: Alessandro Marcolini <alessandromarcolini99@gmail.com> > --- > tools/net/ynl/lib/ynl.py | 54 +++++++++++++++++++++++++++++++++++----- > 1 file changed, 48 insertions(+), 6 deletions(-) > > diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py > index 1e10512b2117..f8c56944f7e7 100644 > --- a/tools/net/ynl/lib/ynl.py > +++ b/tools/net/ynl/lib/ynl.py > @@ -449,7 +449,7 @@ class YnlFamily(SpecFamily): > self.sock.setsockopt(Netlink.SOL_NETLINK, Netlink.NETLINK_ADD_MEMBERSHIP, > mcast_id) > > - def _add_attr(self, space, name, value): > + def _add_attr(self, space, name, value, vals): > try: > attr = self.attr_sets[space][name] > except KeyError: > @@ -458,8 +458,13 @@ class YnlFamily(SpecFamily): > if attr["type"] == 'nest': > nl_type |= Netlink.NLA_F_NESTED > attr_payload = b'' > - for subname, subvalue in value.items(): > - attr_payload += self._add_attr(attr['nested-attributes'], subname, subvalue) > + # Check if it's a list of values (i.e. it contains multi-attr elements) > + for subname, subvalue in ( > + ((k, v) for item in value for k, v in item.items()) > + if isinstance(value, list) > + else value.items() > + ): > + attr_payload += self._add_attr(attr['nested-attributes'], subname, subvalue, vals) Should really check whether multi-attr is true in the spec before processing the json input as a list of values. > elif attr["type"] == 'flag': > attr_payload = b'' > elif attr["type"] == 'string': > @@ -481,6 +486,12 @@ class YnlFamily(SpecFamily): > attr_payload = format.pack(int(value)) > elif attr['type'] in "bitfield32": > attr_payload = struct.pack("II", int(value["value"]), int(value["selector"])) > + elif attr['type'] == "sub-message": > + spec = self._resolve_selector(attr, vals) > + attr_spec = spec["attribute-set"] > + attr_payload = b'' > + for subname, subvalue in value.items(): > + attr_payload += self._add_attr(attr_spec, subname, subvalue, vals) > else: > raise Exception(f'Unknown type at {space} {name} {value} {attr["type"]}') > > @@ -555,9 +566,40 @@ class YnlFamily(SpecFamily): > sub_msg_spec = self.sub_msgs[sub_msg] > > selector = attr_spec.selector > - if selector not in vals: > + > + def _find_attr_path(attr, vals, path=None): > + if path is None: > + path = [] > + if isinstance(vals, dict): > + if attr in vals: > + return path > + for k, v in vals.items(): > + result = _find_attr_path(attr, v, path + [k]) > + if result is not None: > + return result > + elif isinstance(vals, list): > + for idx, v in enumerate(vals): > + result = _find_attr_path(attr, v, path + [idx]) > + if result is not None: > + return result > + return None > + > + def _find_selector_val(sel, vals, path): > + while path != []: > + v = vals.copy() > + for step in path: > + v = v[step] > + if sel in v: > + return v[sel] > + path.pop() > + return vals[sel] if sel in vals else None > + > + attr_path = _find_attr_path(attr_spec.name, vals) > + value = _find_selector_val(selector, vals, attr_path) > + > + if value is None: > raise Exception(f"There is no value for {selector} to resolve '{attr_spec.name}'") > - value = vals[selector] > + > if value not in sub_msg_spec.formats: > raise Exception(f"No message format for '{value}' in sub-message spec '{sub_msg}'") > > @@ -772,7 +814,7 @@ class YnlFamily(SpecFamily): > format = NlAttr.get_format(m.type, m.byte_order) > msg += format.pack(value) > for name, value in vals.items(): > - msg += self._add_attr(op.attr_set.name, name, value) > + msg += self._add_attr(op.attr_set.name, name, value, vals) > msg = _genl_msg_finalize(msg) > > self.sock.send(msg, 0)
On 1/23/24 17:44, Donald Hunter wrote: > Alessandro Marcolini <alessandromarcolini99@gmail.com> writes: > >> Add encoding support for 'sub-message' attribute and for resolving >> sub-message selectors at different nesting level from the key >> attribute. > I think the relevant patches in my series are: > > https://lore.kernel.org/netdev/20240123160538.172-3-donald.hunter@gmail.com/T/#u > https://lore.kernel.org/netdev/20240123160538.172-5-donald.hunter@gmail.com/T/#u I really like your idea of using ChainMap, I think it's better than mine and more concise. >> Also, add encoding support for multi-attr attributes. > This would be better as a separate patch since it is unrelated to the > other changes. You mean as a separate patch in this patchset or as an entirely new patch? >> Signed-off-by: Alessandro Marcolini <alessandromarcolini99@gmail.com> >> --- >> tools/net/ynl/lib/ynl.py | 54 +++++++++++++++++++++++++++++++++++----- >> 1 file changed, 48 insertions(+), 6 deletions(-) >> >> diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py >> index 1e10512b2117..f8c56944f7e7 100644 >> --- a/tools/net/ynl/lib/ynl.py >> +++ b/tools/net/ynl/lib/ynl.py >> @@ -449,7 +449,7 @@ class YnlFamily(SpecFamily): >> self.sock.setsockopt(Netlink.SOL_NETLINK, Netlink.NETLINK_ADD_MEMBERSHIP, >> mcast_id) >> >> - def _add_attr(self, space, name, value): >> + def _add_attr(self, space, name, value, vals): >> try: >> attr = self.attr_sets[space][name] >> except KeyError: >> @@ -458,8 +458,13 @@ class YnlFamily(SpecFamily): >> if attr["type"] == 'nest': >> nl_type |= Netlink.NLA_F_NESTED >> attr_payload = b'' >> - for subname, subvalue in value.items(): >> - attr_payload += self._add_attr(attr['nested-attributes'], subname, subvalue) >> + # Check if it's a list of values (i.e. it contains multi-attr elements) >> + for subname, subvalue in ( >> + ((k, v) for item in value for k, v in item.items()) >> + if isinstance(value, list) >> + else value.items() >> + ): >> + attr_payload += self._add_attr(attr['nested-attributes'], subname, subvalue, vals) > Should really check whether multi-attr is true in the spec before > processing the json input as a list of values. Yes, you're right. Maybe I could resend this on top of your changes, what do you think?
Alessandro Marcolini <alessandromarcolini99@gmail.com> writes: > On 1/23/24 17:44, Donald Hunter wrote: >> Alessandro Marcolini <alessandromarcolini99@gmail.com> writes: >> >>> Add encoding support for 'sub-message' attribute and for resolving >>> sub-message selectors at different nesting level from the key >>> attribute. >> I think the relevant patches in my series are: >> >> https://lore.kernel.org/netdev/20240123160538.172-3-donald.hunter@gmail.com/T/#u >> https://lore.kernel.org/netdev/20240123160538.172-5-donald.hunter@gmail.com/T/#u > I really like your idea of using ChainMap, I think it's better than mine and more concise. >>> Also, add encoding support for multi-attr attributes. >> This would be better as a separate patch since it is unrelated to the >> other changes. > You mean as a separate patch in this patchset or as an entirely new patch? I was thinking as a separate patch in this patchset, yes. >>> Signed-off-by: Alessandro Marcolini <alessandromarcolini99@gmail.com> >>> --- >>> tools/net/ynl/lib/ynl.py | 54 +++++++++++++++++++++++++++++++++++----- >>> 1 file changed, 48 insertions(+), 6 deletions(-) >>> >>> diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py >>> index 1e10512b2117..f8c56944f7e7 100644 >>> --- a/tools/net/ynl/lib/ynl.py >>> +++ b/tools/net/ynl/lib/ynl.py >>> @@ -449,7 +449,7 @@ class YnlFamily(SpecFamily): >>> self.sock.setsockopt(Netlink.SOL_NETLINK, Netlink.NETLINK_ADD_MEMBERSHIP, >>> mcast_id) >>> >>> - def _add_attr(self, space, name, value): >>> + def _add_attr(self, space, name, value, vals): >>> try: >>> attr = self.attr_sets[space][name] >>> except KeyError: >>> @@ -458,8 +458,13 @@ class YnlFamily(SpecFamily): >>> if attr["type"] == 'nest': >>> nl_type |= Netlink.NLA_F_NESTED >>> attr_payload = b'' >>> - for subname, subvalue in value.items(): >>> - attr_payload += self._add_attr(attr['nested-attributes'], subname, subvalue) >>> + # Check if it's a list of values (i.e. it contains multi-attr elements) >>> + for subname, subvalue in ( >>> + ((k, v) for item in value for k, v in item.items()) >>> + if isinstance(value, list) >>> + else value.items() >>> + ): >>> + attr_payload += self._add_attr(attr['nested-attributes'], subname, subvalue, vals) >> Should really check whether multi-attr is true in the spec before >> processing the json input as a list of values. > Yes, you're right. Maybe I could resend this on top of your changes, what do you think? Yes, that would be great. Thanks!
diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py index 1e10512b2117..f8c56944f7e7 100644 --- a/tools/net/ynl/lib/ynl.py +++ b/tools/net/ynl/lib/ynl.py @@ -449,7 +449,7 @@ class YnlFamily(SpecFamily): self.sock.setsockopt(Netlink.SOL_NETLINK, Netlink.NETLINK_ADD_MEMBERSHIP, mcast_id) - def _add_attr(self, space, name, value): + def _add_attr(self, space, name, value, vals): try: attr = self.attr_sets[space][name] except KeyError: @@ -458,8 +458,13 @@ class YnlFamily(SpecFamily): if attr["type"] == 'nest': nl_type |= Netlink.NLA_F_NESTED attr_payload = b'' - for subname, subvalue in value.items(): - attr_payload += self._add_attr(attr['nested-attributes'], subname, subvalue) + # Check if it's a list of values (i.e. it contains multi-attr elements) + for subname, subvalue in ( + ((k, v) for item in value for k, v in item.items()) + if isinstance(value, list) + else value.items() + ): + attr_payload += self._add_attr(attr['nested-attributes'], subname, subvalue, vals) elif attr["type"] == 'flag': attr_payload = b'' elif attr["type"] == 'string': @@ -481,6 +486,12 @@ class YnlFamily(SpecFamily): attr_payload = format.pack(int(value)) elif attr['type'] in "bitfield32": attr_payload = struct.pack("II", int(value["value"]), int(value["selector"])) + elif attr['type'] == "sub-message": + spec = self._resolve_selector(attr, vals) + attr_spec = spec["attribute-set"] + attr_payload = b'' + for subname, subvalue in value.items(): + attr_payload += self._add_attr(attr_spec, subname, subvalue, vals) else: raise Exception(f'Unknown type at {space} {name} {value} {attr["type"]}') @@ -555,9 +566,40 @@ class YnlFamily(SpecFamily): sub_msg_spec = self.sub_msgs[sub_msg] selector = attr_spec.selector - if selector not in vals: + + def _find_attr_path(attr, vals, path=None): + if path is None: + path = [] + if isinstance(vals, dict): + if attr in vals: + return path + for k, v in vals.items(): + result = _find_attr_path(attr, v, path + [k]) + if result is not None: + return result + elif isinstance(vals, list): + for idx, v in enumerate(vals): + result = _find_attr_path(attr, v, path + [idx]) + if result is not None: + return result + return None + + def _find_selector_val(sel, vals, path): + while path != []: + v = vals.copy() + for step in path: + v = v[step] + if sel in v: + return v[sel] + path.pop() + return vals[sel] if sel in vals else None + + attr_path = _find_attr_path(attr_spec.name, vals) + value = _find_selector_val(selector, vals, attr_path) + + if value is None: raise Exception(f"There is no value for {selector} to resolve '{attr_spec.name}'") - value = vals[selector] + if value not in sub_msg_spec.formats: raise Exception(f"No message format for '{value}' in sub-message spec '{sub_msg}'") @@ -772,7 +814,7 @@ class YnlFamily(SpecFamily): format = NlAttr.get_format(m.type, m.byte_order) msg += format.pack(value) for name, value in vals.items(): - msg += self._add_attr(op.attr_set.name, name, value) + msg += self._add_attr(op.attr_set.name, name, value, vals) msg = _genl_msg_finalize(msg) self.sock.send(msg, 0)
Add encoding support for 'sub-message' attribute and for resolving sub-message selectors at different nesting level from the key attribute. Also, add encoding support for multi-attr attributes. Signed-off-by: Alessandro Marcolini <alessandromarcolini99@gmail.com> --- tools/net/ynl/lib/ynl.py | 54 +++++++++++++++++++++++++++++++++++----- 1 file changed, 48 insertions(+), 6 deletions(-)