[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <m2frh1j6jy.fsf@gmail.com>
Date: Mon, 19 May 2025 10:25:37 +0100
From: Donald Hunter <donald.hunter@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org,
jacob.e.keller@...el.com, sdf@...ichev.me, jstancek@...hat.com
Subject: Re: [PATCH net-next 08/11] tools: ynl-gen: support weird
sub-message formats
Jakub Kicinski <kuba@...nel.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@...nel.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@...il.com>
> + ri.cw.p(line)
> for line in get_lines:
> ri.cw.p(line)
> if arg.presence_type() == 'present':
Powered by blists - more mailing lists