diff mbox series

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

Message ID 20231025095736.801231-1-jiri@resnulli.us (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net-next,v3] tools: ynl: introduce option to process unknown attributes or types | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 9 this patch: 9
netdev/cc_maintainers warning 1 maintainers not CCed: donald.hunter@gmail.com
netdev/build_clang success Errors and warnings before: 9 this patch: 9
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 9 this patch: 9
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 95 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jiri Pirko Oct. 25, 2023, 9:57 a.m. UTC
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(-)

Comments

Jakub Kicinski Oct. 26, 2023, 12:56 a.m. UTC | #1
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.
Jiri Pirko Oct. 26, 2023, 5:42 a.m. UTC | #2
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
Jakub Kicinski Oct. 26, 2023, 2:41 p.m. UTC | #3
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.
Jakub Kicinski Oct. 26, 2023, 2:46 p.m. UTC | #4
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?
Jiri Pirko Oct. 26, 2023, 4:24 p.m. UTC | #5
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.
Jiri Pirko Oct. 26, 2023, 4:25 p.m. UTC | #6
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 :)
Jakub Kicinski Oct. 26, 2023, 7:30 p.m. UTC | #7
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})"
Jiri Pirko Oct. 27, 2023, 8:36 a.m. UTC | #8
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.
Jakub Kicinski Oct. 27, 2023, 2:08 p.m. UTC | #9
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)
diff mbox series

Patch

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