* [patch net-next v2] tools: ynl: introduce option to process unknown attributes or types
@ 2023-10-16 11:02 Jiri Pirko
2023-10-17 0:59 ` Jakub Kicinski
0 siblings, 1 reply; 5+ messages in thread
From: Jiri Pirko @ 2023-10-16 11:02 UTC (permalink / raw)
To: netdev; +Cc: kuba, pabeni, davem, edumazet
From: Jiri Pirko <jiri@nvidia.com>
In case the kernel sends message back containing attribute not defined
in family spec, following exception is raised to the user:
$ sudo ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/devlink.yaml --do trap-get --json '{"bus-name": "netdevsim", "dev-name": "netdevsim1", "trap-name": "source_mac_is_multicast"}'
Traceback (most recent call last):
File "/home/jiri/work/linux/tools/net/ynl/lib/ynl.py", line 521, in _decode
attr_spec = attr_space.attrs_by_val[attr.type]
~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^
KeyError: 132
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/jiri/work/linux/./tools/net/ynl/cli.py", line 61, in <module>
main()
File "/home/jiri/work/linux/./tools/net/ynl/cli.py", line 49, in main
reply = ynl.do(args.do, attrs, args.flags)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/jiri/work/linux/tools/net/ynl/lib/ynl.py", line 731, in do
return self._op(method, vals, flags)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/jiri/work/linux/tools/net/ynl/lib/ynl.py", line 719, in _op
rsp_msg = self._decode(decoded.raw_attrs, op.attr_set.name)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/jiri/work/linux/tools/net/ynl/lib/ynl.py", line 525, in _decode
raise Exception(f"Space '{space}' has no attribute with value '{attr.type}'")
Exception: Space 'devlink' has no attribute with value '132'
Introduce a command line option "process-unknown" and pass it down to
YnlFamily class constructor to allow user to process unknown
attributes and types and print them as binaries.
$ sudo ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/devlink.yaml --do trap-get --json '{"bus-name": "netdevsim", "dev-name": "netdevsim1", "trap-name": "source_mac_is_multicast"}' --process-unknown
{'129': {'0': b'\x00\x00\x00\x00\x00\x00\x00\x00',
'1': b'\x00\x00\x00\x00\x00\x00\x00\x00',
'2': b'(\x00\x00\x00\x00\x00\x00\x00'},
'132': b'\x00',
'133': b'',
'134': {'0': b''},
'bus-name': 'netdevsim',
'dev-name': 'netdevsim1',
'trap-action': 'drop',
'trap-group-name': 'l2_drops',
'trap-name': 'source_mac_is_multicast'}
Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
v1->v2:
- changed to process unknown attributes and type instead of ignoring them
---
tools/net/ynl/cli.py | 3 ++-
tools/net/ynl/lib/ynl.py | 32 +++++++++++++++++++++++++++-----
2 files changed, 29 insertions(+), 6 deletions(-)
diff --git a/tools/net/ynl/cli.py b/tools/net/ynl/cli.py
index 564ecf07cd2c..2ad9ec0f5545 100755
--- a/tools/net/ynl/cli.py
+++ b/tools/net/ynl/cli.py
@@ -27,6 +27,7 @@ def main():
const=Netlink.NLM_F_CREATE)
parser.add_argument('--append', dest='flags', action='append_const',
const=Netlink.NLM_F_APPEND)
+ parser.add_argument('--process-unknown', action=argparse.BooleanOptionalAction)
args = parser.parse_args()
if args.no_schema:
@@ -36,7 +37,7 @@ def main():
if args.json_text:
attrs = json.loads(args.json_text)
- ynl = YnlFamily(args.spec, args.schema)
+ ynl = YnlFamily(args.spec, args.schema, args.process_unknown)
if args.ntf:
ynl.ntf_subscribe(args.ntf)
diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index 13c4b019a881..fe63e66694bb 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -403,11 +403,24 @@ class GenlProtocol(NetlinkProtocol):
#
+class FakeSpecAttr:
+ def __init__(self, name):
+ self.dict = {"name": name, "type": None}
+ self.is_multi = False
+
+ def __getitem__(self, key):
+ return self.dict[key]
+
+ def __contains__(self, key):
+ return key in self.dict
+
+
class YnlFamily(SpecFamily):
- def __init__(self, def_path, schema=None):
+ def __init__(self, def_path, schema=None, process_unknown=False):
super().__init__(def_path, schema)
self.include_raw = False
+ self.process_unknown = process_unknown
try:
if self.proto == "netlink-raw":
@@ -513,13 +526,16 @@ class YnlFamily(SpecFamily):
return decoded
def _decode(self, attrs, space):
- attr_space = self.attr_sets[space]
+ if space:
+ attr_space = self.attr_sets[space]
rsp = dict()
for attr in attrs:
try:
attr_spec = attr_space.attrs_by_val[attr.type]
- except KeyError:
- raise Exception(f"Space '{space}' has no attribute with value '{attr.type}'")
+ except (KeyError, UnboundLocalError):
+ if not self.process_unknown:
+ raise Exception(f"Space '{space}' has no attribute with value '{attr.type}'")
+ attr_spec = FakeSpecAttr(str(attr.type))
if attr_spec["type"] == 'nest':
subdict = self._decode(NlAttrs(attr.raw), attr_spec['nested-attributes'])
decoded = subdict
@@ -534,7 +550,13 @@ class YnlFamily(SpecFamily):
elif attr_spec["type"] == 'array-nest':
decoded = self._decode_array_nest(attr, attr_spec)
else:
- raise Exception(f'Unknown {attr_spec["type"]} with name {attr_spec["name"]}')
+ if not self.process_unknown:
+ raise Exception(f'Unknown {attr_spec["type"]} with name {attr_spec["name"]}')
+ if attr._type & Netlink.NLA_F_NESTED:
+ subdict = self._decode(NlAttrs(attr.raw), None)
+ decoded = subdict
+ else:
+ decoded = attr.as_bin()
if 'enum' in attr_spec:
decoded = self._decode_enum(decoded, attr_spec)
--
2.41.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [patch net-next v2] tools: ynl: introduce option to process unknown attributes or types
2023-10-16 11:02 [patch net-next v2] tools: ynl: introduce option to process unknown attributes or types Jiri Pirko
@ 2023-10-17 0:59 ` Jakub Kicinski
2023-10-17 6:18 ` Jiri Pirko
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Kicinski @ 2023-10-17 0:59 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, pabeni, davem, edumazet
On Mon, 16 Oct 2023 13:02:22 +0200 Jiri Pirko wrote:
> +class FakeSpecAttr:
> + def __init__(self, name):
> + self.dict = {"name": name, "type": None}
> + self.is_multi = False
> +
> + def __getitem__(self, key):
> + return self.dict[key]
> +
> + def __contains__(self, key):
> + return key in self.dict
Why the new class? Why not attach the NlAttr object directly?
I have an idea knocking about in my head to support "polymorphic"
nests (nests where decoding depends on value of another attr,
link rtnl link attrs or tc object attrs). The way I'm thinking
about doing it is to return NlAttr / struct nla_attr back to the user.
And let the users call a sub-parser of choice by hand.
So returning a raw NlAttr appeals to me more.
> + if not self.process_unknown:
> + raise Exception(f'Unknown {attr_spec["type"]} with name {attr_spec["name"]}')
> + if attr._type & Netlink.NLA_F_NESTED:
> + subdict = self._decode(NlAttrs(attr.raw), None)
> + decoded = subdict
> + else:
> + decoded = attr.as_bin()
Again, I wouldn't descend at all.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch net-next v2] tools: ynl: introduce option to process unknown attributes or types
2023-10-17 0:59 ` Jakub Kicinski
@ 2023-10-17 6:18 ` Jiri Pirko
2023-10-17 15:50 ` Jakub Kicinski
0 siblings, 1 reply; 5+ messages in thread
From: Jiri Pirko @ 2023-10-17 6:18 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, pabeni, davem, edumazet
Tue, Oct 17, 2023 at 02:59:03AM CEST, kuba@kernel.org wrote:
>On Mon, 16 Oct 2023 13:02:22 +0200 Jiri Pirko wrote:
>> +class FakeSpecAttr:
>> + def __init__(self, name):
>> + self.dict = {"name": name, "type": None}
>> + self.is_multi = False
>> +
>> + def __getitem__(self, key):
>> + return self.dict[key]
>> +
>> + def __contains__(self, key):
>> + return key in self.dict
>
>Why the new class? Why not attach the NlAttr object directly?
It's not NlAttr, it's SpecAttr. And that has a constructor with things I
cannot provide for fake object, that's why I did this dummy object.
>
>I have an idea knocking about in my head to support "polymorphic"
>nests (nests where decoding depends on value of another attr,
>link rtnl link attrs or tc object attrs). The way I'm thinking
>about doing it is to return NlAttr / struct nla_attr back to the user.
>And let the users call a sub-parser of choice by hand.
Sounds parallel to this patch, isn't it?
>
>So returning a raw NlAttr appeals to me more.
Wait, you suggest not to print out attr.as_bin(), but something else?
>
>> + if not self.process_unknown:
>> + raise Exception(f'Unknown {attr_spec["type"]} with name {attr_spec["name"]}')
>> + if attr._type & Netlink.NLA_F_NESTED:
>> + subdict = self._decode(NlAttrs(attr.raw), None)
>> + decoded = subdict
>> + else:
>> + decoded = attr.as_bin()
>
>Again, I wouldn't descend at all.
I don't care that much. I just thought it might be handy for the user to
understand the topology. Actually, I found it quite convenient already.
It's basically a direct dump. What is the reason not to do this exactly?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch net-next v2] tools: ynl: introduce option to process unknown attributes or types
2023-10-17 6:18 ` Jiri Pirko
@ 2023-10-17 15:50 ` Jakub Kicinski
2023-10-18 7:52 ` Jiri Pirko
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Kicinski @ 2023-10-17 15:50 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, pabeni, davem, edumazet
On Tue, 17 Oct 2023 08:18:13 +0200 Jiri Pirko wrote:
> Tue, Oct 17, 2023 at 02:59:03AM CEST, kuba@kernel.org wrote:
> >On Mon, 16 Oct 2023 13:02:22 +0200 Jiri Pirko wrote:
> >> +class FakeSpecAttr:
> >> + def __init__(self, name):
> >> + self.dict = {"name": name, "type": None}
> >> + self.is_multi = False
> >> +
> >> + def __getitem__(self, key):
> >> + return self.dict[key]
> >> +
> >> + def __contains__(self, key):
> >> + return key in self.dict
> >
> >Why the new class? Why not attach the NlAttr object directly?
>
> It's not NlAttr, it's SpecAttr. And that has a constructor with things I
> cannot provide for fake object, that's why I did this dummy object.
Just to be able to do spec["type"] on it?
There is an if "ladder", just replace the first
if attr_spec["type"] == ...
with
if attr_spec is None:
# your code
elif attr_spec["type"] == ...
hm?
> >I have an idea knocking about in my head to support "polymorphic"
> >nests (nests where decoding depends on value of another attr,
> >link rtnl link attrs or tc object attrs). The way I'm thinking
> >about doing it is to return NlAttr / struct nla_attr back to the user.
> >And let the users call a sub-parser of choice by hand.
>
> Sounds parallel to this patch, isn't it?
I'm just giving you extra info to explain my thinking.
Given how we struggle to understand each other lately :S
> >So returning a raw NlAttr appeals to me more.
>
> Wait, you suggest not to print out attr.as_bin(), but something else?
Yea, it should not be needed. NlAttr has a __repr__ which *I think*
should basically do the same thing? Or you may need to call that
__repr__ from __str__, I don't know what PrettyPrinter uses internally
> >> + if not self.process_unknown:
> >> + raise Exception(f'Unknown {attr_spec["type"]} with name {attr_spec["name"]}')
> >> + if attr._type & Netlink.NLA_F_NESTED:
> >> + subdict = self._decode(NlAttrs(attr.raw), None)
> >> + decoded = subdict
> >> + else:
> >> + decoded = attr.as_bin()
> >
> >Again, I wouldn't descend at all.
>
> I don't care that much. I just thought it might be handy for the user to
> understand the topology. Actually, I found it quite convenient already.
> It's basically a direct dump. What is the reason not to do this exactly?
No strong reason but you need to rewrite it to at least not access
attr._type directly.
I have a weak preference for putting this code in NlAttr's __repr__,
could be more broadly useful?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch net-next v2] tools: ynl: introduce option to process unknown attributes or types
2023-10-17 15:50 ` Jakub Kicinski
@ 2023-10-18 7:52 ` Jiri Pirko
0 siblings, 0 replies; 5+ messages in thread
From: Jiri Pirko @ 2023-10-18 7:52 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, pabeni, davem, edumazet
Tue, Oct 17, 2023 at 05:50:53PM CEST, kuba@kernel.org wrote:
>On Tue, 17 Oct 2023 08:18:13 +0200 Jiri Pirko wrote:
>> Tue, Oct 17, 2023 at 02:59:03AM CEST, kuba@kernel.org wrote:
>> >On Mon, 16 Oct 2023 13:02:22 +0200 Jiri Pirko wrote:
>> >> +class FakeSpecAttr:
>> >> + def __init__(self, name):
>> >> + self.dict = {"name": name, "type": None}
>> >> + self.is_multi = False
>> >> +
>> >> + def __getitem__(self, key):
>> >> + return self.dict[key]
>> >> +
>> >> + def __contains__(self, key):
>> >> + return key in self.dict
>> >
>> >Why the new class? Why not attach the NlAttr object directly?
>>
>> It's not NlAttr, it's SpecAttr. And that has a constructor with things I
>> cannot provide for fake object, that's why I did this dummy object.
>
>Just to be able to do spec["type"] on it?
Nope. Need .is_multi() and spec["name"] as well.
>
>There is an if "ladder", just replace the first
>
> if attr_spec["type"] == ...
>
>with
> if attr_spec is None:
> # your code
> elif attr_spec["type"] == ...
>
>hm?
Well, I need the same processing for "else".
Okay, I'll to this with local variables instead. Fake class looked a bit
more elegant.
>
>> >I have an idea knocking about in my head to support "polymorphic"
>> >nests (nests where decoding depends on value of another attr,
>> >link rtnl link attrs or tc object attrs). The way I'm thinking
>> >about doing it is to return NlAttr / struct nla_attr back to the user.
>> >And let the users call a sub-parser of choice by hand.
>>
>> Sounds parallel to this patch, isn't it?
>
>I'm just giving you extra info to explain my thinking.
>Given how we struggle to understand each other lately :S
Yeah :/
>
>> >So returning a raw NlAttr appeals to me more.
>>
>> Wait, you suggest not to print out attr.as_bin(), but something else?
>
>Yea, it should not be needed. NlAttr has a __repr__ which *I think*
>should basically do the same thing? Or you may need to call that
>__repr__ from __str__, I don't know what PrettyPrinter uses internally
Instead of:
{'129': {'0': b'\x00\x00\x00\x00\x00\x00\x00\x00',
'1': b'\x00\x00\x00\x00\x00\x00\x00\x00',
'2': b'(\x00\x00\x00\x00\x00\x00\x00'},
You'd get:
{'129': {'0': [type:0 len:12] b'\x00\x00\x00\x00\x00\x00\x00\x00',
'1': [type:1 len:12] b'\x00\x00\x00\x00\x00\x00\x00\x00',
'2': [type:2 len:12] b'(\x00\x00\x00\x00\x00\x00\x00'},
Looks like unnecessary redundant info, I would rather stick with
"as_bin()". __repr__() is printable representation of the whole object,
we just need value here, already have that in a structured object.
>
>> >> + if not self.process_unknown:
>> >> + raise Exception(f'Unknown {attr_spec["type"]} with name {attr_spec["name"]}')
>> >> + if attr._type & Netlink.NLA_F_NESTED:
>> >> + subdict = self._decode(NlAttrs(attr.raw), None)
>> >> + decoded = subdict
>> >> + else:
>> >> + decoded = attr.as_bin()
>> >
>> >Again, I wouldn't descend at all.
>>
>> I don't care that much. I just thought it might be handy for the user to
>> understand the topology. Actually, I found it quite convenient already.
>> It's basically a direct dump. What is the reason not to do this exactly?
>
>No strong reason but you need to rewrite it to at least not access
>attr._type directly.
Okay.
>
>I have a weak preference for putting this code in NlAttr's __repr__,
>could be more broadly useful?
As I pointed out above, it's a different use case. Here we do decoding
into structured object. __repr__() is fro plain str conversion.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-10-18 7:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-16 11:02 [patch net-next v2] tools: ynl: introduce option to process unknown attributes or types Jiri Pirko
2023-10-17 0:59 ` Jakub Kicinski
2023-10-17 6:18 ` Jiri Pirko
2023-10-17 15:50 ` Jakub Kicinski
2023-10-18 7:52 ` Jiri Pirko
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).