| Message ID | 20240123160538.172-3-donald.hunter@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tools/net/ynl: Add features for tc family | expand |
On Tue, 23 Jan 2024 16:05:28 +0000 Donald Hunter wrote: > Sub-message selectors could only be resolved using values from the > current nest level. Enable value lookup in outer scopes by using > collections.ChainMap to implement an ordered lookup from nested to > outer scopes. Meaning if the key is not found in current scope we'll silently and recursively try outer scopes? Did we already document that? I remember we discussed it, can you share a link to that discussion?
Jakub Kicinski <kuba@kernel.org> writes: > On Tue, 23 Jan 2024 16:05:28 +0000 Donald Hunter wrote: >> Sub-message selectors could only be resolved using values from the >> current nest level. Enable value lookup in outer scopes by using >> collections.ChainMap to implement an ordered lookup from nested to >> outer scopes. > > Meaning if the key is not found in current scope we'll silently and > recursively try outer scopes? Did we already document that? > I remember we discussed it, can you share a link to that discussion? Yes, it silently tries outer scopes. The previous discussion is here: https://patchwork.kernel.org/project/netdevbpf/patch/20231130214959.27377-7-donald.hunter@gmail.com/#25622101 This is the doc patch that describes sub-messages: https://patchwork.kernel.org/project/netdevbpf/patch/20231215093720.18774-4-donald.hunter@gmail.com/ It doesn't mention searching outer scopes so I can add that to the docs.
On Wed, 24 Jan 2024 09:37:31 +0000 Donald Hunter wrote: > > Meaning if the key is not found in current scope we'll silently and > > recursively try outer scopes? Did we already document that? > > I remember we discussed it, can you share a link to that discussion? > > Yes, it silently tries outer scopes. The previous discussion is here: > > https://patchwork.kernel.org/project/netdevbpf/patch/20231130214959.27377-7-donald.hunter@gmail.com/#25622101 > > This is the doc patch that describes sub-messages: > > https://patchwork.kernel.org/project/netdevbpf/patch/20231215093720.18774-4-donald.hunter@gmail.com/ > > It doesn't mention searching outer scopes so I can add that to the docs. I'm a tiny bit worried about the mis-ordered case. If the selector attr is after the sub-msg but outer scope has an attr of the same name we'll silently use the wrong one. It shouldn't happen in practice but can we notice the wrong ordering and error out cleanly?
Jakub Kicinski <kuba@kernel.org> writes: > On Wed, 24 Jan 2024 09:37:31 +0000 Donald Hunter wrote: >> > Meaning if the key is not found in current scope we'll silently and >> > recursively try outer scopes? Did we already document that? >> > I remember we discussed it, can you share a link to that discussion? >> >> Yes, it silently tries outer scopes. The previous discussion is here: >> >> https://patchwork.kernel.org/project/netdevbpf/patch/20231130214959.27377-7-donald.hunter@gmail.com/#25622101 >> >> This is the doc patch that describes sub-messages: >> >> https://patchwork.kernel.org/project/netdevbpf/patch/20231215093720.18774-4-donald.hunter@gmail.com/ >> >> It doesn't mention searching outer scopes so I can add that to the docs. > > I'm a tiny bit worried about the mis-ordered case. If the selector attr > is after the sub-msg but outer scope has an attr of the same name we'll > silently use the wrong one. It shouldn't happen in practice but can we > notice the wrong ordering and error out cleanly? I was quite pleased with how simple the patch turned out to be when I used ChainMap, but it does have this weakness. In practice, the only place this could be a problem is with tc-act-attrs which has the same attribute name 'kind' in the nest and in tc-attrs at the top level. If you send a create message with ynl, you could omit the 'kind' attr in the 'act' nest and ynl would incorrectly resolve to the top level 'kind'. The kernel would reject the action with a missing 'kind' but the rest of payload would be encoded wrongly and/or could break ynl. My initial thought is that this might be better handled as input validation, e.g. adding 'required: true' to the spec for 'act/kind'. After using ynl for a while, I think it would help to specify required attributes for messages, nests and sub-messsages. It's very hard to discover the required attributes for families that don't provide extack responses for errors. Thoughts?
On Fri, 26 Jan 2024 12:44:57 +0000 Donald Hunter wrote: > I was quite pleased with how simple the patch turned out to be when I > used ChainMap, but it does have this weakness. It is very neat, no question about it :( > In practice, the only place this could be a problem is with > tc-act-attrs which has the same attribute name 'kind' in the nest and > in tc-attrs at the top level. If you send a create message with ynl, > you could omit the 'kind' attr in the 'act' nest and ynl would > incorrectly resolve to the top level 'kind'. The kernel would reject > the action with a missing 'kind' but the rest of payload would be > encoded wrongly and/or could break ynl. We can detect the problem post-fact and throw an exception. I primarily care about removing the ambiguity. Is it possible to check at which "level" of the chainmap the key was found? If so we can also construct a 'chainmap of attr sets' and make sure that the key level == attr set level. I.e. that we got a hit at the first level which declares a key of that name. More crude option - we could construct a list of dicts (the levels within the chainmap) and keys they can't contain. Once we got a hit for a sub-message key at level A, all dicts currently on top of A are not allowed to add that key. Once we're done with the message we scan thru the list and make sure the keys haven't appeared? Another random thought, should we mark the keys which can "descend" somehow? IDK, put a ~ in front? selector: ~kind or some other char? > My initial thought is that this might be better handled as input > validation, e.g. adding 'required: true' to the spec for 'act/kind'. > After using ynl for a while, I think it would help to specify required > attributes for messages, nests and sub-messsages. It's very hard to > discover the required attributes for families that don't provide > extack responses for errors. Hah, required attrs. I have been sitting on patches for the kernel for over a year - https://github.com/kuba-moo/linux/tree/req-args Not sure if they actually work but for the kernel I was curious if it's possible to do the validation in constant time (in relation to the policy size, i.e. without scanning the entire policy at the end to confirm that all required attrs are present). And that's what I came up with. I haven't posted it because I was a tiny bit worried that required args will cause bugs (people forgetting to null check attrs) and may cause uAPI breakage down the line (we should clearly state that "required" status is just advisory, and can go away in future kernel release). But that was more of a on-the-fence situation. If you find them useful feel free to move forward! I do think that's a separate story, tho. For sub-message selector - isn't the key _implicitly_ required, in the first attr set where it is defined? Conversely if the sub-message isn't present the key isn't required any more either?
Jakub Kicinski <kuba@kernel.org> writes: > On Fri, 26 Jan 2024 12:44:57 +0000 Donald Hunter wrote: >> I was quite pleased with how simple the patch turned out to be when I >> used ChainMap, but it does have this weakness. > > It is very neat, no question about it :( > >> In practice, the only place this could be a problem is with >> tc-act-attrs which has the same attribute name 'kind' in the nest and >> in tc-attrs at the top level. If you send a create message with ynl, >> you could omit the 'kind' attr in the 'act' nest and ynl would >> incorrectly resolve to the top level 'kind'. The kernel would reject >> the action with a missing 'kind' but the rest of payload would be >> encoded wrongly and/or could break ynl. > > We can detect the problem post-fact and throw an exception. I primarily > care about removing the ambiguity. Agreed. > Is it possible to check at which "level" of the chainmap the key was > found? If so we can also construct a 'chainmap of attr sets' and make > sure that the key level == attr set level. I.e. that we got a hit at > the first level which declares a key of that name. > > More crude option - we could construct a list of dicts (the levels > within the chainmap) and keys they can't contain. Once we got a hit > for a sub-message key at level A, all dicts currently on top of A > are not allowed to add that key. Once we're done with the message we > scan thru the list and make sure the keys haven't appeared? > > Another random thought, should we mark the keys which can "descend" > somehow? IDK, put a ~ in front? > > selector: ~kind > > or some other char? Okay, so I think the behaviour we need is to either search current scope or search the outermost scope. My suggestion would be to replace the ChainMap approach with just choosing between current and outermost scope. The unusual case is needing to search the outermost scope so using a prefix e.g. '/' for that would work. We can have 'selector: kind' continue to refer to current scope and then have 'selector: /kind' refer to the outermost scope. If we run into a case that requires something other than current or outermost then we could add e.g. '../kind' so that the scope to search is always explicitly identified. >> My initial thought is that this might be better handled as input >> validation, e.g. adding 'required: true' to the spec for 'act/kind'. >> After using ynl for a while, I think it would help to specify required >> attributes for messages, nests and sub-messsages. It's very hard to >> discover the required attributes for families that don't provide >> extack responses for errors. > > Hah, required attrs. I have been sitting on patches for the kernel for > over a year - https://github.com/kuba-moo/linux/tree/req-args > Not sure if they actually work but for the kernel I was curious if it's > possible to do the validation in constant time (in relation to the > policy size, i.e. without scanning the entire policy at the end to > confirm that all required attrs are present). And that's what I came up > with. Interesting. It's definitely a thorny problem with varying sets of 'required' attributes. It could be useful to report the absolutely required attributes in policy responses, without any actual enforcement. Would it be possible to report policy for legacy netlink-raw families? Thinking about it, usability would probably be most improved by adding extack messages to more of the tc error paths. > I haven't posted it because I was a tiny bit worried that required args > will cause bugs (people forgetting to null check attrs) and may cause > uAPI breakage down the line (we should clearly state that "required" > status is just advisory, and can go away in future kernel release). > But that was more of a on-the-fence situation. If you find them useful > feel free to move forward! > > I do think that's a separate story, tho. For sub-message selector > - isn't the key _implicitly_ required, in the first attr set where > it is defined? Conversely if the sub-message isn't present the key > isn't required any more either? Yes, the key is implicitly required for sub-messages. The toplevel key is probably required regardless of the presence of a sub-message.
On 1/27/24 18:18, Donald Hunter wrote: > Jakub Kicinski <kuba@kernel.org> writes: >> Is it possible to check at which "level" of the chainmap the key was >> found? If so we can also construct a 'chainmap of attr sets' and make >> sure that the key level == attr set level. I.e. that we got a hit at >> the first level which declares a key of that name. >> >> More crude option - we could construct a list of dicts (the levels >> within the chainmap) and keys they can't contain. Once we got a hit >> for a sub-message key at level A, all dicts currently on top of A >> are not allowed to add that key. Once we're done with the message we >> scan thru the list and make sure the keys haven't appeared? >> >> Another random thought, should we mark the keys which can "descend" >> somehow? IDK, put a ~ in front? >> >> selector: ~kind >> >> or some other char? > Okay, so I think the behaviour we need is to either search current scope > or search the outermost scope. My suggestion would be to replace the > ChainMap approach with just choosing between current and outermost > scope. The unusual case is needing to search the outermost scope so > using a prefix e.g. '/' for that would work. > > We can have 'selector: kind' continue to refer to current scope and then > have 'selector: /kind' refer to the outermost scope. > > If we run into a case that requires something other than current or > outermost then we could add e.g. '../kind' so that the scope to search > is always explicitly identified. Wouldn't add different chars in front of the selctor value be confusing? IMHO the solution of using a ChainMap with levels could be an easier solution. We could just modify the __getitem__() method to output both the value and the level, and the get() method to add the chance to specify a level (in our case the level found in the spec) and error out if the specified level doesn't match with the found one. Something like this: from collections import ChainMap class LevelChainMap(ChainMap): def __getitem__(self, key): for mapping in self.maps: try: return mapping[key], self.maps[::-1].index(mapping) except KeyError: pass return self.__missing__(key) def get(self, key, default=None, level=None): val, lvl = self[key] if key in self else (default, None) if level: if lvl != level: raise Exception("Level mismatch") return val, lvl # example usage c = LevelChainMap({'a':1}, {'inner':{'a':1}}, {'outer': {'inner':{'a':1}}}) print(c.get('a', level=2)) print(c.get('a', level=1)) #raise err This will leave the spec as it is and will require small changes. What do you think?
Alessandro Marcolini <alessandromarcolini99@gmail.com> writes: > On 1/27/24 18:18, Donald Hunter wrote: >> Okay, so I think the behaviour we need is to either search current scope >> or search the outermost scope. My suggestion would be to replace the >> ChainMap approach with just choosing between current and outermost >> scope. The unusual case is needing to search the outermost scope so >> using a prefix e.g. '/' for that would work. >> >> We can have 'selector: kind' continue to refer to current scope and then >> have 'selector: /kind' refer to the outermost scope. >> >> If we run into a case that requires something other than current or >> outermost then we could add e.g. '../kind' so that the scope to search >> is always explicitly identified. > > Wouldn't add different chars in front of the selctor value be confusing? > > IMHO the solution of using a ChainMap with levels could be an easier solution. We could just > modify the __getitem__() method to output both the value and the level, and the get() method to > add the chance to specify a level (in our case the level found in the spec) and error out if the > specified level doesn't match with the found one. Something like this: If we take the approach of resolving the level from the spec then I wouldn't use ChainMap. Per the Python docs [1]: "A ChainMap class is provided for quickly linking a number of mappings so they can be treated as a single unit." I think we could instead pass a list of mappings from current to outermost and then just reference the correct level that was resolved from the spec. > from collections import ChainMap > > class LevelChainMap(ChainMap): > def __getitem__(self, key): > for mapping in self.maps: > try: > return mapping[key], self.maps[::-1].index(mapping) > except KeyError: > pass > return self.__missing__(key) > > def get(self, key, default=None, level=None): > val, lvl = self[key] if key in self else (default, None) > if level: > if lvl != level: > raise Exception("Level mismatch") > return val, lvl > > # example usage > c = LevelChainMap({'a':1}, {'inner':{'a':1}}, {'outer': {'inner':{'a':1}}}) > print(c.get('a', level=2)) > print(c.get('a', level=1)) #raise err > > This will leave the spec as it is and will require small changes. > > What do you think? The more I think about it, the more I agree that using path-like syntax in the selector is overkill. It makes sense to resolve the selector level from the spec and then directly access the mappings from the correct scope level. [1] https://docs.python.org/3/library/collections.html#collections.ChainMap
On 1/28/24 20:36, Donald Hunter wrote: > Alessandro Marcolini <alessandromarcolini99@gmail.com> writes: > >> On 1/27/24 18:18, Donald Hunter wrote: >>> Okay, so I think the behaviour we need is to either search current scope >>> or search the outermost scope. My suggestion would be to replace the >>> ChainMap approach with just choosing between current and outermost >>> scope. The unusual case is needing to search the outermost scope so >>> using a prefix e.g. '/' for that would work. >>> >>> We can have 'selector: kind' continue to refer to current scope and then >>> have 'selector: /kind' refer to the outermost scope. >>> >>> If we run into a case that requires something other than current or >>> outermost then we could add e.g. '../kind' so that the scope to search >>> is always explicitly identified. >> Wouldn't add different chars in front of the selctor value be confusing? >> >> IMHO the solution of using a ChainMap with levels could be an easier solution. We could just >> modify the __getitem__() method to output both the value and the level, and the get() method to >> add the chance to specify a level (in our case the level found in the spec) and error out if the >> specified level doesn't match with the found one. Something like this: > If we take the approach of resolving the level from the spec then I > wouldn't use ChainMap. Per the Python docs [1]: "A ChainMap class is > provided for quickly linking a number of mappings so they can be treated > as a single unit." > > I think we could instead pass a list of mappings from current to > outermost and then just reference the correct level that was resolved > from the spec. Yes, you're right. There is no need to use a ChainMap at all. The implementation I proposed is in fact a list of mappings with unnecessary complications. >> from collections import ChainMap >> >> class LevelChainMap(ChainMap): >> def __getitem__(self, key): >> for mapping in self.maps: >> try: >> return mapping[key], self.maps[::-1].index(mapping) >> except KeyError: >> pass >> return self.__missing__(key) >> >> def get(self, key, default=None, level=None): >> val, lvl = self[key] if key in self else (default, None) >> if level: >> if lvl != level: >> raise Exception("Level mismatch") >> return val, lvl >> >> # example usage >> c = LevelChainMap({'a':1}, {'inner':{'a':1}}, {'outer': {'inner':{'a':1}}}) >> print(c.get('a', level=2)) >> print(c.get('a', level=1)) #raise err >> >> This will leave the spec as it is and will require small changes. >> >> What do you think? > The more I think about it, the more I agree that using path-like syntax > in the selector is overkill. It makes sense to resolve the selector > level from the spec and then directly access the mappings from the > correct scope level. > > [1] https://docs.python.org/3/library/collections.html#collections.ChainMap Agreed.
On Sun, 28 Jan 2024 19:36:29 +0000 Donald Hunter wrote: > > from collections import ChainMap > > > > class LevelChainMap(ChainMap): > > def __getitem__(self, key): > > for mapping in self.maps: > > try: > > return mapping[key], self.maps[::-1].index(mapping) > > except KeyError: > > pass > > return self.__missing__(key) > > > > def get(self, key, default=None, level=None): > > val, lvl = self[key] if key in self else (default, None) > > if level: > > if lvl != level: > > raise Exception("Level mismatch") > > return val, lvl > > > > # example usage > > c = LevelChainMap({'a':1}, {'inner':{'a':1}}, {'outer': {'inner':{'a':1}}}) > > print(c.get('a', level=2)) > > print(c.get('a', level=1)) #raise err > > > > This will leave the spec as it is and will require small changes. > > > > What do you think? > > The more I think about it, the more I agree that using path-like syntax > in the selector is overkill. It makes sense to resolve the selector > level from the spec and then directly access the mappings from the > correct scope level. Plus if we resolve from the spec that's easily reusable in C / C++ code gen :)
On Sat, 27 Jan 2024 17:18:59 +0000 Donald Hunter wrote: > > Hah, required attrs. I have been sitting on patches for the kernel for > > over a year - https://github.com/kuba-moo/linux/tree/req-args > > Not sure if they actually work but for the kernel I was curious if it's > > possible to do the validation in constant time (in relation to the > > policy size, i.e. without scanning the entire policy at the end to > > confirm that all required attrs are present). And that's what I came up > > with. > > Interesting. It's definitely a thorny problem with varying sets of > 'required' attributes. It could be useful to report the absolutely > required attributes in policy responses, without any actual enforcement. > Would it be possible to report policy for legacy netlink-raw families? It's a simple matter of plumbing. We care reuse the genetlink policy dumping, just need to add a new attr to make "classic" family IDs distinct from genetlink ones. The policy vs spec is another interesting question. When I started thinking about YNL my intuition was to extend policies to carry all relevant info. But the more I thought about it the less sense it made. Whether YNL specs should replace policy dumps completely (by building the YAML into the kernel, and exposing via sysfs like kheaders or btf) - I'm not sure. I think I used policy dumps twice in my life. They are not all that useful, IMVHO... > Thinking about it, usability would probably be most improved by adding > extack messages to more of the tc error paths. TC was one of the first netlink families, so we shouldn't judge it too harshly. With that preface - it should only be used as "lessons learned" not to inform modern designs.
Jakub Kicinski <kuba@kernel.org> writes: > On Sat, 27 Jan 2024 17:18:59 +0000 Donald Hunter wrote: >> > Hah, required attrs. I have been sitting on patches for the kernel for >> > over a year - https://github.com/kuba-moo/linux/tree/req-args >> > Not sure if they actually work but for the kernel I was curious if it's >> > possible to do the validation in constant time (in relation to the >> > policy size, i.e. without scanning the entire policy at the end to >> > confirm that all required attrs are present). And that's what I came up >> > with. >> >> Interesting. It's definitely a thorny problem with varying sets of >> 'required' attributes. It could be useful to report the absolutely >> required attributes in policy responses, without any actual enforcement. >> Would it be possible to report policy for legacy netlink-raw families? > > It's a simple matter of plumbing. We care reuse the genetlink policy > dumping, just need to add a new attr to make "classic" family IDs > distinct from genetlink ones. > > The policy vs spec is another interesting question. When I started > thinking about YNL my intuition was to extend policies to carry all > relevant info. But the more I thought about it the less sense it made. > > Whether YNL specs should replace policy dumps completely (by building > the YAML into the kernel, and exposing via sysfs like kheaders or btf) > - I'm not sure. I think I used policy dumps twice in my life. They > are not all that useful, IMVHO... Yeah, fair point. I don't think I've used policy dumps in any meaningful way either. Maybe no real value in exporting it for netlink-raw. >> Thinking about it, usability would probably be most improved by adding >> extack messages to more of the tc error paths. > > TC was one of the first netlink families, so we shouldn't judge it too > harshly. With that preface - it should only be used as "lessons learned" > not to inform modern designs. Oh, not judging TC, just considering whether it would be useful to throw some extack patches at it.
On 1/29/2024 5:42 PM, Jakub Kicinski wrote: > Whether YNL specs should replace policy dumps completely (by building > the YAML into the kernel, and exposing via sysfs like kheaders or btf) > - I'm not sure. I think I used policy dumps twice in my life. They > are not all that useful, IMVHO... Many older genetlink/netlink families don't have a super robust or specific policy. For example, devlink has a single enum for all attributes, and the policy is not specified per command. The policy simply accepts all attributes for every command. This means that you can't rely on policy to decide whether an attribute has meaning for a given command. Unfortunately, we can't really change this because it ultimately counts as uAPI and we require that existing working functionality continues working in the future. I personally find this too stringent as sending such junk attributes requires someone going out of their way to write the messages and add extra attributes. In most cases I think sane users/software would rather be informed that they are sending data which is not relevant. However, I can understand the point that the userspace software "worked", and we don't want to break existing applications just because of a kernel upgrade. The YNL spec does this by telling you at every layer of nesting which set of attributes are allowed and with what values. Even if we can't enforce this for older families its still useful information to report in some manner. In addition, the YNL spec is more readable than the policy dumps which essentially require a separate tool to parse out everything and convert to something useful.
On Thu, 1 Feb 2024 12:53:08 -0800 Jacob Keller wrote: > On 1/29/2024 5:42 PM, Jakub Kicinski wrote: > > Whether YNL specs should replace policy dumps completely (by building > > the YAML into the kernel, and exposing via sysfs like kheaders or btf) > > - I'm not sure. I think I used policy dumps twice in my life. They > > are not all that useful, IMVHO... > > Many older genetlink/netlink families don't have a super robust or > specific policy. For example, devlink has a single enum for all > attributes, and the policy is not specified per command. The policy > simply accepts all attributes for every command. This means that you > can't rely on policy to decide whether an attribute has meaning for a > given command. FWIW Jiri converted devlink to use ynl policy generation. AFAIU it now only accepts what's used and nobody complained, yet, knock wood. Agreed on other points :)
On 2/1/2024 4:04 PM, Jakub Kicinski wrote: > On Thu, 1 Feb 2024 12:53:08 -0800 Jacob Keller wrote: >> On 1/29/2024 5:42 PM, Jakub Kicinski wrote: >>> Whether YNL specs should replace policy dumps completely (by building >>> the YAML into the kernel, and exposing via sysfs like kheaders or btf) >>> - I'm not sure. I think I used policy dumps twice in my life. They >>> are not all that useful, IMVHO... >> >> Many older genetlink/netlink families don't have a super robust or >> specific policy. For example, devlink has a single enum for all >> attributes, and the policy is not specified per command. The policy >> simply accepts all attributes for every command. This means that you >> can't rely on policy to decide whether an attribute has meaning for a >> given command. > > FWIW Jiri converted devlink to use ynl policy generation. AFAIU it now > only accepts what's used and nobody complained, yet, knock wood. > Oh, I guess I missed that. That's awesome. > Agreed on other points :) >
diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py index 1e10512b2117..b00cde5d29e5 100644 --- a/tools/net/ynl/lib/ynl.py +++ b/tools/net/ynl/lib/ynl.py @@ -1,6 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause -from collections import namedtuple +from collections import namedtuple, ChainMap import functools import os import random @@ -564,8 +564,8 @@ class YnlFamily(SpecFamily): spec = sub_msg_spec.formats[value] return spec - def _decode_sub_msg(self, attr, attr_spec, rsp): - msg_format = self._resolve_selector(attr_spec, rsp) + def _decode_sub_msg(self, attr, attr_spec, vals): + msg_format = self._resolve_selector(attr_spec, vals) decoded = {} offset = 0 if msg_format.fixed_header: @@ -579,10 +579,11 @@ class YnlFamily(SpecFamily): raise Exception(f"Unknown attribute-set '{attr_space}' when decoding '{attr_spec.name}'") return decoded - def _decode(self, attrs, space): + def _decode(self, attrs, space, outer_vals = ChainMap()): if space: attr_space = self.attr_sets[space] rsp = dict() + vals = ChainMap(rsp, outer_vals) for attr in attrs: try: attr_spec = attr_space.attrs_by_val[attr.type] @@ -594,7 +595,7 @@ class YnlFamily(SpecFamily): continue if attr_spec["type"] == 'nest': - subdict = self._decode(NlAttrs(attr.raw), attr_spec['nested-attributes']) + subdict = self._decode(NlAttrs(attr.raw), attr_spec['nested-attributes'], vals) decoded = subdict elif attr_spec["type"] == 'string': decoded = attr.as_strz() @@ -617,7 +618,7 @@ class YnlFamily(SpecFamily): selector = self._decode_enum(selector, attr_spec) decoded = {"value": value, "selector": selector} elif attr_spec["type"] == 'sub-message': - decoded = self._decode_sub_msg(attr, attr_spec, rsp) + decoded = self._decode_sub_msg(attr, attr_spec, vals) else: if not self.process_unknown: raise Exception(f'Unknown {attr_spec["type"]} with name {attr_spec["name"]}')
Sub-message selectors could only be resolved using values from the current nest level. Enable value lookup in outer scopes by using collections.ChainMap to implement an ordered lookup from nested to outer scopes. Signed-off-by: Donald Hunter <donald.hunter@gmail.com> --- tools/net/ynl/lib/ynl.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)