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 |
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.
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
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.
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?
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.
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 :)
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})"
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.
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 --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