| Message ID | 9644d866cbc6449525144fb3c679e877c427afce.1706800192.git.alessandromarcolini99@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support for encoding multi-attr to ynl | expand |
On Thu, 1 Feb 2024 16:12:51 +0100 Alessandro Marcolini wrote: > Multi-attr elements could not be encoded because of missing logic in the > ynl code. Enable encoding of these attributes by checking if the nest > attribute in the spec contains multi-attr attributes and if the value to > be processed is a list. > > This has been tested both with the taprio and ets qdisc which contain > this kind of attributes. > > Signed-off-by: Alessandro Marcolini <alessandromarcolini99@gmail.com> > --- > tools/net/ynl/lib/ynl.py | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py > index 0f4193cc2e3b..e4e6a3fe0f23 100644 > --- a/tools/net/ynl/lib/ynl.py > +++ b/tools/net/ynl/lib/ynl.py > @@ -447,10 +447,19 @@ class YnlFamily(SpecFamily): > if attr["type"] == 'nest': > nl_type |= Netlink.NLA_F_NESTED > attr_payload = b'' > - sub_attrs = SpaceAttrs(self.attr_sets[space], value, search_attrs) > - for subname, subvalue in value.items(): > - attr_payload += self._add_attr(attr['nested-attributes'], > - subname, subvalue, sub_attrs) > + nested_attrs = self.attr_sets[attr["nested-attributes"]] > + > + if any(v.is_multi for _,v in nested_attrs.items()) and isinstance(value, list): I think you're trying to handle this at the wrong level. The main message can also contain multi-attr, so looping inside nests won't cut it. Early in the function check if attr.is_multi and isinstance(value, list), and if so do: attr_payload = b'' for subvalue in value: attr_payload += self._add_attr(space, name, subvalue, search_attrs) return attr_payload IOW all you need to do is recursively call _add_attr() with the subvalues stripped. You don't have to descend into a nest. > + for item in value: > + sub_attrs = SpaceAttrs(self.attr_sets[space], item, search_attrs) > + for subname, subvalue in item.items(): > + attr_payload += self._add_attr(attr['nested-attributes'], > + subname, subvalue, sub_attrs) > + else: > + sub_attrs = SpaceAttrs(self.attr_sets[space], value, search_attrs) > + for subname, subvalue in value.items(): > + attr_payload += self._add_attr(attr['nested-attributes'], > + subname, subvalue, sub_attrs) > elif attr["type"] == 'flag': > attr_payload = b'' > elif attr["type"] == 'string':
On 2/2/24 02:24, Jakub Kicinski wrote: > I think you're trying to handle this at the wrong level. The main > message can also contain multi-attr, so looping inside nests won't > cut it. > > Early in the function check if attr.is_multi and isinstance(value, > list), and if so do: > > attr_payload = b'' > for subvalue in value: > attr_payload += self._add_attr(space, name, subvalue, > search_attrs) > return attr_payload > > IOW all you need to do is recursively call _add_attr() with the > subvalues stripped. You don't have to descend into a nest. I (wrongly) supposed that multi-attr attributes were always inside a nest (that's because I've only experimented with the tc spec). That's also because I (mistakenly, again) thought that the syntax for specifying a multi-attr would be: "parent-attr":[{multi-attr:{values}}, {multi-attr: {values}}, ... ] Instead of: "optional-parent-attr": {"multi-attr": [{values in multi-attr}, ...]} By reading the docs [1]: "multi-attr (arrays) Boolean property signifying that the attribute may be present multiple times. Allowing an attribute to repeat is the recommended way of implementing arrays (no extra nesting)." I understood that the syntax should be the former (I was thinking of an array containing all the multi-attr attributes, and not only their values), albeit really verbose and not that readable. I've now made the changes as you suggested and tested it, it works as expected! I'll post a v3 soon, thanks for your review :) [1] https://docs.kernel.org/userspace-api/netlink/specs.html#multi-attr-arrays
Alessandro Marcolini <alessandromarcolini99@gmail.com> writes: > On 2/2/24 02:24, Jakub Kicinski wrote: >> I think you're trying to handle this at the wrong level. The main >> message can also contain multi-attr, so looping inside nests won't >> cut it. >> >> Early in the function check if attr.is_multi and isinstance(value, >> list), and if so do: >> >> attr_payload = b'' >> for subvalue in value: >> attr_payload += self._add_attr(space, name, subvalue, >> search_attrs) >> return attr_payload >> >> IOW all you need to do is recursively call _add_attr() with the >> subvalues stripped. You don't have to descend into a nest. > > I (wrongly) supposed that multi-attr attributes were always inside a nest (that's because I've > only experimented with the tc spec). That's also because I (mistakenly, again) thought that the > syntax for specifying a multi-attr would be: > "parent-attr":[{multi-attr:{values}}, {multi-attr: {values}}, ... ] > Instead of: > "optional-parent-attr": {"multi-attr": [{values in multi-attr}, ...]} > > By reading the docs [1]: > "multi-attr (arrays) > Boolean property signifying that the attribute may be present multiple times. Allowing an > attribute to repeat is the recommended way of implementing arrays (no extra nesting)." > > I understood that the syntax should be the former (I was thinking of an array containing all the > multi-attr attributes, and not only their values), albeit really verbose and not that readable. > > I've now made the changes as you suggested and tested it, it works as expected! > I'll post a v3 soon, thanks for your review :) > > [1] https://docs.kernel.org/userspace-api/netlink/specs.html#multi-attr-arrays Yes, if your input matches the ynl output then you should be good: "sched-entry-list": { "entry": [ { "index": 0, "cmd": 0, "gate-mask": 1, "interval": 500000 }, { "index": 1, "cmd": 0, "gate-mask": 1, "interval": 500000 } ] }
On 2/2/24 12:42, Donald Hunter wrote: > Yes, if your input matches the ynl output then you should be good: > > "sched-entry-list": { > "entry": [ > { > "index": 0, > "cmd": 0, > "gate-mask": 1, > "interval": 500000 > }, > { > "index": 1, > "cmd": 0, > "gate-mask": 1, > "interval": 500000 > } > ] > } Yes, I confirm that this is the input I'm passing to ynl now, thanks! :)
diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py index 0f4193cc2e3b..e4e6a3fe0f23 100644 --- a/tools/net/ynl/lib/ynl.py +++ b/tools/net/ynl/lib/ynl.py @@ -447,10 +447,19 @@ class YnlFamily(SpecFamily): if attr["type"] == 'nest': nl_type |= Netlink.NLA_F_NESTED attr_payload = b'' - sub_attrs = SpaceAttrs(self.attr_sets[space], value, search_attrs) - for subname, subvalue in value.items(): - attr_payload += self._add_attr(attr['nested-attributes'], - subname, subvalue, sub_attrs) + nested_attrs = self.attr_sets[attr["nested-attributes"]] + + if any(v.is_multi for _,v in nested_attrs.items()) and isinstance(value, list): + for item in value: + sub_attrs = SpaceAttrs(self.attr_sets[space], item, search_attrs) + for subname, subvalue in item.items(): + attr_payload += self._add_attr(attr['nested-attributes'], + subname, subvalue, sub_attrs) + else: + sub_attrs = SpaceAttrs(self.attr_sets[space], value, search_attrs) + for subname, subvalue in value.items(): + attr_payload += self._add_attr(attr['nested-attributes'], + subname, subvalue, sub_attrs) elif attr["type"] == 'flag': attr_payload = b'' elif attr["type"] == 'string':
Multi-attr elements could not be encoded because of missing logic in the ynl code. Enable encoding of these attributes by checking if the nest attribute in the spec contains multi-attr attributes and if the value to be processed is a list. This has been tested both with the taprio and ets qdisc which contain this kind of attributes. Signed-off-by: Alessandro Marcolini <alessandromarcolini99@gmail.com> --- tools/net/ynl/lib/ynl.py | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)