Re: [RFC PATCH 2/9] ethtool: introduce ethtool netlink interface

Mon, Dec 11, 2017 at 05:56:51PM CET, davem@davemloft.net wrote:

From: Jiri Pirko <jiri@resnulli.us>
Date: Mon, 11 Dec 2017 17:02:21 +0100

quoted

Mon, Dec 11, 2017 at 02:53:31PM CET, mkubecek@suse.cz wrote:

quoted

No function implemented yet, only genetlink and module infrastructure.
Register/unregister genetlink family "ethtool" and allow the module to be
autoloaded by genetlink code (if built as a module, distributions would
probably prefer "y").

Signed-off-by: Michal Kubecek <redacted>

[...]

quoted

+
+/* identifies the device to query/set
+ * - use either ifindex or ifname, not both
+ * - for dumps and messages not related to a particular devices, fill neither
+ * - info_mask is a bitfield, interpretation depends on the command
+ */
+struct ethnlmsghdr {
+	__u32	ifindex;		/* device ifindex */
+	__u16	flags;			/* request/response flags */
+	__u16	info_mask;		/* request/response info mask */
+	char	ifname[IFNAMSIZ];	/* device name */

Why do you need this header? You can have 2 attrs:
ETHTOOL_ATTR_IFINDEX and
ETHTOOL_ATTR_IFNAME

Why do you need per-command flags and info_mask? Could be bitfield
attr if needed by specific command.

Jiri, we've had this discussion before :-)

For elements which are common to most, if not all, requests it makes
sense to define a base netlink message.

My opinion on this matter has not changed at all since the last time
we discussed this.

The discussion we had before was about flag bitfield that was there
*always*. In this case, that is not true. It is either ifindex or
ifname. Even rtnetlink has ifname as attribute.

The flags and info_mask is just big mystery. If it is per-command,
seems natural to have it as attributes.

So unless you have new information to provide to me on this issue
which might change my mind, please accept the result of the previous
discussion which is that a base netlink message is not only
appropriate but desirable.

Thank you.