Re: [PATCH net-next 2/5] tools: ynl-gen: allow noncontiguous enums

[Jakub Kicinski <kuba@kernel.org>]

On Fri,  2 May 2025 13:38:18 +0200 Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> In case the enum has holes, instead of hard stop, generate a validation
> callback to check valid enum values.
> 
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> ---
> Saeed's v3->v1:
> - add validation callback generation
> ---
>  tools/net/ynl/pyynl/ynl_gen_c.py | 45 +++++++++++++++++++++++++++++---
>  1 file changed, 42 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
> index b4889974f645..c37551473923 100755
> --- a/tools/net/ynl/pyynl/ynl_gen_c.py
> +++ b/tools/net/ynl/pyynl/ynl_gen_c.py
> @@ -358,11 +358,13 @@ class TypeScalar(Type):
>          if 'enum' in self.attr:
>              enum = self.family.consts[self.attr['enum']]
>              low, high = enum.value_range()
> -            if 'min' not in self.checks:
> +            if low and 'min' not in self.checks:
>                  if low != 0 or self.type[0] == 's':
>                      self.checks['min'] = low
> -            if 'max' not in self.checks:
> +            if high and 'max' not in self.checks:
>                  self.checks['max'] = high
> +            if not low and not high:
> +                self.checks['sparse'] = True

you should probably explicitly check for None, 0 is a valid low / high
  
>          if 'min' in self.checks and 'max' in self.checks:
>              if self.get_limit('min') > self.get_limit('max'):

> +def print_kernel_policy_sparse_enum_validates(family, cw):
> +    first = True
> +    for _, attr_set in family.attr_sets.items():
> +        if attr_set.subset_of:
> +            continue
> +
> +        for _, attr in attr_set.items():
> +            if not attr.request:
> +                continue
> +            if not attr.enum_name:
> +                continue
> +            if 'sparse' not in attr.checks:
> +                continue
> +
> +            if first:
> +                cw.p('/* Sparse enums validation callbacks */')
> +                first = False
> +
> +            sign = '' if attr.type[0] == 'u' else '_signed'
> +            suffix = 'ULL' if attr.type[0] == 'u' else 'LL'
> +            cw.write_func_prot('static int', f'{c_lower(attr.enum_name)}_validate',
> +                               ['const struct nlattr *attr', 'struct netlink_ext_ack *extack'])
> +            cw.block_start()
> +            cw.block_start(line=f'switch (nla_get_{attr["type"]}(attr))', noind=True)
> +            enum = family.consts[attr['enum']]
> +            for entry in enum.entries.values():
> +                cw.p(f'case {entry.c_name}: return 0;')

All the cases end in "return 0;"
remove this, and add the return 0; before the block end.
The code should look something like:

	switch (nla_get_...) {
	case VAL1:
	case VAL2:
	case VAL3:
		return 0;
	}

> +            cw.block_end(noind=True)
> +            cw.p('NL_SET_ERR_MSG_ATTR(extack, attr, "invalid enum value");')
> +            cw.p('return -EINVAL;')
> +            cw.block_end()
> +            cw.nl()
> +
> +
>  def print_kernel_op_table_fwd(family, cw, terminate):
>      exported = not kernel_can_gen_family_struct(family)
>  
> @@ -2965,6 +3003,7 @@ def main():
>              print_kernel_family_struct_hdr(parsed, cw)
>          else:
>              print_kernel_policy_ranges(parsed, cw)
> +            print_kernel_policy_sparse_enum_validates(parsed, cw)
>  
>              for _, struct in sorted(parsed.pure_nested_structs.items()):
>                  if struct.request: