| Message ID | 20240219172525.71406-7-jiri@resnulli.us (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netlink: specs: devlink: add the rest of missing attribute definitions | expand |
On Mon, 19 Feb 2024 18:25:22 +0100 Jiri Pirko wrote: > For devlink param, param-value-data attr is used by kernel to fill > different attribute type according to param-type attribute value. > > Currently the sub-message feature allows spec to embed custom message > selected by another attribute. The sub-message is then nested inside the > attr of sub-message type. > > Benefit from the sub-message feature and extend it. Introduce > attribute-replace spec flag by which the spec indicates that ynl should > consider sub-message as not nested in the original attribute, but rather > to consider the original attribute as the sub-message right away. The "type agnostic" / generic style of devlink params and fmsg are contrary to YNL's direction and goals. YNL abstracts parsing and typing using external spec. devlink params and fmsg go in a different direction where all information is carried by netlink values and netlink typing is just to create "JSON-like" formatting. These are incompatible ideas, and combining these two abstractions in one library provides little value - devlink CLI already has an implementation for fmsg and params. YNL doesn't have to cover everything.
Mon, Feb 19, 2024 at 11:52:04PM CET, kuba@kernel.org wrote: >On Mon, 19 Feb 2024 18:25:22 +0100 Jiri Pirko wrote: >> For devlink param, param-value-data attr is used by kernel to fill >> different attribute type according to param-type attribute value. >> >> Currently the sub-message feature allows spec to embed custom message >> selected by another attribute. The sub-message is then nested inside the >> attr of sub-message type. >> >> Benefit from the sub-message feature and extend it. Introduce >> attribute-replace spec flag by which the spec indicates that ynl should >> consider sub-message as not nested in the original attribute, but rather >> to consider the original attribute as the sub-message right away. > >The "type agnostic" / generic style of devlink params and fmsg >are contrary to YNL's direction and goals. YNL abstracts parsing True, but that's what we have. Similar to what we have in TC where sub-messages are present, that is also against ynl's direction and goals. >and typing using external spec. devlink params and fmsg go in a >different direction where all information is carried by netlink values >and netlink typing is just to create "JSON-like" formatting. Only fmsg, not params. >These are incompatible ideas, and combining these two abstractions >in one library provides little value - devlink CLI already has an >implementation for fmsg and params. YNL doesn't have to cover >everything. True. In this patchset, I'm just using the existing sub-message infrastructure with a bit of extension. The json-like thing of fmsg is ignored, I don't try to reconstruct json from netlink message of fmsg.
On Tue, 20 Feb 2024 08:31:23 +0100 Jiri Pirko wrote: > >The "type agnostic" / generic style of devlink params and fmsg > >are contrary to YNL's direction and goals. YNL abstracts parsing > > True, but that's what we have. Similar to what we have in TC where > sub-messages are present, that is also against ynl's direction and > goals. But TC and ip-link are raw netlink, meaning genetlink-legacy remains fairly straightforward. BTW since we currently have full parity in C code gen adding this series will break build for tools/net/ynl. Plus ip-link is a really high value target. I had been pondering how to solve it myself. There's probably a hundred different implementations out there of container management systems which spawn veths using odd hacks because "netlink is scary". Once I find the time to finish rtnetlink codegen we can replace all the unholy libbpf netlink hacks with ynl, too. So at this stage I'd really like to focus YNL on language coverage (adding more codegens), packaging and usability polish, not extending the spec definitions to cover not-so-often used corner cases. Especially those which will barely benefit because they are in themselves built to be an abstraction.
Wed, Feb 21, 2024 at 03:10:04AM CET, kuba@kernel.org wrote: >On Tue, 20 Feb 2024 08:31:23 +0100 Jiri Pirko wrote: >> >The "type agnostic" / generic style of devlink params and fmsg >> >are contrary to YNL's direction and goals. YNL abstracts parsing >> >> True, but that's what we have. Similar to what we have in TC where >> sub-messages are present, that is also against ynl's direction and >> goals. > >But TC and ip-link are raw netlink, meaning genetlink-legacy remains >fairly straightforward. BTW since we currently have full parity in C >code gen adding this series will break build for tools/net/ynl. > >Plus ip-link is a really high value target. I had been pondering how >to solve it myself. There's probably a hundred different implementations >out there of container management systems which spawn veths using odd >hacks because "netlink is scary". Once I find the time to finish >rtnetlink codegen we can replace all the unholy libbpf netlink hacks >with ynl, too. > >So at this stage I'd really like to focus YNL on language coverage >(adding more codegens), packaging and usability polish, not extending >the spec definitions to cover not-so-often used corner cases. >Especially those which will barely benefit because they are in >themselves built to be an abstraction. That leaves devlink.yaml incomplete, which I'm not happy about. It is a legacy, it should be covered by genetlink-legacy I believe. To undestand you correctly, should I wait until codegen for raw netlink is done and then to rebase-repost this? Or do you say this will never be acceptable?
On Wed, 21 Feb 2024 13:48:13 +0100 Jiri Pirko wrote: > >But TC and ip-link are raw netlink, meaning genetlink-legacy remains > >fairly straightforward. BTW since we currently have full parity in C > >code gen adding this series will break build for tools/net/ynl. > > > >Plus ip-link is a really high value target. I had been pondering how > >to solve it myself. There's probably a hundred different implementations > >out there of container management systems which spawn veths using odd > >hacks because "netlink is scary". Once I find the time to finish > >rtnetlink codegen we can replace all the unholy libbpf netlink hacks > >with ynl, too. > > > >So at this stage I'd really like to focus YNL on language coverage > >(adding more codegens), packaging and usability polish, not extending > >the spec definitions to cover not-so-often used corner cases. > >Especially those which will barely benefit because they are in > >themselves built to be an abstraction. > > That leaves devlink.yaml incomplete, which I'm not happy about. It is a > legacy, it should be covered by genetlink-legacy I believe. > > To undestand you correctly, should I wait until codegen for raw netlink > is done and then to rebase-repost this? Or do you say this will never be > acceptable? It'd definitely not acceptable before the rtnetlink C codegen is complete, and at least two other code gens for genetlink-legacy. At that point we can reconsider complicating the schema further.
Wed, Feb 21, 2024 at 07:45:05PM CET, kuba@kernel.org wrote: >On Wed, 21 Feb 2024 13:48:13 +0100 Jiri Pirko wrote: >> >But TC and ip-link are raw netlink, meaning genetlink-legacy remains >> >fairly straightforward. BTW since we currently have full parity in C >> >code gen adding this series will break build for tools/net/ynl. >> > >> >Plus ip-link is a really high value target. I had been pondering how >> >to solve it myself. There's probably a hundred different implementations >> >out there of container management systems which spawn veths using odd >> >hacks because "netlink is scary". Once I find the time to finish >> >rtnetlink codegen we can replace all the unholy libbpf netlink hacks >> >with ynl, too. >> > >> >So at this stage I'd really like to focus YNL on language coverage >> >(adding more codegens), packaging and usability polish, not extending >> >the spec definitions to cover not-so-often used corner cases. >> >Especially those which will barely benefit because they are in >> >themselves built to be an abstraction. >> >> That leaves devlink.yaml incomplete, which I'm not happy about. It is a >> legacy, it should be covered by genetlink-legacy I believe. >> >> To undestand you correctly, should I wait until codegen for raw netlink >> is done and then to rebase-repost this? Or do you say this will never be >> acceptable? > >It'd definitely not acceptable before the rtnetlink C codegen is >complete, and at least two other code gens for genetlink-legacy. >At that point we can reconsider complicating the schema further. Okay, will keep it in the cupboard for now. I would definitelly love to get the devlink.yaml complete. Next step is to generate the uapi header from it replacing the existing one.
diff --git a/Documentation/netlink/genetlink-legacy.yaml b/Documentation/netlink/genetlink-legacy.yaml index 6cb50e2cc021..77d89f81c71f 100644 --- a/Documentation/netlink/genetlink-legacy.yaml +++ b/Documentation/netlink/genetlink-legacy.yaml @@ -328,6 +328,10 @@ properties: Name of the attribute space from which to resolve attributes in the sub message. type: string + attribute-replace: + description: | + Replace the parent nested attribute with attribute set + type: boolean operations: description: Operations supported by the protocol. diff --git a/Documentation/netlink/netlink-raw.yaml b/Documentation/netlink/netlink-raw.yaml index cc38b026c451..e32660fbe6c3 100644 --- a/Documentation/netlink/netlink-raw.yaml +++ b/Documentation/netlink/netlink-raw.yaml @@ -346,6 +346,10 @@ properties: Name of the attribute space from which to resolve attributes in the sub message. type: string + attribute-replace: + description: | + Replace the parent nested attribute with attribute set + type: boolean operations: description: Operations supported by the protocol. diff --git a/Documentation/userspace-api/netlink/genetlink-legacy.rst b/Documentation/userspace-api/netlink/genetlink-legacy.rst index 7126b650090e..a9ccbfbb4a8d 100644 --- a/Documentation/userspace-api/netlink/genetlink-legacy.rst +++ b/Documentation/userspace-api/netlink/genetlink-legacy.rst @@ -381,3 +381,28 @@ alongside a sub-message selector and also in a top level ``attribute-set``, then the selector will be resolved using the value 'closest' to the selector. If the value is not present in the message at the same level as defined in the spec then this is an error. + +Some users, like devlink param, fill different attribute type according to +selector attribute value. ``replace-attribute`` set to ``true`` indicates, +that sub-message is not nested inside the attribute, but rather replacing +the attribute. This allows to treat the attribute type differently according +to the selector: + +.. code-block:: yaml + + sub-messages: + - + name: dl-param-value-data-msg + formats: + - + value: u32 + attribute-set: dl-param-value-data-u32-attrs + attribute-replace: true + - + value: string + attribute-set: dl-param-value-data-string-attrs + attribute-replace: true + - + value: bool + attribute-set: dl-param-value-data-bool-attrs + attribute-replace: true diff --git a/tools/net/ynl/lib/nlspec.py b/tools/net/ynl/lib/nlspec.py index 5e48ee0fb8b4..280d50e9079c 100644 --- a/tools/net/ynl/lib/nlspec.py +++ b/tools/net/ynl/lib/nlspec.py @@ -237,6 +237,9 @@ class SpecAttrSet(SpecElement): def items(self): return self.attrs.items() + def keys(self): + return self.attrs.keys() + class SpecStructMember(SpecElement): """Struct member attribute @@ -318,6 +321,8 @@ class SpecSubMessageFormat(SpecElement): value attribute value to match against type selector fixed_header string, name of fixed header, or None attr_set string, name of attribute set, or None + attr_replace bool, indicates replacement of parent attribute with + attr_set decode, or None """ def __init__(self, family, yaml): super().__init__(family, yaml) @@ -325,6 +330,7 @@ class SpecSubMessageFormat(SpecElement): self.value = yaml.get('value') self.fixed_header = yaml.get('fixed-header') self.attr_set = yaml.get('attribute-set') + self.attr_replace = yaml.get('attribute-replace') class SpecOperation(SpecElement): diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py index d2ea1571d00c..8591e6bfe40b 100644 --- a/tools/net/ynl/lib/ynl.py +++ b/tools/net/ynl/lib/ynl.py @@ -507,11 +507,16 @@ class YnlFamily(SpecFamily): attr_payload += self._encode_struct(msg_format.fixed_header, value) if msg_format.attr_set: if msg_format.attr_set in self.attr_sets: - nl_type |= Netlink.NLA_F_NESTED - sub_attrs = SpaceAttrs(msg_format.attr_set, value, search_attrs) - for subname, subvalue in value.items(): - attr_payload += self._add_attr(msg_format.attr_set, - subname, subvalue, sub_attrs) + if msg_format.attr_replace: + first_attr_name = list(self.attr_sets[msg_format.attr_set].keys())[0] + return self._add_attr(msg_format.attr_set, first_attr_name, + value, search_attrs) + else: + nl_type |= Netlink.NLA_F_NESTED + sub_attrs = SpaceAttrs(msg_format.attr_set, value, search_attrs) + for subname, subvalue in value.items(): + attr_payload += self._add_attr(msg_format.attr_set, + subname, subvalue, sub_attrs) else: raise Exception(f"Unknown attribute-set '{msg_format.attr_set}'") else: @@ -600,8 +605,17 @@ class YnlFamily(SpecFamily): offset = self._struct_size(msg_format.fixed_header) if msg_format.attr_set: if msg_format.attr_set in self.attr_sets: - subdict = self._decode(NlAttrs(attr.raw, offset), msg_format.attr_set) + if msg_format.attr_replace: + attrs = [attr] + else: + attrs = NlAttrs(attr.raw, offset); + subdict = self._decode(attrs, msg_format.attr_set) decoded.update(subdict) + if msg_format.attr_replace: + try: + decoded = decoded[attr_spec.name] + except KeyError: + raise Exception(f"Attribute-set '{attr_space}' does not contain '{attr_spec.name}'") else: raise Exception(f"Unknown attribute-set '{attr_space}' when decoding '{attr_spec.name}'") return decoded