| Message ID | 20240327160302.69378-1-donald.hunter@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v1] tools/net/ynl: Add extack policy attribute decoding | expand |
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 >
Jakub Kicinski <kuba@kernel.org> writes: > On Wed, 27 Mar 2024 16:03:02 +0000 Donald Hunter wrote: > > Nice! > > Some optional comments below... > >> + 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. Yep, makes sense, I'll collapse these into min-value and max-value. > >> + 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. Sure, I can drop these. > >> + 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 >>
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') + 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') + 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
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'} Signed-off-by: Donald Hunter <donald.hunter@gmail.com> --- tools/net/ynl/lib/ynl.py | 50 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)