[PATCH net-next 0/3] tools: ynl-gen: split presence metadata

[PATCH net-next 0/3] tools: ynl-gen: split presence metadata

[Jakub Kicinski]

The presence metadata indicates whether given attribute was/should be
added to the Netlink message. We have 3 types of such metadata:
 - bit presence for simple values like integers,
 - len presence for variable size attrs, like binary and strings,
 - count for arrays.

Previously this information was spread around with first two types
living in a dedicated sub-struct called _present. The counts resided
directly in the main struct with an n_ prefix.

Reshuffle these an uniformly store them in dedicated sub-structs.
The immediate motivation is that current scheme causes name collisions
for TC.

Jakub Kicinski (3):
  tools: ynl-gen: rename basic presence from 'bit' to 'present'
  tools: ynl-gen: split presence metadata
  tools: ynl-gen: move the count into a presence struct too

 tools/net/ynl/samples/devlink.c  |  7 +--
 tools/net/ynl/samples/rt-addr.c  |  4 +-
 tools/net/ynl/samples/rt-route.c |  4 +-
 tools/net/ynl/pyynl/ynl_gen_c.py | 80 +++++++++++++++-----------------
 4 files changed, 45 insertions(+), 50 deletions(-)

-- 
2.49.0

[PATCH net-next 1/3] tools: ynl-gen: rename basic presence from 'bit' to 'present'

[Jakub Kicinski]

Internal change to the code gen. Rename how we indicate a type
has a single bit presence from using a 'bit' string to 'present'.
This is a noop in terms of generated code but will make next
breaking change easier.

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

diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
index 09b87c9a6908..f93e6e79312a 100755
--- a/tools/net/ynl/pyynl/ynl_gen_c.py
+++ b/tools/net/ynl/pyynl/ynl_gen_c.py
@@ -142,13 +142,13 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         return self.is_recursive() and not ri.op
 
     def presence_type(self):
-        return 'bit'
+        return 'present'
 
     def presence_member(self, space, type_filter):
         if self.presence_type() != type_filter:
             return
 
-        if self.presence_type() == 'bit':
+        if self.presence_type() == 'present':
             pfx = '__' if space == 'user' else ''
             return f"{pfx}u32 {self.c_name}:1;"
 
@@ -217,7 +217,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         cw.p(f'[{self.enum_name}] = {"{"} .name = "{self.name}", {typol}{"}"},')
 
     def _attr_put_line(self, ri, var, line):
-        if self.presence_type() == 'bit':
+        if self.presence_type() == 'present':
             ri.cw.p(f"if ({var}->_present.{self.c_name})")
         elif self.presence_type() == 'len':
             ri.cw.p(f"if ({var}->_present.{self.c_name}_len)")
@@ -250,7 +250,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         if not self.is_multi_val():
             ri.cw.p("if (ynl_attr_validate(yarg, attr))")
             ri.cw.p("return YNL_PARSE_CB_ERROR;")
-            if self.presence_type() == 'bit':
+            if self.presence_type() == 'present':
                 ri.cw.p(f"{var}->_present.{self.c_name} = 1;")
 
         if init_lines:
@@ -281,7 +281,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
             presence = f"{var}->{'.'.join(ref[:i] + [''])}_present.{ref[i]}"
             # Every layer below last is a nest, so we know it uses bit presence
             # last layer is "self" and may be a complex type
-            if i == len(ref) - 1 and self.presence_type() != 'bit':
+            if i == len(ref) - 1 and self.presence_type() != 'present':
                 continue
             code.append(presence + ' = 1;')
         ref_path = '.'.join(ref[:-1])
@@ -2183,7 +2183,7 @@ _C_KW = {
 
     meta_started = False
     for _, attr in struct.member_list():
-        for type_filter in ['len', 'bit']:
+        for type_filter in ['len', 'present']:
             line = attr.presence_member(ri.ku_space, type_filter)
             if line:
                 if not meta_started:
-- 
2.49.0

[PATCH net-next 2/3] tools: ynl-gen: split presence metadata

[Jakub Kicinski]

Each YNL struct contains the data and a sub-struct indicating which
fields are valid. Something like:

  struct family_op_req {
      struct {
            u32 a:1;
            u32 b:1;
	    u32 bin_len;
      } _present;

      u32 a;
      u64 b;
      const unsigned char *bin;
  };

Note that the bin object 'bin' has a length stored, and that length
has a _len suffix added to the field name. This breaks if there
is a explicit field called bin_len, which is the case for some
TC actions. Move the length fields out of the _present struct,
create a new struct called _len:

  struct family_op_req {
      struct {
            u32 a:1;
            u32 b:1;
      } _present;
      struct {
	    u32 bin;
      } _len;

      u32 a;
      u64 b;
      const unsigned char *bin;
  };

This should prevent name collisions and help with the packing
of the struct.

Unfortunately this is a breaking change, but hopefully the migration
isn't too painful.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/samples/devlink.c  |  2 +-
 tools/net/ynl/samples/rt-addr.c  |  4 +--
 tools/net/ynl/samples/rt-route.c |  4 +--
 tools/net/ynl/pyynl/ynl_gen_c.py | 46 ++++++++++++++++----------------
 4 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/tools/net/ynl/samples/devlink.c b/tools/net/ynl/samples/devlink.c
index d2611d7ebab4..3d32a6335044 100644
--- a/tools/net/ynl/samples/devlink.c
+++ b/tools/net/ynl/samples/devlink.c
@@ -34,7 +34,7 @@ int main(int argc, char **argv)
 		if (!info_rsp)
 			goto err_free_devs;
 
-		if (info_rsp->_present.info_driver_name_len)
+		if (info_rsp->_len.info_driver_name)
 			printf("    driver: %s\n", info_rsp->info_driver_name);
 		if (info_rsp->n_info_version_running)
 			printf("    running fw:\n");
diff --git a/tools/net/ynl/samples/rt-addr.c b/tools/net/ynl/samples/rt-addr.c
index 0f4851b4ec57..2edde5c36b18 100644
--- a/tools/net/ynl/samples/rt-addr.c
+++ b/tools/net/ynl/samples/rt-addr.c
@@ -20,7 +20,7 @@ static void rt_addr_print(struct rt_addr_getaddr_rsp *a)
 	if (name)
 		printf("%16s: ", name);
 
-	switch (a->_present.address_len) {
+	switch (a->_len.address) {
 	case 4:
 		addr = inet_ntop(AF_INET, a->address,
 				 addr_str, sizeof(addr_str));
@@ -36,7 +36,7 @@ static void rt_addr_print(struct rt_addr_getaddr_rsp *a)
 	if (addr)
 		printf("%s", addr);
 	else
-		printf("[%d]", a->_present.address_len);
+		printf("[%d]", a->_len.address);
 
 	printf("\n");
 }
diff --git a/tools/net/ynl/samples/rt-route.c b/tools/net/ynl/samples/rt-route.c
index 9d9c868f8873..7427104a96df 100644
--- a/tools/net/ynl/samples/rt-route.c
+++ b/tools/net/ynl/samples/rt-route.c
@@ -26,13 +26,13 @@ static void rt_route_print(struct rt_route_getroute_rsp *r)
 			printf("oif: %-16s ", name);
 	}
 
-	if (r->_present.dst_len) {
+	if (r->_len.dst) {
 		route = inet_ntop(r->_hdr.rtm_family, r->dst,
 				  route_str, sizeof(route_str));
 		printf("dst: %s/%d", route, r->_hdr.rtm_dst_len);
 	}
 
-	if (r->_present.gateway_len) {
+	if (r->_len.gateway) {
 		route = inet_ntop(r->_hdr.rtm_family, r->gateway,
 				  route_str, sizeof(route_str));
 		printf("gateway: %s ", route);
diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
index f93e6e79312a..800710fe96c9 100755
--- a/tools/net/ynl/pyynl/ynl_gen_c.py
+++ b/tools/net/ynl/pyynl/ynl_gen_c.py
@@ -154,7 +154,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
 
         if self.presence_type() == 'len':
             pfx = '__' if space == 'user' else ''
-            return f"{pfx}u32 {self.c_name}_len;"
+            return f"{pfx}u32 {self.c_name};"
 
     def _complex_member_type(self, ri):
         return None
@@ -217,10 +217,9 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         cw.p(f'[{self.enum_name}] = {"{"} .name = "{self.name}", {typol}{"}"},')
 
     def _attr_put_line(self, ri, var, line):
-        if self.presence_type() == 'present':
-            ri.cw.p(f"if ({var}->_present.{self.c_name})")
-        elif self.presence_type() == 'len':
-            ri.cw.p(f"if ({var}->_present.{self.c_name}_len)")
+        presence = self.presence_type()
+        if presence in {'present', 'len'}:
+            ri.cw.p(f"if ({var}->_{presence}.{self.c_name})")
         ri.cw.p(f"{line};")
 
     def _attr_put_simple(self, ri, var, put_type):
@@ -282,6 +281,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
             # Every layer below last is a nest, so we know it uses bit presence
             # last layer is "self" and may be a complex type
             if i == len(ref) - 1 and self.presence_type() != 'present':
+                presence = f"{var}->{'.'.join(ref[:i] + [''])}_{self.presence_type()}.{ref[i]}"
                 continue
             code.append(presence + ' = 1;')
         ref_path = '.'.join(ref[:-1])
@@ -496,7 +496,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         self._attr_put_simple(ri, var, 'str')
 
     def _attr_get(self, ri, var):
-        len_mem = var + '->_present.' + self.c_name + '_len'
+        len_mem = var + '->_len.' + self.c_name
         return [f"{len_mem} = len;",
                 f"{var}->{self.c_name} = malloc(len + 1);",
                 f"memcpy({var}->{self.c_name}, ynl_attr_get_str(attr), len);",
@@ -505,10 +505,10 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
                ['unsigned int len;']
 
     def _setter_lines(self, ri, member, presence):
-        return [f"{presence}_len = strlen({self.c_name});",
-                f"{member} = malloc({presence}_len + 1);",
-                f'memcpy({member}, {self.c_name}, {presence}_len);',
-                f'{member}[{presence}_len] = 0;']
+        return [f"{presence} = strlen({self.c_name});",
+                f"{member} = malloc({presence} + 1);",
+                f'memcpy({member}, {self.c_name}, {presence});',
+                f'{member}[{presence}] = 0;']
 
 
 class TypeBinary(Type):
@@ -547,10 +547,10 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
 
     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}->_present.{self.c_name}_len)")
+                            f"{var}->{self.c_name}, {var}->_len.{self.c_name})")
 
     def _attr_get(self, ri, var):
-        len_mem = var + '->_present.' + self.c_name + '_len'
+        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);"], \
@@ -558,9 +558,9 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
                ['unsigned int len;']
 
     def _setter_lines(self, ri, member, presence):
-        return [f"{presence}_len = len;",
-                f"{member} = malloc({presence}_len);",
-                f'memcpy({member}, {self.c_name}, {presence}_len);']
+        return [f"{presence} = len;",
+                f"{member} = malloc({presence});",
+                f'memcpy({member}, {self.c_name}, {presence});']
 
 
 class TypeBitfield32(Type):
@@ -716,7 +716,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
 
     def _setter_lines(self, ri, member, presence):
         # For multi-attr we have a count, not presence, hack up the presence
-        presence = presence[:-(len('_present.') + len(self.c_name))] + "n_" + self.c_name
+        presence = presence[:-(len('_count.') + len(self.c_name))] + "n_" + self.c_name
         return [f"{member} = {self.c_name};",
                 f"{presence} = n_{self.c_name};"]
 
@@ -779,7 +779,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
 
     def _setter_lines(self, ri, member, presence):
         # For multi-attr we have a count, not presence, hack up the presence
-        presence = presence[:-(len('_present.') + len(self.c_name))] + "n_" + self.c_name
+        presence = presence[:-(len('_count.') + len(self.c_name))] + "n_" + self.c_name
         return [f"{member} = {self.c_name};",
                 f"{presence} = n_{self.c_name};"]
 
@@ -2181,18 +2181,18 @@ _C_KW = {
         ri.cw.p(ri.fixed_hdr + ' _hdr;')
         ri.cw.nl()
 
-    meta_started = False
-    for _, attr in struct.member_list():
-        for type_filter in ['len', 'present']:
+    for type_filter in ['present', 'len']:
+        meta_started = False
+        for _, attr in struct.member_list():
             line = attr.presence_member(ri.ku_space, type_filter)
             if line:
                 if not meta_started:
                     ri.cw.block_start(line=f"struct")
                     meta_started = True
                 ri.cw.p(line)
-    if meta_started:
-        ri.cw.block_end(line='_present;')
-        ri.cw.nl()
+        if meta_started:
+            ri.cw.block_end(line=f'_{type_filter};')
+    ri.cw.nl()
 
     for arg in struct.inherited:
         ri.cw.p(f"__u32 {arg};")
-- 
2.49.0

[PATCH net-next 3/3] tools: ynl-gen: move the count into a presence struct too

[Jakub Kicinski]

While we reshuffle the presence members, move the counts as well.
Previously array count members would have been place directly in
the struct, so:

  struct family_op_req {
      struct {
            u32 a:1;
            u32 b:1;
      } _present;
      struct {
            u32 bin;
      } _len;

      u32 a;
      u64 b;
      const unsigned char *bin;
      u32 n_multi;                 << count
      u32 *multi;                  << objects
  };

Since len has been moved to its own presence struct move the count
as well:

  struct family_op_req {
      struct {
            u32 a:1;
            u32 b:1;
      } _present;
      struct {
            u32 bin;
      } _len;
      struct {
            u32 multi;             << count
      } _count;

      u32 a;
      u64 b;
      const unsigned char *bin;
      u32 *multi;                  << objects
  };

This improves the consistency and allows us to remove some hacks
in the codegen. Unlike for len there is no known name collision
with the existing scheme.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/net/ynl/samples/devlink.c  |  5 +++--
 tools/net/ynl/pyynl/ynl_gen_c.py | 32 +++++++++++++-------------------
 2 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/tools/net/ynl/samples/devlink.c b/tools/net/ynl/samples/devlink.c
index 3d32a6335044..ac9dfb01f280 100644
--- a/tools/net/ynl/samples/devlink.c
+++ b/tools/net/ynl/samples/devlink.c
@@ -22,6 +22,7 @@ int main(int argc, char **argv)
 	ynl_dump_foreach(devs, d) {
 		struct devlink_info_get_req *info_req;
 		struct devlink_info_get_rsp *info_rsp;
+		unsigned i;
 
 		printf("%s/%s:\n", d->bus_name, d->dev_name);
 
@@ -36,9 +37,9 @@ int main(int argc, char **argv)
 
 		if (info_rsp->_len.info_driver_name)
 			printf("    driver: %s\n", info_rsp->info_driver_name);
-		if (info_rsp->n_info_version_running)
+		if (info_rsp->_count.info_version_running)
 			printf("    running fw:\n");
-		for (unsigned i = 0; i < info_rsp->n_info_version_running; i++)
+		for (i = 0; i < info_rsp->_count.info_version_running; i++)
 			printf("        %s: %s\n",
 			       info_rsp->info_version_running[i].info_version_name,
 			       info_rsp->info_version_running[i].info_version_value);
diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
index 800710fe96c9..65c83818b2ed 100755
--- a/tools/net/ynl/pyynl/ynl_gen_c.py
+++ b/tools/net/ynl/pyynl/ynl_gen_c.py
@@ -152,7 +152,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
             pfx = '__' if space == 'user' else ''
             return f"{pfx}u32 {self.c_name}:1;"
 
-        if self.presence_type() == 'len':
+        if self.presence_type() in {'len', 'count'}:
             pfx = '__' if space == 'user' else ''
             return f"{pfx}u32 {self.c_name};"
 
@@ -185,8 +185,6 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
     def struct_member(self, ri):
         member = self._complex_member_type(ri)
         if member:
-            if self.is_multi_val():
-                ri.cw.p(f"unsigned int n_{self.c_name};")
             ptr = '*' if self.is_multi_val() else ''
             if self.is_recursive_for_op(ri):
                 ptr = '*'
@@ -673,13 +671,13 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
             lines += [f"free({var}->{ref}{self.c_name});"]
         elif self.attr['type'] == 'string':
             lines += [
-                f"for (i = 0; i < {var}->{ref}n_{self.c_name}; i++)",
+                f"for (i = 0; i < {var}->{ref}_count.{self.c_name}; i++)",
                 f"free({var}->{ref}{self.c_name}[i]);",
                 f"free({var}->{ref}{self.c_name});",
             ]
         elif 'type' not in self.attr or self.attr['type'] == 'nest':
             lines += [
-                f"for (i = 0; i < {var}->{ref}n_{self.c_name}; i++)",
+                f"for (i = 0; i < {var}->{ref}_count.{self.c_name}; i++)",
                 f'{self.nested_render_name}_free(&{var}->{ref}{self.c_name}[i]);',
                 f"free({var}->{ref}{self.c_name});",
             ]
@@ -699,24 +697,22 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
     def attr_put(self, ri, var):
         if self.attr['type'] in scalars:
             put_type = self.type
-            ri.cw.p(f"for (i = 0; i < {var}->n_{self.c_name}; i++)")
+            ri.cw.p(f"for (i = 0; i < {var}->_count.{self.c_name}; i++)")
             ri.cw.p(f"ynl_attr_put_{put_type}(nlh, {self.enum_name}, {var}->{self.c_name}[i]);")
         elif self.attr['type'] == 'binary' and 'struct' in self.attr:
-            ri.cw.p(f"for (i = 0; i < {var}->n_{self.c_name}; i++)")
+            ri.cw.p(f"for (i = 0; i < {var}->_count.{self.c_name}; i++)")
             ri.cw.p(f"ynl_attr_put(nlh, {self.enum_name}, &{var}->{self.c_name}[i], sizeof(struct {c_lower(self.attr['struct'])}));")
         elif self.attr['type'] == 'string':
-            ri.cw.p(f"for (i = 0; i < {var}->n_{self.c_name}; i++)")
+            ri.cw.p(f"for (i = 0; i < {var}->_count.{self.c_name}; i++)")
             ri.cw.p(f"ynl_attr_put_str(nlh, {self.enum_name}, {var}->{self.c_name}[i]->str);")
         elif 'type' not in self.attr or self.attr['type'] == 'nest':
-            ri.cw.p(f"for (i = 0; i < {var}->n_{self.c_name}; i++)")
+            ri.cw.p(f"for (i = 0; i < {var}->_count.{self.c_name}; i++)")
             self._attr_put_line(ri, var, f"{self.nested_render_name}_put(nlh, " +
                                 f"{self.enum_name}, &{var}->{self.c_name}[i])")
         else:
             raise Exception(f"Put of MultiAttr sub-type {self.attr['type']} not supported yet")
 
     def _setter_lines(self, ri, member, presence):
-        # For multi-attr we have a count, not presence, hack up the presence
-        presence = presence[:-(len('_count.') + len(self.c_name))] + "n_" + self.c_name
         return [f"{member} = {self.c_name};",
                 f"{presence} = n_{self.c_name};"]
 
@@ -759,7 +755,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
                      'ynl_attr_for_each_nested(attr2, attr) {',
                      '\tif (ynl_attr_validate(yarg, attr2))',
                      '\t\treturn YNL_PARSE_CB_ERROR;',
-                     f'\t{var}->n_{self.c_name}++;',
+                     f'\t{var}->_count.{self.c_name}++;',
                      '}']
         return get_lines, None, local_vars
 
@@ -767,19 +763,17 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         ri.cw.p(f'array = ynl_attr_nest_start(nlh, {self.enum_name});')
         if self.sub_type in scalars:
             put_type = self.sub_type
-            ri.cw.block_start(line=f'for (i = 0; i < {var}->n_{self.c_name}; i++)')
+            ri.cw.block_start(line=f'for (i = 0; i < {var}->_count.{self.c_name}; i++)')
             ri.cw.p(f"ynl_attr_put_{put_type}(nlh, i, {var}->{self.c_name}[i]);")
             ri.cw.block_end()
         elif self.sub_type == 'binary' and 'exact-len' in self.checks:
-            ri.cw.p(f'for (i = 0; i < {var}->n_{self.c_name}; i++)')
+            ri.cw.p(f'for (i = 0; i < {var}->_count.{self.c_name}; i++)')
             ri.cw.p(f"ynl_attr_put(nlh, i, {var}->{self.c_name}[i], {self.checks['exact-len']});")
         else:
             raise Exception(f"Put for ArrayNest sub-type {self.attr['sub-type']} not supported, yet")
         ri.cw.p('ynl_attr_nest_end(nlh, array);')
 
     def _setter_lines(self, ri, member, presence):
-        # For multi-attr we have a count, not presence, hack up the presence
-        presence = presence[:-(len('_count.') + len(self.c_name))] + "n_" + self.c_name
         return [f"{member} = {self.c_name};",
                 f"{presence} = n_{self.c_name};"]
 
@@ -1879,7 +1873,7 @@ _C_KW = {
 
         ri.cw.block_start(line=f"if (n_{aspec.c_name})")
         ri.cw.p(f"dst->{aspec.c_name} = calloc(n_{aspec.c_name}, sizeof(*dst->{aspec.c_name}));")
-        ri.cw.p(f"dst->n_{aspec.c_name} = n_{aspec.c_name};")
+        ri.cw.p(f"dst->_count.{aspec.c_name} = n_{aspec.c_name};")
         ri.cw.p('i = 0;')
         if 'nested-attributes' in aspec:
             ri.cw.p(f"parg.rsp_policy = &{aspec.nested_render_name}_nest;")
@@ -1904,7 +1898,7 @@ _C_KW = {
         aspec = struct[anest]
         ri.cw.block_start(line=f"if (n_{aspec.c_name})")
         ri.cw.p(f"dst->{aspec.c_name} = calloc(n_{aspec.c_name}, sizeof(*dst->{aspec.c_name}));")
-        ri.cw.p(f"dst->n_{aspec.c_name} = n_{aspec.c_name};")
+        ri.cw.p(f"dst->_count.{aspec.c_name} = n_{aspec.c_name};")
         ri.cw.p('i = 0;')
         if 'nested-attributes' in aspec:
             ri.cw.p(f"parg.rsp_policy = &{aspec.nested_render_name}_nest;")
@@ -2181,7 +2175,7 @@ _C_KW = {
         ri.cw.p(ri.fixed_hdr + ' _hdr;')
         ri.cw.nl()
 
-    for type_filter in ['present', 'len']:
+    for type_filter in ['present', 'len', 'count']:
         meta_started = False
         for _, attr in struct.member_list():
             line = attr.presence_member(ri.ku_space, type_filter)
-- 
2.49.0

Re: [PATCH net-next 1/3] tools: ynl-gen: rename basic presence from 'bit' to 'present'

[David Wei]

On 5/5/25 09:52, Jakub Kicinski wrote:
> Internal change to the code gen. Rename how we indicate a type
> has a single bit presence from using a 'bit' string to 'present'.
> This is a noop in terms of generated code but will make next
> breaking change easier.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Confirmed mechanical change.

Reviewed-by: David Wei <dw@davidwei.uk>

Re: [PATCH net-next 2/3] tools: ynl-gen: split presence metadata

[David Wei]

On 5/5/25 09:52, Jakub Kicinski wrote:
> Each YNL struct contains the data and a sub-struct indicating which
> fields are valid. Something like:
> 
>    struct family_op_req {
>        struct {
>              u32 a:1;
>              u32 b:1;
> 	    u32 bin_len;
>        } _present;
> 
>        u32 a;
>        u64 b;
>        const unsigned char *bin;
>    };
> 
> Note that the bin object 'bin' has a length stored, and that length
> has a _len suffix added to the field name. This breaks if there
> is a explicit field called bin_len, which is the case for some
> TC actions. Move the length fields out of the _present struct,
> create a new struct called _len:
> 
>    struct family_op_req {
>        struct {
>              u32 a:1;
>              u32 b:1;
>        } _present;
>        struct {
> 	    u32 bin;
>        } _len;
> 
>        u32 a;
>        u64 b;
>        const unsigned char *bin;
>    };
> 
> This should prevent name collisions and help with the packing
> of the struct.
> 
> Unfortunately this is a breaking change, but hopefully the migration
> isn't too painful.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>   tools/net/ynl/samples/devlink.c  |  2 +-
>   tools/net/ynl/samples/rt-addr.c  |  4 +--
>   tools/net/ynl/samples/rt-route.c |  4 +--
>   tools/net/ynl/pyynl/ynl_gen_c.py | 46 ++++++++++++++++----------------
>   4 files changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/tools/net/ynl/samples/devlink.c b/tools/net/ynl/samples/devlink.c
> index d2611d7ebab4..3d32a6335044 100644
> --- a/tools/net/ynl/samples/devlink.c
> +++ b/tools/net/ynl/samples/devlink.c
> @@ -34,7 +34,7 @@ int main(int argc, char **argv)
>   		if (!info_rsp)
>   			goto err_free_devs;
>   
> -		if (info_rsp->_present.info_driver_name_len)
> +		if (info_rsp->_len.info_driver_name)
>   			printf("    driver: %s\n", info_rsp->info_driver_name);
>   		if (info_rsp->n_info_version_running)
>   			printf("    running fw:\n");
> diff --git a/tools/net/ynl/samples/rt-addr.c b/tools/net/ynl/samples/rt-addr.c
> index 0f4851b4ec57..2edde5c36b18 100644
> --- a/tools/net/ynl/samples/rt-addr.c
> +++ b/tools/net/ynl/samples/rt-addr.c
> @@ -20,7 +20,7 @@ static void rt_addr_print(struct rt_addr_getaddr_rsp *a)
>   	if (name)
>   		printf("%16s: ", name);
>   
> -	switch (a->_present.address_len) {
> +	switch (a->_len.address) {
>   	case 4:
>   		addr = inet_ntop(AF_INET, a->address,
>   				 addr_str, sizeof(addr_str));
> @@ -36,7 +36,7 @@ static void rt_addr_print(struct rt_addr_getaddr_rsp *a)
>   	if (addr)
>   		printf("%s", addr);
>   	else
> -		printf("[%d]", a->_present.address_len);
> +		printf("[%d]", a->_len.address);
>   
>   	printf("\n");
>   }
> diff --git a/tools/net/ynl/samples/rt-route.c b/tools/net/ynl/samples/rt-route.c
> index 9d9c868f8873..7427104a96df 100644
> --- a/tools/net/ynl/samples/rt-route.c
> +++ b/tools/net/ynl/samples/rt-route.c
> @@ -26,13 +26,13 @@ static void rt_route_print(struct rt_route_getroute_rsp *r)
>   			printf("oif: %-16s ", name);
>   	}
>   
> -	if (r->_present.dst_len) {
> +	if (r->_len.dst) {
>   		route = inet_ntop(r->_hdr.rtm_family, r->dst,
>   				  route_str, sizeof(route_str));
>   		printf("dst: %s/%d", route, r->_hdr.rtm_dst_len);
>   	}
>   
> -	if (r->_present.gateway_len) {
> +	if (r->_len.gateway) {
>   		route = inet_ntop(r->_hdr.rtm_family, r->gateway,
>   				  route_str, sizeof(route_str));
>   		printf("gateway: %s ", route);
> diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
> index f93e6e79312a..800710fe96c9 100755
> --- a/tools/net/ynl/pyynl/ynl_gen_c.py
> +++ b/tools/net/ynl/pyynl/ynl_gen_c.py
> @@ -154,7 +154,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
>   
>           if self.presence_type() == 'len':
>               pfx = '__' if space == 'user' else ''
> -            return f"{pfx}u32 {self.c_name}_len;"
> +            return f"{pfx}u32 {self.c_name};"
>   
>       def _complex_member_type(self, ri):
>           return None
> @@ -217,10 +217,9 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
>           cw.p(f'[{self.enum_name}] = {"{"} .name = "{self.name}", {typol}{"}"},')
>   
>       def _attr_put_line(self, ri, var, line):
> -        if self.presence_type() == 'present':
> -            ri.cw.p(f"if ({var}->_present.{self.c_name})")
> -        elif self.presence_type() == 'len':
> -            ri.cw.p(f"if ({var}->_present.{self.c_name}_len)")
> +        presence = self.presence_type()
> +        if presence in {'present', 'len'}:
> +            ri.cw.p(f"if ({var}->_{presence}.{self.c_name})")
>           ri.cw.p(f"{line};")
>   
>       def _attr_put_simple(self, ri, var, put_type):
> @@ -282,6 +281,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
>               # Every layer below last is a nest, so we know it uses bit presence
>               # last layer is "self" and may be a complex type
>               if i == len(ref) - 1 and self.presence_type() != 'present':
> +                presence = f"{var}->{'.'.join(ref[:i] + [''])}_{self.presence_type()}.{ref[i]}"

Can this go a few lines higher and replace:

             presence = f"{var}->{'.'.join(ref[:i] + [''])}_present.{ref[i]}"

Since self.presence_type() would always return the correct string,
including "_present"?

Re: [PATCH net-next 2/3] tools: ynl-gen: split presence metadata

[Jakub Kicinski]

On Mon, 5 May 2025 14:06:29 -0700 David Wei wrote:
> > @@ -282,6 +281,7 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
> >               # Every layer below last is a nest, so we know it uses bit presence
> >               # last layer is "self" and may be a complex type
> >               if i == len(ref) - 1 and self.presence_type() != 'present':
> > +                presence = f"{var}->{'.'.join(ref[:i] + [''])}_{self.presence_type()}.{ref[i]}"  
> 
> Can this go a few lines higher and replace:
> 
>              presence = f"{var}->{'.'.join(ref[:i] + [''])}_present.{ref[i]}"
> 
> Since self.presence_type() would always return the correct string,
> including "_present"?

Hm, I don't think so. This is some of the gnarliest code the codegen
has, but it tries to generate something like:

	req->_present.options = 1;
	req->options._present.cgroup = 1;
	req->options.cgroup._count.act = n_act;

We have a nested member called "act". It's two nesting layers deep
inside the requests. We need to mark the "act" as present, but also
all parent nests as present. The nests always have presence type
of "present" (that's why its hardcoded at the start of the loop).
Last layer is the "real" presence type of the attribute, so we use
self.presence_type().

Re: [PATCH net-next 0/3] tools: ynl-gen: split presence metadata

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon,  5 May 2025 09:52:04 -0700 you wrote:
> The presence metadata indicates whether given attribute was/should be
> added to the Netlink message. We have 3 types of such metadata:
>  - bit presence for simple values like integers,
>  - len presence for variable size attrs, like binary and strings,
>  - count for arrays.
> 
> Previously this information was spread around with first two types
> living in a dedicated sub-struct called _present. The counts resided
> directly in the main struct with an n_ prefix.
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] tools: ynl-gen: rename basic presence from 'bit' to 'present'
    https://git.kernel.org/netdev/net-next/c/a512be0ecb14
  - [net-next,2/3] tools: ynl-gen: split presence metadata
    (no matching commit)
  - [net-next,3/3] tools: ynl-gen: move the count into a presence struct too
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html