Re: [PATCH net-next v1] tools/net/ynl: Add extack policy attribute decoding

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 27 Mar 2024 16:03:02 +0000 Donald Hunter wrote:
> The NLMSGERR_ATTR_POLICY extack attribute has been ignored by ynl up to
> now. Extend extack decoding to include _POLICY and the nested
> NL_POLICY_TYPE_ATTR_* attributes.
> 
> For example:
> 
> ./tools/net/ynl/cli.py \
>   --spec Documentation/netlink/specs/rt_link.yaml \
>   --create --do newlink --json '{
>     "ifname": "12345678901234567890",
>     "linkinfo": {"kind": "bridge"}
>     }'
> Netlink error: Numerical result out of range
> nl_len = 104 (88) nl_flags = 0x300 nl_type = 2
> 	error: -34	extack: {'msg': 'Attribute failed policy validation',
> 'policy': {'max-length': 15, 'type': 'string'}, 'bad-attr': '.ifname'}

Nice!

Some optional comments below...

> diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
> index 5fa7957f6e0f..557ef5a22b7d 100644
> --- a/tools/net/ynl/lib/ynl.py
> +++ b/tools/net/ynl/lib/ynl.py
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
>  
>  from collections import namedtuple
> +from enum import Enum
>  import functools
>  import os
>  import random
> @@ -76,6 +77,25 @@ class Netlink:
>      NLMSGERR_ATTR_MISS_TYPE = 5
>      NLMSGERR_ATTR_MISS_NEST = 6
>  
> +    # Policy types
> +    NL_POLICY_TYPE_ATTR_TYPE = 1
> +    NL_POLICY_TYPE_ATTR_MIN_VALUE_S = 2
> +    NL_POLICY_TYPE_ATTR_MAX_VALUE_S = 3
> +    NL_POLICY_TYPE_ATTR_MIN_VALUE_U = 4
> +    NL_POLICY_TYPE_ATTR_MAX_VALUE_U = 5
> +    NL_POLICY_TYPE_ATTR_MIN_LENGTH = 6
> +    NL_POLICY_TYPE_ATTR_MAX_LENGTH = 7
> +    NL_POLICY_TYPE_ATTR_POLICY_IDX = 8
> +    NL_POLICY_TYPE_ATTR_POLICY_MAXTYPE = 9
> +    NL_POLICY_TYPE_ATTR_BITFIELD32_MASK = 10
> +    NL_POLICY_TYPE_ATTR_PAD = 11
> +    NL_POLICY_TYPE_ATTR_MASK = 12
> +
> +    AttrType = Enum('AttrType', ['flag', 'u8', 'u16', 'u32', 'u64',
> +                                  's8', 's16', 's32', 's64',
> +                                  'binary', 'string', 'nul-string',
> +                                  'nested', 'nested-array',
> +                                  'bitfield32', 'sint', 'uint'])
>  
>  class NlError(Exception):
>    def __init__(self, nl_msg):
> @@ -198,6 +218,8 @@ class NlMsg:
>                      self.extack['miss-nest'] = extack.as_scalar('u32')
>                  elif extack.type == Netlink.NLMSGERR_ATTR_OFFS:
>                      self.extack['bad-attr-offs'] = extack.as_scalar('u32')
> +                elif extack.type == Netlink.NLMSGERR_ATTR_POLICY:
> +                    self.extack['policy'] = self._decode_policy(extack.raw)
>                  else:
>                      if 'unknown' not in self.extack:
>                          self.extack['unknown'] = []
> @@ -214,6 +236,34 @@ class NlMsg:
>                              desc += f" ({spec['doc']})"
>                          self.extack['miss-type'] = desc
>  
> +    def _decode_policy(self, raw):
> +        policy = {}
> +        for attr in NlAttrs(raw):
> +            if attr.type == Netlink.NL_POLICY_TYPE_ATTR_TYPE:
> +                type = attr.as_scalar('u32')
> +                policy['type'] = Netlink.AttrType(type).name
> +            elif attr.type == Netlink.NL_POLICY_TYPE_ATTR_MIN_VALUE_S:
> +                policy['min-value-s'] = attr.as_scalar('s64')
> +            elif attr.type == Netlink.NL_POLICY_TYPE_ATTR_MAX_VALUE_S:
> +                policy['max-value-s'] = attr.as_scalar('s64')
> +            elif attr.type == Netlink.NL_POLICY_TYPE_ATTR_MIN_VALUE_U:
> +                policy['min-value-u'] = attr.as_scalar('u64')
> +            elif attr.type == Netlink.NL_POLICY_TYPE_ATTR_MAX_VALUE_U:
> +                policy['max-value-u'] = attr.as_scalar('u64')

I think the signed / unsigned thing is primarily so that decode knows
if its s64 or u64. Is it useful for the person seeing the decoded
extack whether max was signed or unsigned?

IOW are we losing any useful info if we stop the -u / -s suffixes?

Otherwise I'd vote lose them.

> +            elif attr.type == Netlink.NL_POLICY_TYPE_ATTR_MIN_LENGTH:
> +                policy['min-length'] = attr.as_scalar('u32')
> +            elif attr.type == Netlink.NL_POLICY_TYPE_ATTR_MAX_LENGTH:
> +                policy['max-length'] = attr.as_scalar('u32')
> +            elif attr.type == Netlink.NL_POLICY_TYPE_ATTR_POLICY_IDX:
> +                policy['policy-idx'] = attr.as_scalar('u32')
> +            elif attr.type == Netlink.NL_POLICY_TYPE_ATTR_POLICY_MAXTYPE:
> +                policy['policy-maxtype'] = attr.as_scalar('u32')

I don't think these two (policy-..) can actually pop up in extack.
They are for cross-referencing nested policies in policy dumps.
extack only carries constraints local to the attr.

Up to you if you want to keep them.

> +            elif attr.type == Netlink.NL_POLICY_TYPE_ATTR_BITFIELD32_MASK:
> +                policy['bitfield32-mask'] = attr.as_scalar('u32')
> +            elif attr.type == Netlink.NL_POLICY_TYPE_ATTR_MASK:
> +                policy['mask'] = attr.as_scalar('u64')
> +        return policy
> +
>      def cmd(self):
>          return self.nl_type
>