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

[David Wei <dw@davidwei.uk>]

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"?