[PATCH v2 net-next 0/3] Add support for encoding multi-attr to ynl

[PATCH v2 net-next 0/3] Add support for encoding multi-attr to ynl

[Alessandro Marcolini]

This patchset add the support for encoding multi-attr attributes, making
it possible to use ynl with qdisc which have this kind of attributes
(e.g: taprio, ets).

Patch 1 corrects two docstrings in nlspec.py
Patch 2 adds the multi-attr attribute to taprio entry
Patch 3 adds the support for encoding multi-attr

v1 --> v2:
- Use SearchAttrs instead of ChainMap

Alessandro Marcolini (3):
  tools: ynl: correct typo and docstring
  doc: netlink: specs: tc: add multi-attr to tc-taprio-sched-entry
  tools: ynl: add support for encoding multi-attr

 Documentation/netlink/specs/tc.yaml |  1 +
 tools/net/ynl/lib/nlspec.py         |  7 +++----
 tools/net/ynl/lib/ynl.py            | 17 +++++++++++++----
 3 files changed, 17 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH v2 net-next 1/3] tools: ynl: correct typo and docstring

[Alessandro Marcolini]

Correct typo in SpecAttr docstring. Changed SpecSubMessageFormat
docstring.

Signed-off-by: Alessandro Marcolini <alessandromarcolini99@gmail.com>
Reviewed-by: Donald Hunter <donald.hunter@gmail.com>
---
 tools/net/ynl/lib/nlspec.py | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/net/ynl/lib/nlspec.py b/tools/net/ynl/lib/nlspec.py
index 5d197a12ab8d..fbce52395b3b 100644
--- a/tools/net/ynl/lib/nlspec.py
+++ b/tools/net/ynl/lib/nlspec.py
@@ -144,7 +144,7 @@ class SpecEnumSet(SpecElement):
 
 
 class SpecAttr(SpecElement):
-    """ Single Netlink atttribute type
+    """ Single Netlink attribute type
 
     Represents a single attribute type within an attr space.
 
@@ -308,10 +308,9 @@ class SpecSubMessage(SpecElement):
 
 
 class SpecSubMessageFormat(SpecElement):
-    """ Netlink sub-message definition
+    """ Netlink sub-message format definition
 
-    Represents a set of sub-message formats for polymorphic nlattrs
-    that contain type-specific sub messages.
+    Represents a single format for a sub-message.
 
     Attributes:
         value         attribute value to match against type selector
-- 
2.43.0

[PATCH v2 net-next 2/3] doc: netlink: specs: tc: add multi-attr to tc-taprio-sched-entry

[Alessandro Marcolini]

Add multi-attr attribute to tc-taprio-sched-entry to specify multiple
entries.

Signed-off-by: Alessandro Marcolini <alessandromarcolini99@gmail.com>
---
 Documentation/netlink/specs/tc.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/netlink/specs/tc.yaml b/Documentation/netlink/specs/tc.yaml
index 4b21b00dbebe..324fa182cd14 100644
--- a/Documentation/netlink/specs/tc.yaml
+++ b/Documentation/netlink/specs/tc.yaml
@@ -3376,6 +3376,7 @@ attribute-sets:
         name: entry
         type: nest
         nested-attributes: tc-taprio-sched-entry
+        multi-attr: true
   -
     name: tc-taprio-sched-entry
     attributes:
-- 
2.43.0

[PATCH v2 net-next 3/3] tools: ynl: add support for encoding multi-attr

[Alessandro Marcolini]

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):
+                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':
-- 
2.43.0

Re: [PATCH v2 net-next 2/3] doc: netlink: specs: tc: add multi-attr to tc-taprio-sched-entry

[Donald Hunter]

Alessandro Marcolini <alessandromarcolini99@gmail.com> writes:

> Add multi-attr attribute to tc-taprio-sched-entry to specify multiple
> entries.
>
> Signed-off-by: Alessandro Marcolini <alessandromarcolini99@gmail.com>

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>

Re: [PATCH v2 net-next 3/3] tools: ynl: add support for encoding multi-attr

[Jakub Kicinski]

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':

Re: [PATCH v2 net-next 3/3] tools: ynl: add support for encoding multi-attr

[Alessandro Marcolini]

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

Re: [PATCH v2 net-next 3/3] tools: ynl: add support for encoding multi-attr

[Donald Hunter]

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
  }
 ]
}

Re: [PATCH v2 net-next 3/3] tools: ynl: add support for encoding multi-attr

[Alessandro Marcolini]

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! :)