[PATCH net-next 0/2] ynl: rename array-nest to indexed-array

[PATCH net-next 0/2] ynl: rename array-nest to indexed-array

[Hangbin Liu]

rename array-nest to indexed-array and add un-nest sub-type support

Hangbin Liu (2):
  ynl: rename array-nest to indexed-array
  ynl: support un-nest sub-type for indexed-array

 Documentation/netlink/genetlink-c.yaml      |  2 +-
 Documentation/netlink/genetlink-legacy.yaml |  2 +-
 Documentation/netlink/genetlink.yaml        |  2 +-
 Documentation/netlink/netlink-raw.yaml      |  2 +-
 Documentation/netlink/specs/nlctrl.yaml     |  6 ++++--
 Documentation/netlink/specs/rt_link.yaml    |  3 ++-
 Documentation/netlink/specs/tc.yaml         | 21 +++++++++++++-------
 tools/net/ynl/lib/ynl.py                    | 22 +++++++++++++++++++--
 tools/net/ynl/ynl-gen-c.py                  | 17 +++++++++-------
 9 files changed, 54 insertions(+), 23 deletions(-)

-- 
2.43.0

[PATCH net-next 1/2] ynl: rename array-nest to indexed-array

[Hangbin Liu]

Some implementations, like bonding, has nest array with same attr type.
To support all kinds of entries under one nest array. As disscussed[1],
let's rename array-nest to indexed-array, and assuming the value is
a nest by passing the type via sub-type.

[1] https://lore.kernel.org/netdev/20240312100105.16a59086@kernel.org/

Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 Documentation/netlink/genetlink-c.yaml      |  2 +-
 Documentation/netlink/genetlink-legacy.yaml |  2 +-
 Documentation/netlink/genetlink.yaml        |  2 +-
 Documentation/netlink/netlink-raw.yaml      |  2 +-
 Documentation/netlink/specs/nlctrl.yaml     |  6 ++++--
 Documentation/netlink/specs/rt_link.yaml    |  3 ++-
 Documentation/netlink/specs/tc.yaml         | 21 ++++++++++++++-------
 tools/net/ynl/lib/ynl.py                    |  5 +++--
 tools/net/ynl/ynl-gen-c.py                  | 17 ++++++++++-------
 9 files changed, 37 insertions(+), 23 deletions(-)

diff --git a/Documentation/netlink/genetlink-c.yaml b/Documentation/netlink/genetlink-c.yaml
index 4dfd899a1661..4f803eaac6d8 100644
--- a/Documentation/netlink/genetlink-c.yaml
+++ b/Documentation/netlink/genetlink-c.yaml
@@ -158,7 +158,7 @@ properties:
               type: &attr-type
                 enum: [ unused, pad, flag, binary,
                         uint, sint, u8, u16, u32, u64, s32, s64,
-                        string, nest, array-nest, nest-type-value ]
+                        string, nest, indexed-array, nest-type-value ]
               doc:
                 description: Documentation of the attribute.
                 type: string
diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml
index b48ad3b1cc32..8db0e22fa72c 100644
--- a/Documentation/netlink/genetlink-legacy.yaml
+++ b/Documentation/netlink/genetlink-legacy.yaml
@@ -201,7 +201,7 @@ properties:
                 description: The netlink attribute type
                 enum: [ unused, pad, flag, binary, bitfield32,
                         uint, sint, u8, u16, u32, u64, s32, s64,
-                        string, nest, array-nest, nest-type-value ]
+                        string, nest, indexed-array, nest-type-value ]
               doc:
                 description: Documentation of the attribute.
                 type: string
diff --git a/Documentation/netlink/genetlink.yaml b/Documentation/netlink/genetlink.yaml
index ebd6ee743fcc..b036227b46f1 100644
--- a/Documentation/netlink/genetlink.yaml
+++ b/Documentation/netlink/genetlink.yaml
@@ -124,7 +124,7 @@ properties:
               type: &attr-type
                 enum: [ unused, pad, flag, binary,
                         uint, sint, u8, u16, u32, u64, s32, s64,
-                        string, nest, array-nest, nest-type-value ]
+                        string, nest, indexed-array, nest-type-value ]
               doc:
                 description: Documentation of the attribute.
                 type: string
diff --git a/Documentation/netlink/netlink-raw.yaml b/Documentation/netlink/netlink-raw.yaml
index a76e54cbadbc..914aa1c0a273 100644
--- a/Documentation/netlink/netlink-raw.yaml
+++ b/Documentation/netlink/netlink-raw.yaml
@@ -222,7 +222,7 @@ properties:
                 description: The netlink attribute type
                 enum: [ unused, pad, flag, binary, bitfield32,
                         u8, u16, u32, u64, s8, s16, s32, s64,
-                        string, nest, array-nest, nest-type-value,
+                        string, nest, indexed-array, nest-type-value,
                         sub-message ]
               doc:
                 description: Documentation of the attribute.
diff --git a/Documentation/netlink/specs/nlctrl.yaml b/Documentation/netlink/specs/nlctrl.yaml
index b1632b95f725..a36535350bdb 100644
--- a/Documentation/netlink/specs/nlctrl.yaml
+++ b/Documentation/netlink/specs/nlctrl.yaml
@@ -65,11 +65,13 @@ attribute-sets:
         type: u32
       -
         name: ops
-        type: array-nest
+        type: indexed-array
+        sub-type: nest
         nested-attributes: op-attrs
       -
         name: mcast-groups
-        type: array-nest
+        type: indexed-array
+        sub-type: nest
         nested-attributes: mcast-group-attrs
       -
         name: policy
diff --git a/Documentation/netlink/specs/rt_link.yaml b/Documentation/netlink/specs/rt_link.yaml
index 8e4d19adee8c..08bec2537a63 100644
--- a/Documentation/netlink/specs/rt_link.yaml
+++ b/Documentation/netlink/specs/rt_link.yaml
@@ -1617,7 +1617,8 @@ attribute-sets:
         type: binary
       -
         name: hw-s-info
-        type: array-nest
+        type: indexed-array
+        sub-type: nest
         nested-attributes: hw-s-info-one
       -
         name: l3-stats
diff --git a/Documentation/netlink/specs/tc.yaml b/Documentation/netlink/specs/tc.yaml
index 324fa182cd14..dbcf19e494ec 100644
--- a/Documentation/netlink/specs/tc.yaml
+++ b/Documentation/netlink/specs/tc.yaml
@@ -1937,7 +1937,8 @@ attribute-sets:
         nested-attributes: tc-ematch-attrs
       -
         name: act
-        type: array-nest
+        type: indexed-array
+        sub-type: nest
         nested-attributes: tc-act-attrs
       -
         name: police
@@ -2077,7 +2078,8 @@ attribute-sets:
         type: u32
       -
         name: tin-stats
-        type: array-nest
+        type: indexed-array
+        sub-type: nest
         nested-attributes: tc-cake-tin-stats-attrs
       -
         name: deficit
@@ -2297,7 +2299,8 @@ attribute-sets:
         type: string
       -
         name: act
-        type: array-nest
+        type: indexed-array
+        sub-type: nest
         nested-attributes: tc-act-attrs
       -
         name: key-eth-dst
@@ -2798,7 +2801,8 @@ attribute-sets:
         type: string
       -
         name: act
-        type: array-nest
+        type: indexed-array
+        sub-type: nest
         nested-attributes: tc-act-attrs
       -
         name: mask
@@ -2951,7 +2955,8 @@ attribute-sets:
         type: u32
       -
         name: act
-        type: array-nest
+        type: indexed-array
+        sub-type: nest
         nested-attributes: tc-act-attrs
       -
         name: flags
@@ -3324,7 +3329,8 @@ attribute-sets:
         nested-attributes: tc-police-attrs
       -
         name: act
-        type: array-nest
+        type: indexed-array
+        sub-type: nest
         nested-attributes: tc-act-attrs
   -
     name: tc-taprio-attrs
@@ -3542,7 +3548,8 @@ attribute-sets:
         nested-attributes: tc-police-attrs
       -
         name: act
-        type: array-nest
+        type: indexed-array
+        sub-type: nest
         nested-attributes: tc-act-attrs
       -
         name: indev
diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index 5fa7957f6e0f..7239e673a28a 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -686,8 +686,9 @@ class YnlFamily(SpecFamily):
                 decoded = attr.as_scalar(attr_spec['type'], attr_spec.byte_order)
                 if 'enum' in attr_spec:
                     decoded = self._decode_enum(decoded, attr_spec)
-            elif attr_spec["type"] == 'array-nest':
-                decoded = self._decode_array_nest(attr, attr_spec)
+            elif attr_spec["type"] == 'indexed-array' and 'sub-type' in attr_spec:
+                if attr_spec["sub-type"] == 'nest':
+                    decoded = self._decode_array_nest(attr, attr_spec)
             elif attr_spec["type"] == 'bitfield32':
                 value, selector = struct.unpack("II", attr.raw)
                 if 'enum' in attr_spec:
diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index 6b7eb2d2aaf1..8d5ec5449648 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -838,8 +838,9 @@ class AttrSet(SpecAttrSet):
             t = TypeBitfield32(self.family, self, elem, value)
         elif elem['type'] == 'nest':
             t = TypeNest(self.family, self, elem, value)
-        elif elem['type'] == 'array-nest':
-            t = TypeArrayNest(self.family, self, elem, value)
+        elif elem['type'] == 'indexed-array' and 'sub-type' in elem:
+            if elem["sub-type"] == 'nest':
+                t = TypeArrayNest(self.family, self, elem, value)
         elif elem['type'] == 'nest-type-value':
             t = TypeNestTypeValue(self.family, self, elem, value)
         else:
@@ -1052,8 +1053,9 @@ class Family(SpecFamily):
                     if nested in self.root_sets:
                         raise Exception("Inheriting members to a space used as root not supported")
                     inherit.update(set(spec['type-value']))
-                elif spec['type'] == 'array-nest':
-                    inherit.add('idx')
+                elif spec['type'] == 'indexed-array' and 'sub-type' in spec:
+                    if spec["sub-type"] == 'nest':
+                        inherit.add('idx')
                 self.pure_nested_structs[nested].set_inherited(inherit)
 
         for root_set, rs_members in self.root_sets.items():
@@ -1616,9 +1618,10 @@ def _multi_parse(ri, struct, init_lines, local_vars):
     multi_attrs = set()
     needs_parg = False
     for arg, aspec in struct.member_list():
-        if aspec['type'] == 'array-nest':
-            local_vars.append(f'const struct nlattr *attr_{aspec.c_name};')
-            array_nests.add(arg)
+        if aspec['type'] == 'indexed-array' and 'sub-type' in aspec:
+            if aspec["sub-type"] == 'nest':
+                local_vars.append(f'const struct nlattr *attr_{aspec.c_name};')
+                array_nests.add(arg)
         if 'multi-attr' in aspec:
             multi_attrs.add(arg)
         needs_parg |= 'nested-attributes' in aspec
-- 
2.43.0

[PATCH net-next 2/2] ynl: support un-nest sub-type for indexed-array

[Hangbin Liu]

Support un-nest sub-type for indexed-array. Since all the attr types are
same for un-nest sub-ype, the index number is used as attr name.
The result would look like:

 # ip link add bond0 type bond mode 1 \
   arp_ip_target 192.168.1.1,192.168.1.2 ns_ip6_target 2001::1,2001::2
 # ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/rt_link.yaml \
   --do getlink --json '{"ifname": "bond0"}' --output-json | jq '.linkinfo'

    "arp-ip-target": [
      {
        "1": "192.168.1.1"
      },
      {
        "2": "192.168.1.2"
      }
    ],

    [...]

    "ns-ip6-target": [
      {
        "1": "2001::1"
      },
      {
        "2": "2001::2"
      }
    ],

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 tools/net/ynl/lib/ynl.py | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index 7239e673a28a..58a602ff9544 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -595,6 +595,21 @@ class YnlFamily(SpecFamily):
             decoded.append({ item.type: subattrs })
         return decoded
 
+    def _decode_index_array(self, attr, attr_spec):
+        decoded = []
+        offset = 0
+        index = 0
+        while offset < len(attr.raw):
+            index = index + 1
+            item = NlAttr(attr.raw, offset)
+            offset += item.full_len
+
+            subattrs = item.as_bin()
+            if attr_spec.display_hint:
+                subattrs = self._formatted_string(subattrs, attr_spec.display_hint)
+            decoded.append({ index: subattrs })
+        return decoded
+
     def _decode_nest_type_value(self, attr, attr_spec):
         decoded = {}
         value = attr
@@ -689,6 +704,8 @@ class YnlFamily(SpecFamily):
             elif attr_spec["type"] == 'indexed-array' and 'sub-type' in attr_spec:
                 if attr_spec["sub-type"] == 'nest':
                     decoded = self._decode_array_nest(attr, attr_spec)
+                else:
+                    decoded = self._decode_index_array(attr, attr_spec)
             elif attr_spec["type"] == 'bitfield32':
                 value, selector = struct.unpack("II", attr.raw)
                 if 'enum' in attr_spec:
-- 
2.43.0

Re: [PATCH net-next 1/2] ynl: rename array-nest to indexed-array

[Jakub Kicinski]

On Tue, 26 Mar 2024 14:37:27 +0800 Hangbin Liu wrote:
> diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
> index 5fa7957f6e0f..7239e673a28a 100644
> --- a/tools/net/ynl/lib/ynl.py
> +++ b/tools/net/ynl/lib/ynl.py
> @@ -686,8 +686,9 @@ class YnlFamily(SpecFamily):
>                  decoded = attr.as_scalar(attr_spec['type'], attr_spec.byte_order)
>                  if 'enum' in attr_spec:
>                      decoded = self._decode_enum(decoded, attr_spec)
> -            elif attr_spec["type"] == 'array-nest':
> -                decoded = self._decode_array_nest(attr, attr_spec)
> +            elif attr_spec["type"] == 'indexed-array' and 'sub-type' in attr_spec:
> +                if attr_spec["sub-type"] == 'nest':
> +                    decoded = self._decode_array_nest(attr, attr_spec)

We need to make sure somehow cleanly that we treat unknown subtype the
same we would treat unknown type. In this elif ladder we have:

            else:
                if not self.process_unknown:
                    raise Exception(f'Unknown {attr_spec["type"]} with name {attr_spec["name"]}')

So we should raise an exception if sub-type != nest.

>              elif attr_spec["type"] == 'bitfield32':
>                  value, selector = struct.unpack("II", attr.raw)
>                  if 'enum' in attr_spec:
> diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
> index 6b7eb2d2aaf1..8d5ec5449648 100755
> --- a/tools/net/ynl/ynl-gen-c.py
> +++ b/tools/net/ynl/ynl-gen-c.py
> @@ -838,8 +838,9 @@ class AttrSet(SpecAttrSet):
>              t = TypeBitfield32(self.family, self, elem, value)
>          elif elem['type'] == 'nest':
>              t = TypeNest(self.family, self, elem, value)
> -        elif elem['type'] == 'array-nest':
> -            t = TypeArrayNest(self.family, self, elem, value)
> +        elif elem['type'] == 'indexed-array' and 'sub-type' in elem:
> +            if elem["sub-type"] == 'nest':
> +                t = TypeArrayNest(self.family, self, elem, value)

same here

>          elif elem['type'] == 'nest-type-value':
>              t = TypeNestTypeValue(self.family, self, elem, value)
>          else:
> @@ -1052,8 +1053,9 @@ class Family(SpecFamily):
>                      if nested in self.root_sets:
>                          raise Exception("Inheriting members to a space used as root not supported")
>                      inherit.update(set(spec['type-value']))
> -                elif spec['type'] == 'array-nest':
> -                    inherit.add('idx')
> +                elif spec['type'] == 'indexed-array' and 'sub-type' in spec:
> +                    if spec["sub-type"] == 'nest':
> +                        inherit.add('idx')

Here you don't have to match on sub-type, all indexed-arrays will have
an idx (index) member.

>                  self.pure_nested_structs[nested].set_inherited(inherit)
>  
>          for root_set, rs_members in self.root_sets.items():
> @@ -1616,9 +1618,10 @@ def _multi_parse(ri, struct, init_lines, local_vars):
>      multi_attrs = set()
>      needs_parg = False
>      for arg, aspec in struct.member_list():
> -        if aspec['type'] == 'array-nest':
> -            local_vars.append(f'const struct nlattr *attr_{aspec.c_name};')
> -            array_nests.add(arg)
> +        if aspec['type'] == 'indexed-array' and 'sub-type' in aspec:
> +            if aspec["sub-type"] == 'nest':
> +                local_vars.append(f'const struct nlattr *attr_{aspec.c_name};')
> +                array_nests.add(arg)

Please also update the info about nested-array in
Documentation/userspace-api/netlink/genetlink-legacy.rst
-- 
pw-bot: cr

Re: [PATCH net-next 2/2] ynl: support un-nest sub-type for indexed-array

[Hangbin Liu]

Hi Jakub,
On Tue, Mar 26, 2024 at 02:37:28PM +0800, Hangbin Liu wrote:
> Support un-nest sub-type for indexed-array. Since all the attr types are
> same for un-nest sub-ype, the index number is used as attr name.
> The result would look like:
> 
>  # ip link add bond0 type bond mode 1 \
>    arp_ip_target 192.168.1.1,192.168.1.2 ns_ip6_target 2001::1,2001::2
>  # ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/rt_link.yaml \
>    --do getlink --json '{"ifname": "bond0"}' --output-json | jq '.linkinfo'
> 
>     "arp-ip-target": [
>       {
>         "1": "192.168.1.1"
>       },
>       {
>         "2": "192.168.1.2"
>       }
>     ],

For index array, do you think if we need to add the index in the result
like upper example? Or we just omit the index and show it like:

    "arp-ip-target": [
      "192.168.1.1",
      "192.168.1.2"
    ],
    "ns-ip6-target": [
      "2001::1",
      "2001::2"
    ],

Thanks
Hangbin

Re: [PATCH net-next 1/2] ynl: rename array-nest to indexed-array

[Hangbin Liu]

On Tue, Mar 26, 2024 at 08:46:10PM -0700, Jakub Kicinski wrote:
> On Tue, 26 Mar 2024 14:37:27 +0800 Hangbin Liu wrote:
> > diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
> > index 5fa7957f6e0f..7239e673a28a 100644
> > --- a/tools/net/ynl/lib/ynl.py
> > +++ b/tools/net/ynl/lib/ynl.py
> > @@ -686,8 +686,9 @@ class YnlFamily(SpecFamily):
> >                  decoded = attr.as_scalar(attr_spec['type'], attr_spec.byte_order)
> >                  if 'enum' in attr_spec:
> >                      decoded = self._decode_enum(decoded, attr_spec)
> > -            elif attr_spec["type"] == 'array-nest':
> > -                decoded = self._decode_array_nest(attr, attr_spec)
> > +            elif attr_spec["type"] == 'indexed-array' and 'sub-type' in attr_spec:
> > +                if attr_spec["sub-type"] == 'nest':
> > +                    decoded = self._decode_array_nest(attr, attr_spec)
> 
> We need to make sure somehow cleanly that we treat unknown subtype the
> same we would treat unknown type. In this elif ladder we have:
> 
>             else:
>                 if not self.process_unknown:
>                     raise Exception(f'Unknown {attr_spec["type"]} with name {attr_spec["name"]}')
> 
> So we should raise an exception if sub-type != nest.

I agree we need raise exception when only support nest sub-type. But
what about after adding other sub-types in patch 2/2. e.g.

	if attr_spec["sub-type"] == 'nest':
		decoded = self._decode_array_nest(attr, attr_spec)
	else:
		decoded = self._decode_index_array(attr, attr_spec)

Should we remove the exception in patch 2?

Thanks
Hangbin

Re: [PATCH net-next 1/2] ynl: rename array-nest to indexed-array

[Jakub Kicinski]

On Thu, 28 Mar 2024 15:50:35 +0800 Hangbin Liu wrote:
> I agree we need raise exception when only support nest sub-type. But
> what about after adding other sub-types in patch 2/2. e.g.
> 
> 	if attr_spec["sub-type"] == 'nest':
> 		decoded = self._decode_array_nest(attr, attr_spec)
> 	else:
> 		decoded = self._decode_index_array(attr, attr_spec)
> 
> Should we remove the exception in patch 2?

Looking a bit closer you should probably have:

-            elif attr_spec["type"] == 'array-nest':
-                decoded = self._decode_array_nest(attr, attr_spec)
+            elif attr_spec["type"] == 'indexed-array':
+                decoded = self._decode_array_attr(attr, attr_spec)
             elif attr_spec["type"] == 'bitfield32':

and do the sub-type handling inside (now renamed) _decode_array_attr()
Throw the exception there appropriately.

Re: [PATCH net-next 2/2] ynl: support un-nest sub-type for indexed-array

[Jakub Kicinski]

On Thu, 28 Mar 2024 15:41:53 +0800 Hangbin Liu wrote:
> >  # ip link add bond0 type bond mode 1 \
> >    arp_ip_target 192.168.1.1,192.168.1.2 ns_ip6_target 2001::1,2001::2
> >  # ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/rt_link.yaml \
> >    --do getlink --json '{"ifname": "bond0"}' --output-json | jq '.linkinfo'
> > 
> >     "arp-ip-target": [
> >       {
> >         "1": "192.168.1.1"
> >       },
> >       {
> >         "2": "192.168.1.2"
> >       }
> >     ],  
> 
> For index array, do you think if we need to add the index in the result
> like upper example? Or we just omit the index and show it like:

Yes, the index in some funny dumps can actually be non-contiguous.
You should use the value from the attr, like the nest does.