| Message ID | 20241203130655.45293-1-donald.hunter@gmail.com |
|---|---|
| Headers | show |
| Series | netlink: specs: add a spec for nl80211 wiphy | expand |
On Tue, 3 Dec 2024 13:06:53 +0000 Donald Hunter wrote: > Nested structs are already supported in netlink-raw. Add the same > capability to the genetlink legacy schema. Acked-by: Jakub Kicinski <kuba@kernel.org>
On Tue, 3 Dec 2024 at 13:12, Johannes Berg <johannes@sipsolutions.net> wrote: > > On Tue, 2024-12-03 at 13:06 +0000, Donald Hunter wrote: > > Add a rudimentary YNL spec for nl80211 that covers get-wiphy and > > get-interface. > > OK, that says what it's doing, but why? My main motivation is coverage, for 2 reasons: firstly to flush out any feature gaps in YNL such as the ones I fixed in this series and secondly to achieve a critical mass of YNL specs that encourages people to build more tooling around the specs. YNL is already used for in-tree test automation and documentation generation. There is potential for generating strace dumpers and people are starting to use generated user space code. > Also, I don't know how we will maintain this if it's not tied to any > kernel code. What do you suggest? Do you want to just maintain it > following the nl80211.h spec all the time? It's a good question. I am okay with maintaining it alongside the nl80211.h file, which will likely motivate me to write some automation at least for notifying any divergence. There might come a time when it becomes desirable to generate some of nl80211.h from the spec, as Stanislav Fomichev is doing for ethtool here: https://lore.kernel.org/netdev/20241202162936.3778016-1-sdf@fomichev.me/ > > + name: get-wiphy > > + doc: Get information about a wiphy or dump a list of all wiphys > > + attribute-set: nl80211-attrs > > + do: > > + request: > > + value: 1 > > + attributes: > > + - wiphy > > + reply: > > + value: 3 > > + dump: > > + request: > > + attributes: > > + - wiphy > > > > This already seems wrong - dump wiphy really should unconditionally > include NL80211_ATTR_SPLIT_WIPHY_DUMP these days. Yes, the valid parameter attributes should be wiphy, wdev, ifindex and split-wiphy-dump by the look of it. Thanks, Donald.
On Wed, 4 Dec 2024 at 02:03, Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 3 Dec 2024 13:06:51 +0000 Donald Hunter wrote: > > Add support for translating arrays of scalars into enum names. > > But not formatting hints.. ? ;) Oooh, good catch. This does suggest that a refactor is needed for scalar handling. > > Signed-off-by: Donald Hunter <donald.hunter@gmail.com> > > --- > > tools/net/ynl/lib/ynl.py | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py > > index 0d39a83574d5..f07a8404f71a 100644 > > --- a/tools/net/ynl/lib/ynl.py > > +++ b/tools/net/ynl/lib/ynl.py > > @@ -627,6 +627,8 @@ class YnlFamily(SpecFamily): > > decoded = self._decode_struct(attr.raw, attr_spec.struct_name) > > elif attr_spec.sub_type: > > decoded = attr.as_c_array(attr_spec.sub_type) > > + if 'enum' in attr_spec: > > + decoded = [ self._decode_enum(x, attr_spec) for x in decoded] > > nit: missing space after 'decoded' or extra space before self, with > that fixed: ack. > Acked-by: Jakub Kicinski <kuba@kernel.org>
On Wed, 2024-12-04 at 13:12 +0000, Donald Hunter wrote: > My main motivation is coverage, for 2 reasons: firstly to flush out > any feature gaps in YNL such as the ones I fixed in this series Yeah, OK, though I'm not sure YNL is really meant to be feature complete for everything netlink may be doing, rather than for what's needed - and some of the things we did may even be things that are not meant to be done any more (e.g. nested array vs. multi-attr arrays.) > and > secondly to achieve a critical mass of YNL specs that encourages > people to build more tooling around the specs. OK, fair :) > YNL is already used for > in-tree test automation and documentation generation. There is > potential for generating strace dumpers and people are starting to use > generated user space code. Right. > > Also, I don't know how we will maintain this if it's not tied to any > > kernel code. What do you suggest? Do you want to just maintain it > > following the nl80211.h spec all the time? > > It's a good question. I am okay with maintaining it alongside the > nl80211.h file, which will likely motivate me to write some automation > at least for notifying any divergence. There might come a time when it > becomes desirable to generate some of nl80211.h from the spec, as > Stanislav Fomichev is doing for ethtool here: > > https://lore.kernel.org/netdev/20241202162936.3778016-1-sdf@fomichev.me/ I think I wouldn't mind that - I'm hoping it'll also generate policies etc.? Though on that front we probably have weird quirks too ... But until then I guess someone's going to have to maintain it, and I'm not sure I want that to be me right now :) > > > + name: get-wiphy > > > + doc: Get information about a wiphy or dump a list of all wiphys > > > + attribute-set: nl80211-attrs > > > + do: > > > + request: > > > + value: 1 > > > + attributes: > > > + - wiphy > > > + reply: > > > + value: 3 > > > + dump: > > > + request: > > > + attributes: > > > + - wiphy > > > > > > > This already seems wrong - dump wiphy really should unconditionally > > include NL80211_ATTR_SPLIT_WIPHY_DUMP these days. > > Yes, the valid parameter attributes should be wiphy, wdev, ifindex and > split-wiphy-dump by the look of it. Well there's that about valid parameters, but also no (new) tools today should ever *not* include the split-wiphy-dump attribute. I guess that can't be expressed here, but it's a gotcha for implementers that just follow the YNL spec? johannes
On Wed, 4 Dec 2024 at 13:28, Johannes Berg <johannes@sipsolutions.net> wrote: > > > > Also, I don't know how we will maintain this if it's not tied to any > > > kernel code. What do you suggest? Do you want to just maintain it > > > following the nl80211.h spec all the time? > > > > It's a good question. I am okay with maintaining it alongside the > > nl80211.h file, which will likely motivate me to write some automation > > at least for notifying any divergence. There might come a time when it > > becomes desirable to generate some of nl80211.h from the spec, as > > Stanislav Fomichev is doing for ethtool here: > > > > https://lore.kernel.org/netdev/20241202162936.3778016-1-sdf@fomichev.me/ > > I think I wouldn't mind that - I'm hoping it'll also generate policies > etc.? Though on that front we probably have weird quirks too ... Yes, the policies are generated, quirks notwithstanding ;-) > But until then I guess someone's going to have to maintain it, and I'm > not sure I want that to be me right now :) Ack that. The burden is on me. > > > > + name: get-wiphy > > > > + doc: Get information about a wiphy or dump a list of all wiphys > > > > + attribute-set: nl80211-attrs > > > > + do: > > > > + request: > > > > + value: 1 > > > > + attributes: > > > > + - wiphy > > > > + reply: > > > > + value: 3 > > > > + dump: > > > > + request: > > > > + attributes: > > > > + - wiphy > > > > > > > > > > This already seems wrong - dump wiphy really should unconditionally > > > include NL80211_ATTR_SPLIT_WIPHY_DUMP these days. > > > > Yes, the valid parameter attributes should be wiphy, wdev, ifindex and > > split-wiphy-dump by the look of it. > > Well there's that about valid parameters, but also no (new) tools today > should ever *not* include the split-wiphy-dump attribute. I guess that > can't be expressed here, but it's a gotcha for implementers that just > follow the YNL spec? There's no way to specify that, but the constraint can be described clearly in the doc string. I'll do that for v2. Thanks, Donald.