* [PATCH net-next v1] tools/net/ynl: Add extack policy attribute decoding
@ 2024-03-27 16:03 Donald Hunter
2024-03-28 0:47 ` Jakub Kicinski
0 siblings, 1 reply; 3+ messages in thread
From: Donald Hunter @ 2024-03-27 16:03 UTC (permalink / raw)
To: netdev, Jakub Kicinski, David S. Miller, Eric Dumazet,
Paolo Abeni, Jiri Pirko, Jacob Keller, Stanislav Fomichev
Cc: donald.hunter, Donald Hunter
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(+)
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
--
2.44.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH net-next v1] tools/net/ynl: Add extack policy attribute decoding
2024-03-27 16:03 [PATCH net-next v1] tools/net/ynl: Add extack policy attribute decoding Donald Hunter
@ 2024-03-28 0:47 ` Jakub Kicinski
2024-03-28 15:32 ` Donald Hunter
0 siblings, 1 reply; 3+ messages in thread
From: Jakub Kicinski @ 2024-03-28 0:47 UTC (permalink / raw)
To: Donald Hunter
Cc: netdev, David S. Miller, Eric Dumazet, Paolo Abeni, Jiri Pirko,
Jacob Keller, Stanislav Fomichev, donald.hunter
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net-next v1] tools/net/ynl: Add extack policy attribute decoding
2024-03-28 0:47 ` Jakub Kicinski
@ 2024-03-28 15:32 ` Donald Hunter
0 siblings, 0 replies; 3+ messages in thread
From: Donald Hunter @ 2024-03-28 15:32 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, David S. Miller, Eric Dumazet, Paolo Abeni, Jiri Pirko,
Jacob Keller, Stanislav Fomichev, donald.hunter
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
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-03-28 15:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-27 16:03 [PATCH net-next v1] tools/net/ynl: Add extack policy attribute decoding Donald Hunter
2024-03-28 0:47 ` Jakub Kicinski
2024-03-28 15:32 ` Donald Hunter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).