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

[Jiri Pirko]

Mon, Dec 11, 2017 at 05:56:51PM CET, davem@xxxxxxxxxxxxx wrote:
>From: Jiri Pirko <jiri@xxxxxxxxxxx>
>Date: Mon, 11 Dec 2017 17:02:21 +0100
>
>> Mon, Dec 11, 2017 at 02:53:31PM CET, mkubecek@xxxxxxx wrote:
>>>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 <mkubecek@xxxxxxx>
>> 
>> [...]
>> 
>> 
>>>+
>>>+/* 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.