| Message ID | 20231016110222.465453-1-jiri@resnulli.us (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] tools: ynl: introduce option to process unknown attributes or types | expand |
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.
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?
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?
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.
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)