Date: Mon, 11 Dec 2017 19:02:19 +0100
From: Jiri Pirko <jiri@resnulli.us>
To: David Miller <davem@davemloft.net>
Cc: mkubecek@suse.cz, netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 2/9] ethtool: introduce ethtool netlink interface
Message-ID: <20171211180219.GB2047@nanopsycho>
References: <cover.1513000306.git.mkubecek@suse.cz>
 <37174026f818dd1a61ca1b37bef2f1b114198de2.1513000306.git.mkubecek@suse.cz>
 <20171211160221.GA1885@nanopsycho>
 <20171211.115651.1046181633998981619.davem@davemloft.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20171211.115651.1046181633998981619.davem@davemloft.net>
User-Agent: Mutt/1.8.3 (2017-05-23)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2017
Lines: 57

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
>
>> Mon, Dec 11, 2017 at 02:53:31PM CET, mkubecek@suse.cz 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@suse.cz>
>> 
>> [...]
>> 
>> 
>>>+
>>>+/* 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.