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

[sashiko-bot@kernel.org]

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()`?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-ynl_rt_encap-v3-0-4106c21b9ee7@kylinos.cn?part=2