Re: [PATCH RFC 01/20] ctlv: Add the schema language and generator

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] ctlvgen: ABI compatibility checker incorrectly flags widening to unlimited as an error
- [Low] ctlvgen: missing validation for --mode uapi --source causes KeyError
--

commit dbbe210aabdf036fa0f92c7b8932bca4fd127ce7
Author: Jiri Pirko <jiri@nvidia.com>

ctlv: Add the schema language and generator

Define the YAML schema and append-only family registry. Add ctlvgen to
validate specifications and emit kernel and UAPI outputs.

> diff --git a/tools/ctlv/ctlvgen/cli.py b/tools/ctlv/ctlvgen/cli.py
> new file mode 100755
> index 0000000000000..c6bfafeb28525
> --- /dev/null
> +++ b/tools/ctlv/ctlvgen/cli.py

[ ... ]

> @@ -0,0 +1,172 @@
[ ... ]
> +    kinds = (args.header, args.source)
> +    if args.mode and not any(kinds):
> +        parser.error('--mode needs --header or --source')
> +    if any(kinds) and not args.mode:
> +        parser.error('--header and --source need --mode')

[Severity: Low]
Should the command-line parsing prevent the combination of --mode uapi
and --source here?

Right now, this combination passes validation, but the GENERATORS dictionary
only maps ('uapi', 'header'). If a user provides --mode uapi --source, it
will cause an unhandled KeyError later in _generate() when looking up the
output renderer.

> +    if args.schema_only and args.mode != 'kernel':
> +        parser.error('--schema-only needs --mode kernel')

[ ... ]

> +    if args.mode == 'kernel':
> +        renderer = (gen_kernel.render_header if args.header
> +                    else gen_kernel.render_source)
> +        text = renderer(family, family_api=not args.schema_only)
> +    else:
> +        text = GENERATORS[(args.mode, kind)](family)

[ ... ]

> diff --git a/tools/ctlv/ctlvgen/lib/compat.py b/tools/ctlv/ctlvgen/lib/compat.py
> new file mode 100644
> index 0000000000000..4cf93af06ea90
> --- /dev/null
> +++ b/tools/ctlv/ctlvgen/lib/compat.py

[ ... ]

> @@ -0,0 +1,821 @@
[ ... ]
> +def _compare_occurrences(problems, where, was, now):
> +    old_max = was['max-occurrences']
> +    new_max = now['max-occurrences']
> +    if old_max == new_max:
> +        return
> +    if old_max == spec.UNLIMITED or new_max < old_max:

[Severity: Medium]
Will this incorrectly flag a compatible widening as an ABI break?

Because spec.UNLIMITED is represented as 0, widening max-occurrences
from a finite value like 10 to unlimited means new_max is 0 and
old_max is 10. The condition new_max < old_max evaluates to true
(0 < 10), misidentifying the widened bound as a narrowed bound.

Does this check need to specifically ignore comparisons where new_max
is spec.UNLIMITED?

> +        problems.append(f'{where}: max-occurrences was narrowed from '
> +                        f'{_occurrences(old_max)} to {_occurrences(new_max)}')
> +
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015145053.3664509-1-jiri@resnulli.us?part=1