Re: [RFC net-next 4/4] ynl: add a sample user for ethtool
From: <hidden>
Date: 2022-08-11 16:46:56
Also in:
linux-doc
On 08/10, Jakub Kicinski wrote:
A sample schema describing ethtool channels and a script showing that it works. The script is called like this:
./tools/net/ynl/samples/ethtool.py \
--spec Documentation/netlink/bindings/ethtool.yaml \
--device eth0I have schemas for genetlink, FOU and the proposed DPLL subsystem, to validate that the concept has wide applicability, but ethtool feels like the best demo of the 4.
Looks super promising! I'm going to comment mostly here because it's easier to talk about the sample schema definition.
Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- Documentation/netlink/bindings/ethtool.yaml | 115 ++++++++++++++++++++ tools/net/ynl/samples/ethtool.py | 30 +++++ 2 files changed, 145 insertions(+) create mode 100644 Documentation/netlink/bindings/ethtool.yaml create mode 100755 tools/net/ynl/samples/ethtool.py
quoted hunk ↗ jump to hunk
diff --git a/Documentation/netlink/bindings/ethtool.yamlb/Documentation/netlink/bindings/ethtool.yaml new file mode 100644 index 000000000000..b4540d60b4b3--- /dev/null +++ b/Documentation/netlink/bindings/ethtool.yaml@@ -0,0 +1,115 @@ +# SPDX-License-Identifier: BSD-3-Clause + +name: ethtool + +description: | + Ethernet device configuration interface. +
[..]
+attr-cnt-suffix: CNT
Is it a hack to make the generated header fit into existing implementation? Should we #define ETHTOOL_XXX_CNT ETHTOOL_XXX in the implementation instead? (or s/ETHTOOL_XXX_CNT/ETHTOOL_XXX/ the source itself?)
+attribute-spaces:
Are you open to bike shedding? :-) I like how ethtool_netlink.h calls these 'message types'.
+ - + name: header + name-prefix: ETHTOOL_A_HEADER_
Any issue with name-prefix+name-suffix being non-greppable? Have you tried
something like this instead:
- name: ETHTOOL_A_HEADER # this is fake, for ynl reference only
attributes:
- name: ETHTOOL_A_HEADER_DEV_INDEX
val:
type:
- name ETHTOOL_A_HEADER_DEV_NAME
..
It seems a bit easier to map the spec into what it's going to produce.
For example, it took me a while to translate 'channels_get' below into
ETHTOOL_MSG_CHANNELS_GET.
Or is it too much ETHTOOL_A_HEADER_?
+ attributes: + - + name: dev_index + val: 1 + type: u32 + - + name: dev_name + type: nul-string
[..]
+ len: ALTIFNAMSIZ - 1
Not sure how strict you want to be here. ALTIFNAMSIZ is defined somewhere else it seems? (IOW, do we want to have implicit dependencies on external/uapi headers?)
+ - + name: flags + type: u32 + - + name: channels + name-prefix: ETHTOOL_A_CHANNELS_ + attributes: + - + name: header + val: 1 + type: nest + nested-attributes: header + - + name: rx_max + type: u32 + - + name: tx_max + type: u32 + - + name: other_max + type: u32 + - + name: combined_max + type: u32 + - + name: rx_count + type: u32 + - + name: tx_count + type: u32 + - + name: other_count + type: u32 + - + name: combined_count + type: u32 + +headers: + user: linux/if.h + uapi: linux/ethtool_netlink.h + +operations: + name-prefix: ETHTOOL_MSG_ + async-prefix: ETHTOOL_MSG_ + list: + - + name: channels_get + val: 17 + description: Get current and max supported number of channels. + attribute-space: channels + do: + request: + attributes: + - header + reply: &channel_reply + attributes: + - header + - rx_max + - tx_max + - other_max + - combined_max + - rx_count + - tx_count + - other_count + - combined_count + dump: + reply: *channel_reply + + - + name: channels_ntf + description: Notification for device changing its number of channels. + notify: channels_get + mcgrp: monitor + + - + name: channels_set + description: Set number of channels. + attribute-space: channels + do: + request: + attributes:
[..]
+ - header + - rx_count + - tx_count + - other_count + - combined_count
My netlink is super rusty, might be worth mentioning in the spec: these are possible attributes, but are all of them required? You also mention the validation part in the cover letter, do you plan add some of these policy properties to the spec or everything is there already? (I'm assuming we care about the types which we have above and optional/required attribute indication?)
quoted hunk ↗ jump to hunk
+ +mcast-groups: + name-prefix: ETHTOOL_MCGRP_ + name-suffix: _NAME + list: + - + name: monitordiff --git a/tools/net/ynl/samples/ethtool.pyb/tools/net/ynl/samples/ethtool.py new file mode 100755 index 000000000000..63c8e29f8e5d--- /dev/null +++ b/tools/net/ynl/samples/ethtool.py@@ -0,0 +1,30 @@ +#!/usr/bin/env python +# SPDX-License-Identifier: BSD-3-Clause + +import argparse + +from ynl import YnlFamily + + +def main(): + parser = argparse.ArgumentParser(description='YNL ethtool sample') + parser.add_argument('--spec', dest='spec', type=str, required=True) + parser.add_argument('--schema', dest='schema', type=str) + parser.add_argument('--device', dest='dev_name', type=str) + parser.add_argument('--ifindex', dest='ifindex', type=str) + args = parser.parse_args() + + ynl = YnlFamily(args.spec) + + if args.dev_name: + channels = ynl.channels_get({'header': {'dev_name':args.dev_name}}) + elif args.ifindex: + channels = ynl.channels_get({'header': {'dev_index': args.ifindex}}) + else: + return + print('Netlink responded with:') + print(channels) + + +if __name__ == "__main__": + main() -- 2.37.1