[PATCH net-next 0/3] tools: ynl-gen: support sub-types for binary attributes

[PATCH net-next 0/3] tools: ynl-gen: support sub-types for binary attributes

[Jakub Kicinski]

Binary attributes have sub-type annotations which either indicate
that the binary object should be interpreted as a raw / C array of
a simple type (e.g. u32), or that it's a struct.

Use this information in the C codegen instead of outputting void *
for all binary attrs. It doesn't make a huge difference in the genl
families, but in classic Netlink there is a lot more structs.

Jakub Kicinski (3):
  tools: ynl-gen: support sub-type for binary attributes
  tools: ynl-gen: auto-indent else
  tools: ynl-gen: support struct for binary attributes

 tools/net/ynl/pyynl/ynl_gen_c.py | 66 +++++++++++++++++++++++++++-----
 1 file changed, 56 insertions(+), 10 deletions(-)

-- 
2.49.0

[PATCH net-next 1/3] tools: ynl-gen: support sub-type for binary attributes

[Jakub Kicinski]

Sub-type annotation on binary attributes may indicate that the attribute
carries an array of simple types (also referred to as "C array" in docs).
Support rendering them as such in the C user code. For example for u32,
instead of:

  struct {
    u32 arr;
  } _len;

  void *arr;

render:

  struct {
    u32 arr;
  } _count;

  __u32 *arr;

Note that count is the number of elements while len was the length in bytes.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/pyynl/ynl_gen_c.py | 56 ++++++++++++++++++++++++++------
 1 file changed, 46 insertions(+), 10 deletions(-)

diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
index 4a2d3cc07e14..2ae5eaf2611d 100755
--- a/tools/net/ynl/pyynl/ynl_gen_c.py
+++ b/tools/net/ynl/pyynl/ynl_gen_c.py
@@ -163,7 +163,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         return False
 
     def _free_lines(self, ri, var, ref):
-        if self.is_multi_val() or self.presence_type() == 'len':
+        if self.is_multi_val() or self.presence_type() in {'count', 'len'}:
             return [f'free({var}->{ref}{self.c_name});']
         return []
 
@@ -516,13 +516,21 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
 
 class TypeBinary(Type):
     def arg_member(self, ri):
+        if self.get('sub-type') and self.get('sub-type') in scalars:
+            return [f'__{self.get("sub-type")} *{self.c_name}', 'size_t count']
         return [f"const void *{self.c_name}", 'size_t len']
 
     def presence_type(self):
-        return 'len'
+        if self.get('sub-type') and self.get('sub-type') in scalars:
+            return 'count'
+        else:
+            return 'len'
 
     def struct_member(self, ri):
-        ri.cw.p(f"void *{self.c_name};")
+        if self.get('sub-type') and self.get('sub-type') in scalars:
+            ri.cw.p(f'__{self.get("sub-type")} *{self.c_name};')
+        else:
+            ri.cw.p(f"void *{self.c_name};")
 
     def _attr_typol(self):
         return f'.type = YNL_PT_BINARY,'
@@ -549,18 +557,46 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         return mem
 
     def attr_put(self, ri, var):
-        self._attr_put_line(ri, var, f"ynl_attr_put(nlh, {self.enum_name}, " +
-                            f"{var}->{self.c_name}, {var}->_len.{self.c_name})")
+        if self.get('sub-type') and self.get('sub-type') in scalars:
+            presence = self.presence_type()
+            ri.cw.block_start(line=f"if ({var}->_{presence}.{self.c_name})")
+            ri.cw.p(f"i = {var}->_{presence}.{self.c_name} * sizeof(__{self.get('sub-type')});")
+            ri.cw.p(f"ynl_attr_put(nlh, {self.enum_name}, " +
+                    f"{var}->{self.c_name}, i);")
+            ri.cw.block_end()
+            pass
+        else:
+            self._attr_put_line(ri, var, f"ynl_attr_put(nlh, {self.enum_name}, "
+                                f"{var}->{self.c_name}, {var}->_len.{self.c_name})")
 
     def _attr_get(self, ri, var):
-        len_mem = var + '->_len.' + self.c_name
-        return [f"{len_mem} = len;",
-                f"{var}->{self.c_name} = malloc(len);",
-                f"memcpy({var}->{self.c_name}, ynl_attr_data(attr), len);"], \
+        get_lines = []
+        len_mem = var + '->_' + self.presence_type() + '.' + self.c_name
+
+        if self.get('sub-type') and self.get('sub-type') in scalars:
+            get_lines = [
+                f"{len_mem} = len / sizeof(__{self.get('sub-type')});",
+                f"len = {len_mem} * sizeof(__{self.get('sub-type')});",
+            ]
+        else:
+            get_lines += [f"{len_mem} = len;"]
+
+        get_lines += [
+            f"{var}->{self.c_name} = malloc(len);",
+            f"memcpy({var}->{self.c_name}, ynl_attr_data(attr), len);"
+        ]
+
+        return get_lines, \
                ['len = ynl_attr_data_len(attr);'], \
                ['unsigned int len;']
 
     def _setter_lines(self, ri, member, presence):
+        if self.get('sub-type') and self.get('sub-type') in scalars:
+            return [f"{presence} = count;",
+                    f"count *= sizeof(__{self.get('sub-type')});",
+                    f"{member} = malloc(count);",
+                    f'memcpy({member}, {self.c_name}, count);']
+
         return [f"{presence} = len;",
                 f"{member} = malloc({presence});",
                 f'memcpy({member}, {self.c_name}, {presence});']
@@ -672,7 +708,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         lines = []
         if self.attr['type'] in scalars:
             lines += [f"free({var}->{ref}{self.c_name});"]
-        elif self.attr['type'] == 'binary' and 'struct' in self.attr:
+        elif self.attr['type'] == 'binary':
             lines += [f"free({var}->{ref}{self.c_name});"]
         elif self.attr['type'] == 'string':
             lines += [
-- 
2.49.0

[PATCH net-next 2/3] tools: ynl-gen: auto-indent else

[Jakub Kicinski]

We auto-indent if statements (increase the indent of the subsequent
line by 1), do the same thing for else branches without a block.
There hasn't been any else branches before but we're about to add one.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/pyynl/ynl_gen_c.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
index 2ae5eaf2611d..df429494461d 100755
--- a/tools/net/ynl/pyynl/ynl_gen_c.py
+++ b/tools/net/ynl/pyynl/ynl_gen_c.py
@@ -1457,6 +1457,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         if self._silent_block:
             ind += 1
         self._silent_block = line.endswith(')') and CodeWriter._is_cond(line)
+        self._silent_block |= line.strip() == 'else'
         if line[0] == '#':
             ind = 0
         if add_ind:
-- 
2.49.0

[PATCH net-next 3/3] tools: ynl-gen: support struct for binary attributes

[Jakub Kicinski]

Support using a struct pointer for binary attrs. Len field is maintained
because the structs may grow with newer kernel versions. Or, which matters
more, be shorter if the binary is built against newer uAPI than kernel
against which it's executed. Since we are storing a pointer to a struct
type - always allocate at least the amount of memory needed by the struct
per current uAPI headers (unused mem is zeroed). Technically users should
check the length field but per modern ASAN checks storing a short object
under a pointer seems like a bad idea.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/pyynl/ynl_gen_c.py | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
index df429494461d..9c6340a16185 100755
--- a/tools/net/ynl/pyynl/ynl_gen_c.py
+++ b/tools/net/ynl/pyynl/ynl_gen_c.py
@@ -529,6 +529,8 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
     def struct_member(self, ri):
         if self.get('sub-type') and self.get('sub-type') in scalars:
             ri.cw.p(f'__{self.get("sub-type")} *{self.c_name};')
+        elif self.get('struct'):
+            ri.cw.p(f'struct {c_lower(self.get("struct"))} *{self.c_name};')
         else:
             ri.cw.p(f"void *{self.c_name};")
 
@@ -581,6 +583,13 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         else:
             get_lines += [f"{len_mem} = len;"]
 
+        if self.get('struct'):
+            struct_sz = 'sizeof(struct ' + c_lower(self.get("struct")) + ')'
+            get_lines += [
+                f"if (len < {struct_sz})",
+                f"{var}->{self.c_name} = calloc(1, {struct_sz});",
+                "else",
+            ]
         get_lines += [
             f"{var}->{self.c_name} = malloc(len);",
             f"memcpy({var}->{self.c_name}, ynl_attr_data(attr), len);"
-- 
2.49.0

Re: [PATCH net-next 1/3] tools: ynl-gen: support sub-type for binary attributes

[Donald Hunter]

On Thu, 8 May 2025 at 03:28, Jakub Kicinski <kuba@kernel.org> wrote:
>
> @@ -516,13 +516,21 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
>
>  class TypeBinary(Type):
>      def arg_member(self, ri):
> +        if self.get('sub-type') and self.get('sub-type') in scalars:

This check is repeated a lot, so maybe it would benefit from a
_has_scalar_sub_type() helper?

> +            return [f'__{self.get("sub-type")} *{self.c_name}', 'size_t count']
>          return [f"const void *{self.c_name}", 'size_t len']
>
>      def presence_type(self):
> -        return 'len'
> +        if self.get('sub-type') and self.get('sub-type') in scalars:
> +            return 'count'
> +        else:
> +            return 'len'
>
>      def struct_member(self, ri):
> -        ri.cw.p(f"void *{self.c_name};")
> +        if self.get('sub-type') and self.get('sub-type') in scalars:
> +            ri.cw.p(f'__{self.get("sub-type")} *{self.c_name};')
> +        else:
> +            ri.cw.p(f"void *{self.c_name};")
>
>      def _attr_typol(self):
>          return f'.type = YNL_PT_BINARY,'
> @@ -549,18 +557,46 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
>          return mem
>
>      def attr_put(self, ri, var):
> -        self._attr_put_line(ri, var, f"ynl_attr_put(nlh, {self.enum_name}, " +
> -                            f"{var}->{self.c_name}, {var}->_len.{self.c_name})")
> +        if self.get('sub-type') and self.get('sub-type') in scalars:
> +            presence = self.presence_type()
> +            ri.cw.block_start(line=f"if ({var}->_{presence}.{self.c_name})")
> +            ri.cw.p(f"i = {var}->_{presence}.{self.c_name} * sizeof(__{self.get('sub-type')});")
> +            ri.cw.p(f"ynl_attr_put(nlh, {self.enum_name}, " +
> +                    f"{var}->{self.c_name}, i);")
> +            ri.cw.block_end()
> +            pass
> +        else:
> +            self._attr_put_line(ri, var, f"ynl_attr_put(nlh, {self.enum_name}, "
> +                                f"{var}->{self.c_name}, {var}->_len.{self.c_name})")
>
>      def _attr_get(self, ri, var):
> -        len_mem = var + '->_len.' + self.c_name
> -        return [f"{len_mem} = len;",
> -                f"{var}->{self.c_name} = malloc(len);",
> -                f"memcpy({var}->{self.c_name}, ynl_attr_data(attr), len);"], \
> +        get_lines = []
> +        len_mem = var + '->_' + self.presence_type() + '.' + self.c_name
> +
> +        if self.get('sub-type') and self.get('sub-type') in scalars:
> +            get_lines = [
> +                f"{len_mem} = len / sizeof(__{self.get('sub-type')});",
> +                f"len = {len_mem} * sizeof(__{self.get('sub-type')});",
> +            ]
> +        else:
> +            get_lines += [f"{len_mem} = len;"]
> +
> +        get_lines += [
> +            f"{var}->{self.c_name} = malloc(len);",
> +            f"memcpy({var}->{self.c_name}, ynl_attr_data(attr), len);"
> +        ]
> +
> +        return get_lines, \
>                 ['len = ynl_attr_data_len(attr);'], \
>                 ['unsigned int len;']
>
>      def _setter_lines(self, ri, member, presence):
> +        if self.get('sub-type') and self.get('sub-type') in scalars:
> +            return [f"{presence} = count;",
> +                    f"count *= sizeof(__{self.get('sub-type')});",
> +                    f"{member} = malloc(count);",
> +                    f'memcpy({member}, {self.c_name}, count);']
> +
>          return [f"{presence} = len;",
>                  f"{member} = malloc({presence});",
>                  f'memcpy({member}, {self.c_name}, {presence});']
> @@ -672,7 +708,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
>          lines = []
>          if self.attr['type'] in scalars:
>              lines += [f"free({var}->{ref}{self.c_name});"]
> -        elif self.attr['type'] == 'binary' and 'struct' in self.attr:
> +        elif self.attr['type'] == 'binary':
>              lines += [f"free({var}->{ref}{self.c_name});"]
>          elif self.attr['type'] == 'string':
>              lines += [
> --
> 2.49.0
>

Re: [PATCH net-next 2/3] tools: ynl-gen: auto-indent else

[Donald Hunter]

On Thu, 8 May 2025 at 03:28, Jakub Kicinski <kuba@kernel.org> wrote:
>
> We auto-indent if statements (increase the indent of the subsequent
> line by 1), do the same thing for else branches without a block.
> There hasn't been any else branches before but we're about to add one.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

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

Re: [PATCH net-next 3/3] tools: ynl-gen: support struct for binary attributes

[Donald Hunter]

On Thu, 8 May 2025 at 03:28, Jakub Kicinski <kuba@kernel.org> wrote:
>
> Support using a struct pointer for binary attrs. Len field is maintained
> because the structs may grow with newer kernel versions. Or, which matters
> more, be shorter if the binary is built against newer uAPI than kernel
> against which it's executed. Since we are storing a pointer to a struct
> type - always allocate at least the amount of memory needed by the struct
> per current uAPI headers (unused mem is zeroed). Technically users should
> check the length field but per modern ASAN checks storing a short object
> under a pointer seems like a bad idea.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

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

Re: [PATCH net-next 1/3] tools: ynl-gen: support sub-type for binary attributes

[Jakub Kicinski]

On Thu, 8 May 2025 12:33:10 +0100 Donald Hunter wrote:
> >  class TypeBinary(Type):
> >      def arg_member(self, ri):
> > +        if self.get('sub-type') and self.get('sub-type') in scalars:  
> 
> This check is repeated a lot, so maybe it would benefit from a
> _has_scalar_sub_type() helper?

I've gotten used to repeating the conditions in TypeArrayNest 
and TypeMultiAttr...

Looking at it now, since I'm using .get() the part of the condition 
is unnecessary. How about just:

	if self.get('sub-type') in scalars:

? None is not a scalar after all

Re: [PATCH net-next 1/3] tools: ynl-gen: support sub-type for binary attributes

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> On Thu, 8 May 2025 12:33:10 +0100 Donald Hunter wrote:
>> >  class TypeBinary(Type):
>> >      def arg_member(self, ri):
>> > +        if self.get('sub-type') and self.get('sub-type') in scalars:  
>> 
>> This check is repeated a lot, so maybe it would benefit from a
>> _has_scalar_sub_type() helper?
>
> I've gotten used to repeating the conditions in TypeArrayNest 
> and TypeMultiAttr...
>
> Looking at it now, since I'm using .get() the part of the condition 
> is unnecessary. How about just:
>
> 	if self.get('sub-type') in scalars:
>
> ? None is not a scalar after all

That's definitely an improvement.

I realise that it's not so much the repeated conditions that concerns me
as the need to check in so many places. Perhaps there is a refactoring
that specialises TypeBinary?