Re: [PATCH net-next v3 2/6] tools: ynl: convert enum selector to string for sub-message parsing

[Hangbin Liu <hangbin.liu@linux.dev>]

On Thu, Oct 01, 2026 at 01:51:34AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] ynl: early return 0 aborts entire parsing loop on unknown sub-message
> - [Medium] ynl: invalid C code generated for external enum selectors
> --
> 
> commit f2c434cd3f008807f1723585dcbfd634aa3f28b7
> Author: Hangbin Liu <liuhangbin@kylinos.cn>
> 
> tools: ynl: convert enum selector to string for sub-message parsing
> 
> YNL sub-message parsing expects a string selector for strcmp(). So for
> non-external enum selectors, convert the integer value to its string form
> via the family's {enum}_str() helper. This enables correct decoding of
> sub-messages keyed by enum values.
> 
> > diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
> > index 15c79849c6096..783242537fe48 100755
> > --- a/tools/net/ynl/pyynl/ynl_gen_c.py
> > +++ b/tools/net/ynl/pyynl/ynl_gen_c.py
> > @@ -951,13 +951,31 @@ class TypeSubMessage(TypeNest):
> >              sel_var = f"_sel_{sel}"
> >          else:
> >              sel_var = f"{var}->{sel}"
> > -        get_lines = [f'if (!{sel_var})',
> > -                     f'return ynl_submsg_failed(yarg, "{self.name}", "{selector}");',
> > -                     f"if ({self.nested_render_name}_parse(&parg, {sel_var}, attr))",
> > -                     "return YNL_PARSE_CB_ERROR;"]
> > +
> > +        local_vars = None
> > +
> > +        if self.selector.is_enum_val():
>             ^^^^
> 
> [Severity: Medium]
> Will this generate invalid C code for external enum selectors?
> 
> The condition doesn't check if the selector is external. If a YAML spec
> defines a sub-message with an external enum selector, it appears this will
> generate a non-existent structure member access for the presence variable:
> 
>     pres_var = f"{var}->_present.{sel}"
> 
> and try to pass the string pointer to the enum string helper, which expects
> an integer. Should this check include `and not self.selector.is_external()`?

OK...

> 
> > +            enum = self.family.consts[self.selector.get_enum_name()]
> > +            pres_var = f"{var}->_present.{sel}"
> > +            parse_sel = f"{sel}_str"
> > +            local_vars = [f'const char *{parse_sel};']
> > +
> > +            get_lines = [
> > +                f'if (!{pres_var})',
> > +                f'return ynl_submsg_failed(yarg, "{self.name}", "{selector}");',
> > +                f'{parse_sel} = {enum.render_name}_str({sel_var});',
> > +                f'if (!{parse_sel})',
> > +                'return 0;']
>                  ^^^^
> 
> [Severity: High]
> Does returning 0 here abort the entire parsing loop?
> 
> If a generated YNL parser encounters an unrecognized sub-message enum
> selector (for example, sent by a newer kernel), it looks like `return 0;`
> executes directly inside the attribute loop of the parsing function.
> 
> This would silently abort processing the remainder of the Netlink message and
> drop all trailing attributes without returning an error. Is it possible to
> skip the attribute and continue parsing instead?

OK, I will use continue then.

Thanks
Hangbin