| Message ID | 20240301171431.65892-4-donald.hunter@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tools/net/ynl: Add support for nlctrl netlink family | expand |
On Fri, 1 Mar 2024 17:14:30 +0000 Donald Hunter wrote: > The nlctrl family uses 2 levels of array nesting for policy attributes. > Add a 'nest-depth' property to genetlink-legacy and extend ynl to use > it. Hm, I'm 90% sure we don't need this... because nlctrl is basically what the legacy level was written for, initially. The spec itself wasn't sent, because the C codegen for it was quite painful. And the Python CLI was an afterthought. Could you describe what nesting you're trying to cover here? Isn't it a type-value? BTW we'll also need to deal with the C codegen situation somehow. Try making it work, if it's not a simple matter of fixing up the names to match the header - we can grep nlctrl out in the Makefile.
On Sun, 3 Mar 2024 at 04:05, Jakub Kicinski <kuba@kernel.org> wrote: > > On Fri, 1 Mar 2024 17:14:30 +0000 Donald Hunter wrote: > > The nlctrl family uses 2 levels of array nesting for policy attributes. > > Add a 'nest-depth' property to genetlink-legacy and extend ynl to use > > it. > > Hm, I'm 90% sure we don't need this... because nlctrl is basically what > the legacy level was written for, initially. The spec itself wasn't > sent, because the C codegen for it was quite painful. And the Python > CLI was an afterthought. > > Could you describe what nesting you're trying to cover here? > Isn't it a type-value? I added it for getpolicy which is indexed by policy_idx and attr_idx. ./tools/net/ynl/cli.py \ --spec Documentation/netlink/specs/nlctrl.yaml \ --dump getpolicy --json '{"family-name": "nlctrl"}' [{'family-id': 16, 'op-policy': [{3: {'do': 0, 'dump': 0}}]}, {'family-id': 16, 'op-policy': [{0: {'dump': 1}}]}, {'family-id': 16, 'policy': [{0: [{1: {'max-value-u': 65535, 'min-value-u': 0, 'type': 'u16'}}]}]}, {'family-id': 16, 'policy': [{0: [{2: {'max-length': 15, 'type': 'nul-string'}}]}]}, {'family-id': 16, 'policy': [{1: [{1: {'max-value-u': 65535, 'min-value-u': 0, 'type': 'u16'}}]}]}, {'family-id': 16, 'policy': [{1: [{2: {'max-length': 15, 'type': 'nul-string'}}]}]}, {'family-id': 16, 'policy': [{1: [{10: {'max-value-u': 4294967295, 'min-value-u': 0, 'type': 'u32'}}]}]}] > BTW we'll also need to deal with the C codegen situation somehow. > Try making it work, if it's not a simple matter of fixing up the > names to match the header - we can grep nlctrl out in the Makefile. Yeah, I forgot to check codegen but saw the failures on patchwork. I have fixed the names but still have a couple more things to fix. BTW, this patchset was a step towards experimenting with removing the hard-coded msg decoding in the Python library. Not so much for genetlink families, more for the extack decoding so that I could add policy attr decoding. Thinking about it some more, that might be better done with a "core" spec that contains just extack-attrs and policy-attrs because they don't belong to any single family - they're kinda infrastructure for all families.
On Sun, 3 Mar 2024 10:50:09 +0000 Donald Hunter wrote: > On Sun, 3 Mar 2024 at 04:05, Jakub Kicinski <kuba@kernel.org> wrote: > > On Fri, 1 Mar 2024 17:14:30 +0000 Donald Hunter wrote: > > > The nlctrl family uses 2 levels of array nesting for policy attributes. > > > Add a 'nest-depth' property to genetlink-legacy and extend ynl to use > > > it. > > > > Hm, I'm 90% sure we don't need this... because nlctrl is basically what > > the legacy level was written for, initially. The spec itself wasn't > > sent, because the C codegen for it was quite painful. And the Python > > CLI was an afterthought. > > > > Could you describe what nesting you're trying to cover here? > > Isn't it a type-value? > > I added it for getpolicy which is indexed by policy_idx and attr_idx. > > ./tools/net/ynl/cli.py \ > --spec Documentation/netlink/specs/nlctrl.yaml \ > --dump getpolicy --json '{"family-name": "nlctrl"}' > [{'family-id': 16, 'op-policy': [{3: {'do': 0, 'dump': 0}}]}, > {'family-id': 16, 'op-policy': [{0: {'dump': 1}}]}, > {'family-id': 16, > 'policy': [{0: [{1: {'max-value-u': 65535, > 'min-value-u': 0, > 'type': 'u16'}}]}]}, > {'family-id': 16, > 'policy': [{0: [{2: {'max-length': 15, 'type': 'nul-string'}}]}]}, > {'family-id': 16, > 'policy': [{1: [{1: {'max-value-u': 65535, > 'min-value-u': 0, > 'type': 'u16'}}]}]}, > {'family-id': 16, > 'policy': [{1: [{2: {'max-length': 15, 'type': 'nul-string'}}]}]}, > {'family-id': 16, > 'policy': [{1: [{10: {'max-value-u': 4294967295, > 'min-value-u': 0, > 'type': 'u32'}}]}]}] Yeah.. look at the example I used for type-value :) https://docs.kernel.org/next/userspace-api/netlink/genetlink-legacy.html#type-value > > BTW we'll also need to deal with the C codegen situation somehow. > > Try making it work, if it's not a simple matter of fixing up the > > names to match the header - we can grep nlctrl out in the Makefile. > > Yeah, I forgot to check codegen but saw the failures on patchwork. I > have fixed the names but still have a couple more things to fix. > > BTW, this patchset was a step towards experimenting with removing the > hard-coded msg decoding in the Python library. Not so much for > genetlink families, more for the extack decoding so that I could add > policy attr decoding. Thinking about it some more, that might be > better done with a "core" spec that contains just extack-attrs and > policy-attrs because they don't belong to any single family - they're > kinda infrastructure for all families. YAML specs describe information on how to parse data YNL doesn't have to understand, just format correctly. The base level of netlink processing, applicable to all families, is a different story. I think hand-coding that is more than okay. The goal is not to express everything in YAML but to avoid duplicated work per family, if that makes sense.
On Mon, 4 Mar 2024 at 15:22, Jakub Kicinski <kuba@kernel.org> wrote: > > Yeah.. look at the example I used for type-value :) > > https://docs.kernel.org/next/userspace-api/netlink/genetlink-legacy.html#type-value Apologies, I totally missed this. Is the intended usage something like this: - name: policy type: nest-type-value type-value: [ policy-id, attr-id ] nested-attributes: policy-attrs > YAML specs describe information on how to parse data YNL doesn't have > to understand, just format correctly. The base level of netlink > processing, applicable to all families, is a different story. > I think hand-coding that is more than okay. The goal is not to express > everything in YAML but to avoid duplicated work per family, if that > makes sense. Okay, I can go ahead and hard-code the policy attr decoding for extack messages.
On Mon, 4 Mar 2024 16:21:11 +0000 Donald Hunter wrote: > > Yeah.. look at the example I used for type-value :) > > > > https://docs.kernel.org/next/userspace-api/netlink/genetlink-legacy.html#type-value > > Apologies, I totally missed this. Is the intended usage something like this: > > - > name: policy > type: nest-type-value > type-value: [ policy-id, attr-id ] > nested-attributes: policy-attrs Yes, I believe that'd be the right way. And then the policy-id and attr-id are used as keys in the output / response dict.
diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml index 938703088306..872c76065f1b 100644 --- a/Documentation/netlink/genetlink-legacy.yaml +++ b/Documentation/netlink/genetlink-legacy.yaml @@ -261,6 +261,9 @@ properties: struct: description: Name of the struct type used for the attribute. type: string + nest-depth: + description: Depth of nesting for an array-nest, defaults to 1. + type: integer # End genetlink-legacy # Make sure name-prefix does not appear in subsets (subsets inherit naming) diff --git a/tools/net/ynl/lib/nlspec.py b/tools/net/ynl/lib/nlspec.py index fbce52395b3b..50e8447f089a 100644 --- a/tools/net/ynl/lib/nlspec.py +++ b/tools/net/ynl/lib/nlspec.py @@ -161,6 +161,7 @@ class SpecAttr(SpecElement): sub_message string, name of sub message type selector string, name of attribute used to select sub-message type + nest_depth integer, depth of array nesting is_auto_scalar bool, attr is a variable-size scalar """ @@ -178,6 +179,7 @@ class SpecAttr(SpecElement): self.display_hint = yaml.get('display-hint') self.sub_message = yaml.get('sub-message') self.selector = yaml.get('selector') + self.nest_depth = yaml.get('nest-depth', 1) self.is_auto_scalar = self.type == "sint" or self.type == "uint" diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py index 29262505a3f2..efceea9433f2 100644 --- a/tools/net/ynl/lib/ynl.py +++ b/tools/net/ynl/lib/ynl.py @@ -556,14 +556,17 @@ class YnlFamily(SpecFamily): decoded = self._formatted_string(decoded, attr_spec.display_hint) return decoded - def _decode_array_nest(self, attr, attr_spec): + def _decode_array_nest(self, attr, attr_spec, depth): decoded = [] offset = 0 while offset < len(attr.raw): item = NlAttr(attr.raw, offset) offset += item.full_len - subattrs = self._decode(NlAttrs(item.raw), attr_spec['nested-attributes']) + if depth > 1: + subattrs = self._decode_array_nest(item, attr_spec, depth - 1) + else: + subattrs = self._decode(NlAttrs(item.raw), attr_spec['nested-attributes']) decoded.append({ item.type: subattrs }) return decoded @@ -649,7 +652,7 @@ class YnlFamily(SpecFamily): if 'enum' in attr_spec: decoded = self._decode_enum(decoded, attr_spec) elif attr_spec["type"] == 'array-nest': - decoded = self._decode_array_nest(attr, attr_spec) + decoded = self._decode_array_nest(attr, attr_spec, attr_spec.nest_depth) elif attr_spec["type"] == 'bitfield32': value, selector = struct.unpack("II", attr.raw) if 'enum' in attr_spec:
The nlctrl family uses 2 levels of array nesting for policy attributes. Add a 'nest-depth' property to genetlink-legacy and extend ynl to use it. Signed-off-by: Donald Hunter <donald.hunter@gmail.com> --- Documentation/netlink/genetlink-legacy.yaml | 3 +++ tools/net/ynl/lib/nlspec.py | 2 ++ tools/net/ynl/lib/ynl.py | 9 ++++++--- 3 files changed, 11 insertions(+), 3 deletions(-)