Re: [PATCH net-next 08/11] tools: ynl-gen: support weird sub-message formats

[Donald Hunter <donald.hunter@gmail.com>]

Jakub Kicinski <kuba@kernel.org> writes:

> TC uses all possible sub-message formats:
>  - nested attrs
>  - fixed headers + nested attrs
>  - fixed headers
>  - empty
>
> Nested attrs are already supported for rt-link. Add support
> for remaining 3. The empty and fixed headers ones are fairly
> trivial, we can fake a Binary or Flags type instead of a Nest.
>
> For fixed headers + nest we need to teach nest parsing and
> nest put to handle fixed headers.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/net/ynl/lib/ynl-priv.h     |  8 +++--
>  tools/net/ynl/pyynl/ynl_gen_c.py | 51 ++++++++++++++++++++++++--------
>  2 files changed, 45 insertions(+), 14 deletions(-)
>
> diff --git a/tools/net/ynl/lib/ynl-priv.h b/tools/net/ynl/lib/ynl-priv.h
> index 416866f85820..824777d7e05e 100644
> --- a/tools/net/ynl/lib/ynl-priv.h
> +++ b/tools/net/ynl/lib/ynl-priv.h
> @@ -213,11 +213,15 @@ static inline void *ynl_attr_data_end(const struct nlattr *attr)
>  				     NLMSG_HDRLEN + fixed_hdr_sz); attr; \
>  	     (attr) = ynl_attr_next(ynl_nlmsg_end_addr(nlh), attr))
>  
> -#define ynl_attr_for_each_nested(attr, outer)				\
> +#define ynl_attr_for_each_nested_off(attr, outer, offset)		\
>  	for ((attr) = ynl_attr_first(outer, outer->nla_len,		\
> -				     sizeof(struct nlattr)); attr;	\
> +				     sizeof(struct nlattr) + offset);	\
> +	     attr;							\
>  	     (attr) = ynl_attr_next(ynl_attr_data_end(outer), attr))
>  
> +#define ynl_attr_for_each_nested(attr, outer)				\
> +	ynl_attr_for_each_nested_off(attr, outer, 0)
> +
>  #define ynl_attr_for_each_payload(start, len, attr)			\
>  	for ((attr) = ynl_attr_first(start, len, 0); attr;		\
>  	     (attr) = ynl_attr_next(start + len, attr))
> diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
> index f2a4404d0d21..5abf7dd86f42 100755
> --- a/tools/net/ynl/pyynl/ynl_gen_c.py
> +++ b/tools/net/ynl/pyynl/ynl_gen_c.py
> @@ -1372,12 +1372,25 @@ from lib import SpecSubMessage, SpecSubMessageFormat
>  
>          attrs = []
>          for name, fmt in submsg.formats.items():
> -            attrs.append({
> +            attr = {
>                  "name": name,
> -                "type": "nest",
>                  "parent-sub-message": spec,
> -                "nested-attributes": fmt['attribute-set']
> -            })
> +            }
> +            if 'attribute-set' in fmt:
> +                attr |= {
> +                    "type": "nest",
> +                    "nested-attributes": fmt['attribute-set'],
> +                }
> +                if 'fixed-header' in fmt:
> +                    attr |= { "fixed-header": fmt["fixed-header"] }
> +            elif 'fixed-header' in fmt:
> +                attr |= {
> +                    "type": "binary",
> +                    "struct": fmt["fixed-header"],
> +                }
> +            else:
> +                attr["type"] = "flag"
> +            attrs.append(attr)
>  
>          self.attr_sets[nested] = AttrSet(self, {
>              "name": nested,
> @@ -1921,8 +1934,11 @@ _C_KW = {
>  
>      i = 0
>      for name, arg in struct.member_list():
> -        cw.p('[%d] = { .type = YNL_PT_SUBMSG, .name = "%s", .nest = &%s_nest, },' %
> -             (i, name, arg.nested_render_name))
> +        nest = ""
> +        if arg.type == 'nest':
> +            nest = f" .nest = &{arg.nested_render_name}_nest,"
> +        cw.p('[%d] = { .type = YNL_PT_SUBMSG, .name = "%s",%s },' %
> +             (i, name, nest))
>          i += 1
>  
>      cw.block_end(line=';')
> @@ -2032,6 +2048,11 @@ _C_KW = {
>      if struct.submsg is None:
>          local_vars.append('struct nlattr *nest;')
>          init_lines.append("nest = ynl_attr_nest_start(nlh, attr_type);")
> +    if struct.fixed_header:
> +        local_vars.append('void *hdr;')
> +        struct_sz = f'sizeof({struct.fixed_header})'
> +        init_lines.append(f"hdr = ynl_nlmsg_put_extra_header(nlh, {struct_sz});")
> +        init_lines.append(f"memcpy(hdr, &obj->_hdr, {struct_sz});")
>  
>      has_anest = False
>      has_count = False
> @@ -2063,11 +2084,14 @@ _C_KW = {
>  
>  
>  def _multi_parse(ri, struct, init_lines, local_vars):
> +    if struct.fixed_header:
> +        local_vars += ['void *hdr;']
>      if struct.nested:
> -        iter_line = "ynl_attr_for_each_nested(attr, nested)"
> -    else:
>          if struct.fixed_header:
> -            local_vars += ['void *hdr;']
> +            iter_line = f"ynl_attr_for_each_nested_off(attr, nested, sizeof({struct.fixed_header}))"
> +        else:
> +            iter_line = "ynl_attr_for_each_nested(attr, nested)"
> +    else:
>          iter_line = "ynl_attr_for_each(attr, nlh, yarg->ys->family->hdr_len)"
>          if ri.op.fixed_header != ri.family.fixed_header:
>              if ri.family.is_classic():
> @@ -2114,7 +2138,9 @@ _C_KW = {
>          ri.cw.p(f'dst->{arg} = {arg};')
>  
>      if struct.fixed_header:
> -        if ri.family.is_classic():
> +        if struct.nested:
> +            ri.cw.p('hdr = ynl_attr_data(nested);')
> +        elif ri.family.is_classic():
>              ri.cw.p('hdr = ynl_nlmsg_data(nlh);')
>          else:
>              ri.cw.p('hdr = ynl_nlmsg_data_offset(nlh, sizeof(struct genlmsghdr));')
> @@ -2234,8 +2260,9 @@ _C_KW = {
>  
>          ri.cw.block_start(line=f'{kw} (!strcmp(sel, "{name}"))')
>          get_lines, init_lines, _ = arg._attr_get(ri, var)
> -        for line in init_lines:
> -            ri.cw.p(line)
> +        if init_lines:
> +            for line in init_lines:

I have a tiny preference for this construction, to eliminate the if
statement. WDYT?

           for line in init_lines or []:

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

> +                ri.cw.p(line)
>          for line in get_lines:
>              ri.cw.p(line)
>          if arg.presence_type() == 'present':