[patch net-next v3] tools: ynl: introduce option to process unknown attributes or types

[patch net-next v3] tools: ynl: introduce option to process unknown attributes or types

[Jiri Pirko]

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>
---
v2->v3:
- rebased on top of previous patchset and recent net-next
- removed fake attr spec class
- introduced "attr.is_nest" and using it instead of direct access
  to "attr._type"
- pushed out rsp value addition into separate helper and sanitize
  the unknown attr is possibly multi-value there
- pushed out unknown attr decode into separate helper
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 | 47 ++++++++++++++++++++++++++++++----------
 2 files changed, 38 insertions(+), 12 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 b1da4aea9336..9e4ac9575313 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -100,6 +100,7 @@ class NlAttr:
     def __init__(self, raw, offset):
         self._len, self._type = struct.unpack("HH", raw[offset:offset + 4])
         self.type = self._type & ~Netlink.NLA_TYPE_MASK
+        self.is_nest = self._type & Netlink.NLA_F_NESTED
         self.payload_len = self._len
         self.full_len = (self.payload_len + 3) & ~3
         self.raw = raw[offset + 4:offset + self.payload_len]
@@ -411,10 +412,11 @@ class GenlProtocol(NetlinkProtocol):
 
 
 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":
@@ -526,14 +528,40 @@ class YnlFamily(SpecFamily):
             decoded.append({ item.type: subattrs })
         return decoded
 
+    def _decode_unknown(self, attr):
+        if attr.is_nest:
+            return self._decode(NlAttrs(attr.raw), None)
+        else:
+            return attr.as_bin()
+
+    def _rsp_add(self, rsp, name, is_multi, decoded):
+        if is_multi == None:
+            if name in rsp and type(rsp[name]) is not list:
+                rsp[name] = [rsp[name]]
+                is_multi = True
+            else:
+                is_multi = False
+
+        if not is_multi:
+            rsp[name] = decoded
+        elif name in rsp:
+            rsp[name].append(decoded)
+        else:
+            rsp[name] = [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}'")
+                self._rsp_add(rsp, str(attr.type), None, self._decode_unknown(attr))
+                continue
+
             if attr_spec["type"] == 'nest':
                 subdict = self._decode(NlAttrs(attr.raw), attr_spec['nested-attributes'])
                 decoded = subdict
@@ -558,14 +586,11 @@ class YnlFamily(SpecFamily):
                     selector = self._decode_enum(selector, attr_spec)
                 decoded = {"value": value, "selector": selector}
             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"]}')
+                decoded = self._decode_unknown(attr)
 
-            if not attr_spec.is_multi:
-                rsp[attr_spec['name']] = decoded
-            elif attr_spec.name in rsp:
-                rsp[attr_spec.name].append(decoded)
-            else:
-                rsp[attr_spec.name] = [decoded]
+            self._rsp_add(rsp, attr_spec["name"], attr_spec.is_multi, decoded)
 
         return rsp
 
-- 
2.41.0

Re: [patch net-next v3] tools: ynl: introduce option to process unknown attributes or types

[Jakub Kicinski]

On Wed, 25 Oct 2023 11:57:36 +0200 Jiri Pirko wrote:
> {'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''},

I'm not convinced, and still prefer leaving NlAttr objects in place.
-- 
pw-bot: reject

Re: [patch net-next v3] tools: ynl: introduce option to process unknown attributes or types

[Jiri Pirko]

Thu, Oct 26, 2023 at 02:56:36AM CEST, kuba@kernel.org wrote:
>On Wed, 25 Oct 2023 11:57:36 +0200 Jiri Pirko wrote:
>> {'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''},
>
>I'm not convinced, and still prefer leaving NlAttr objects in place.

If I understand that correctly, you would like to dump the
NlAttr.__repr__() as a printable representation of the objec, correct?

It yes, this is what I wrote in the discussion of v2:

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.


What is "type" and "len" good for here?



>-- 
>pw-bot: reject

Re: [patch net-next v3] tools: ynl: introduce option to process unknown attributes or types

[Jakub Kicinski]

On Thu, 26 Oct 2023 07:42:33 +0200 Jiri Pirko wrote:
> {'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.
> 
> 
> What is "type" and "len" good for here?

I already gave you a longer explanation, if you don't like the
duplication, how about you stop keying them on a (stringified?!) id.

Re: [patch net-next v3] tools: ynl: introduce option to process unknown attributes or types

[Jakub Kicinski]

On Thu, 26 Oct 2023 07:41:20 -0700 Jakub Kicinski wrote:
> > What is "type" and "len" good for here?  
> 
> I already gave you a longer explanation, if you don't like the
> duplication, how about you stop keying them on a (stringified?!) id.

Let's step back, why do you needs this?
Is what you're trying to decode inherently un-typed?
Or is it truly just for ease of writing specs for old families?

Re: [patch net-next v3] tools: ynl: introduce option to process unknown attributes or types

[Jiri Pirko]

Thu, Oct 26, 2023 at 04:46:38PM CEST, kuba@kernel.org wrote:
>On Thu, 26 Oct 2023 07:41:20 -0700 Jakub Kicinski wrote:
>> > What is "type" and "len" good for here?  
>> 
>> I already gave you a longer explanation, if you don't like the
>> duplication, how about you stop keying them on a (stringified?!) id.
>
>Let's step back, why do you needs this?
>Is what you're trying to decode inherently un-typed?
>Or is it truly just for ease of writing specs for old families?

When running this with newer kernel which supports unknown attr would be
another usecase, yes. Better to print out known attr then keyerror.

Re: [patch net-next v3] tools: ynl: introduce option to process unknown attributes or types

[Jiri Pirko]

Thu, Oct 26, 2023 at 04:41:20PM CEST, kuba@kernel.org wrote:
>On Thu, 26 Oct 2023 07:42:33 +0200 Jiri Pirko wrote:
>> {'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.
>> 
>> 
>> What is "type" and "len" good for here?
>
>I already gave you a longer explanation, if you don't like the
>duplication, how about you stop keying them on a (stringified?!) id.

I don't care that much, it just looks weird :)

Re: [patch net-next v3] tools: ynl: introduce option to process unknown attributes or types

[Jakub Kicinski]

On Thu, 26 Oct 2023 18:25:14 +0200 Jiri Pirko wrote:
> Thu, Oct 26, 2023 at 04:41:20PM CEST, kuba@kernel.org wrote:
> >On Thu, 26 Oct 2023 07:42:33 +0200 Jiri Pirko wrote:  
> >> {'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.
> >> 
> >> 
> >> What is "type" and "len" good for here?  
> >
> >I already gave you a longer explanation, if you don't like the
> >duplication, how about you stop keying them on a (stringified?!) id.  
> 
> I don't care that much, it just looks weird :)

As I said my key requirement is that the NlAttr object must still 
be there in the result.

Maybe a good compromise is to stick it into the key, instead of the
value. Replacing the stringified type id. Then you can keep the
value as binary. We'd need to wrap it into another class but whatever,
compromises.

IDK how this works in Python exactly but to give you a rough idea
here's pseudo code typed in the email client:

class UnknownNlAttrKey:
	def __init__(self, nlattr):
		self.nla = nlattr
	def __hash__(self):
		return self.nla.type
	def __eq__(self, other):
		if isintance(other, Unknown...):
			return other.nla.type == self.nla.type
		return False
	def __repr__():
		return f"UnknownAttr({self.nla.type})"

Re: [patch net-next v3] tools: ynl: introduce option to process unknown attributes or types

[Jiri Pirko]

Thu, Oct 26, 2023 at 09:30:58PM CEST, kuba@kernel.org wrote:
>On Thu, 26 Oct 2023 18:25:14 +0200 Jiri Pirko wrote:
>> Thu, Oct 26, 2023 at 04:41:20PM CEST, kuba@kernel.org wrote:
>> >On Thu, 26 Oct 2023 07:42:33 +0200 Jiri Pirko wrote:  
>> >> {'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.
>> >> 
>> >> 
>> >> What is "type" and "len" good for here?  
>> >
>> >I already gave you a longer explanation, if you don't like the
>> >duplication, how about you stop keying them on a (stringified?!) id.  
>> 
>> I don't care that much, it just looks weird :)
>
>As I said my key requirement is that the NlAttr object must still 
>be there in the result.

Yeah, that I don't how to do honestly. See below.


>
>Maybe a good compromise is to stick it into the key, instead of the
>value. Replacing the stringified type id. Then you can keep the
>value as binary.

Okay, that sounds good. But "key": \bvalue is not something to be
printed out by __repr__() as it outs string. Therefore I don't
understand how this compiles with your key requirement above.
I have to be missing something, pardon my ignorance.



>We'd need to wrap it into another class but whatever,
>compromises.

Will check on how to implement this.

>
>IDK how this works in Python exactly but to give you a rough idea
>here's pseudo code typed in the email client:
>
>class UnknownNlAttrKey:
>	def __init__(self, nlattr):
>		self.nla = nlattr
>	def __hash__(self):
>		return self.nla.type
>	def __eq__(self, other):
>		if isintance(other, Unknown...):
>			return other.nla.type == self.nla.type
>		return False
>	def __repr__():
>		return f"UnknownAttr({self.nla.type})"

I see, will check if this is needed.

Re: [patch net-next v3] tools: ynl: introduce option to process unknown attributes or types

[Jakub Kicinski]

On Fri, 27 Oct 2023 10:36:51 +0200 Jiri Pirko wrote:
> >Maybe a good compromise is to stick it into the key, instead of the
> >value. Replacing the stringified type id. Then you can keep the
> >value as binary.  
> 
> Okay, that sounds good. But "key": \bvalue is not something to be
> printed out by __repr__() as it outs string. Therefore I don't
> understand how this compiles with your key requirement above.
> I have to be missing something, pardon my ignorance.

FWIW the assignment would then become (pseudo-code):

	if real attr:
		rsp[name] = [decoded]
	else:
		rsp[UnknownNlAttrKey(nla)] = self._decode_unknown(nla)