| Message ID | 20230929134742.1292632-4-jiri@resnulli.us (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tools: ynl-gen: fix subset attributes handling | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| 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 | success | CCed 6 of 6 maintainers |
| 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, 8 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Fri, 29 Sep 2023 15:47:42 +0200 Jiri Pirko wrote: > From: Jiri Pirko <jiri@nvidia.com> > > The only key used in the elem dictionary is "name" to lookup the real > attribute of a set. Raise exception in case there are other keys > present. Mm, there are definitely other things that can be set. I'm not fully sold that type can't change but even if - checks can easily be adjusted or nested-attributes, based on the parsing path.
Thu, Oct 05, 2023 at 02:13:50AM CEST, kuba@kernel.org wrote: >On Fri, 29 Sep 2023 15:47:42 +0200 Jiri Pirko wrote: >> From: Jiri Pirko <jiri@nvidia.com> >> >> The only key used in the elem dictionary is "name" to lookup the real >> attribute of a set. Raise exception in case there are other keys >> present. > >Mm, there are definitely other things that can be set. I'm not fully Which ones? The name is used, the rest is ignored in the existing code. I just make this obvious to the user. If future show other keys are needed here, the patch adding that would just adjust the exception condition. Do you see any problem in that? >sold that type can't change but even if - checks can easily be adjusted >or nested-attributes, based on the parsing path.
On Thu, 5 Oct 2023 09:25:55 +0200 Jiri Pirko wrote: > >> The only key used in the elem dictionary is "name" to lookup the real > >> attribute of a set. Raise exception in case there are other keys > >> present. > > > >Mm, there are definitely other things that can be set. I'm not fully > > Which ones? The name is used, the rest is ignored in the existing code. > I just make this obvious to the user. If future show other keys are > needed here, the patch adding that would just adjust the exception > condition. Do you see any problem in that? Just don't want to give people the impression that this is what's intended, rather than it was simply not implemented yet. If you want to keep the exception please update the message (and the if, no outer brackets necessary in Python ;)).
Thu, Oct 05, 2023 at 04:21:51PM CEST, kuba@kernel.org wrote: >On Thu, 5 Oct 2023 09:25:55 +0200 Jiri Pirko wrote: >> >> The only key used in the elem dictionary is "name" to lookup the real >> >> attribute of a set. Raise exception in case there are other keys >> >> present. >> > >> >Mm, there are definitely other things that can be set. I'm not fully >> >> Which ones? The name is used, the rest is ignored in the existing code. >> I just make this obvious to the user. If future show other keys are >> needed here, the patch adding that would just adjust the exception >> condition. Do you see any problem in that? > >Just don't want to give people the impression that this is what's >intended, rather than it was simply not implemented yet. >If you want to keep the exception please update the message >(and the if, no outer brackets necessary in Python ;)). I don't mind dropping the patch entirely. I just thought it would be nice to do some sanitization so the user is not surprised that other possible keys are ignored. I tried and I was :)
diff --git a/tools/net/ynl/lib/nlspec.py b/tools/net/ynl/lib/nlspec.py index 37bcb4d8b37b..12e15ac70309 100644 --- a/tools/net/ynl/lib/nlspec.py +++ b/tools/net/ynl/lib/nlspec.py @@ -208,6 +208,8 @@ class SpecAttrSet(SpecElement): attr = real_set[elem['name']] self.attrs[attr.name] = attr self.attrs_by_val[attr.value] = attr + if (len(elem.keys()) > 1): + raise Exception(f"Subset attribute '{elem['name']}' contains other keys") def new_attr(self, elem, value): return SpecAttr(self.family, self, elem, value)