Re: [PATCH net-next v5 05/22] ethtool: introduce ethtool netlink interface

From: Jiri Pirko <jiri@resnulli.us>
To: Michal Kubecek <mkubecek@suse.cz>
Cc: David Miller <davem@davemloft.net>,
	netdev@vger.kernel.org,
	Jakub Kicinski <jakub.kicinski@netronome.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	John Linville <linville@tuxdriver.com>,
	Stephen Hemminger <stephen@networkplumber.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v5 05/22] ethtool: introduce ethtool netlink interface
Date: Tue, 26 Mar 2019 14:42:51 +0100	[thread overview]
Message-ID: <20190326134251.GA5181@nanopsycho.orion> (raw)
In-Reply-To: <20190326132427.GK26076@unicorn.suse.cz>

Tue, Mar 26, 2019 at 02:24:27PM CET, mkubecek@suse.cz wrote:
>On Tue, Mar 26, 2019 at 01:09:09PM +0100, Jiri Pirko wrote:
>> Mon, Mar 25, 2019 at 06:08:09PM CET, mkubecek@suse.cz wrote:
>> >+All "set" and "action" type requests require admin privileges (CAP_NET_ADMIN
>> >+in the namespace). Most "get" type request are allowed for anyone but there
>> 
>> s/request/requests/
>
>Will fix.
>
>> >+Device identification
>> >+---------------------
>> >+
>> >+When appropriate, network device is identified by a nested attribute named
>> >+ETHA_*_DEV. This attribute can contain
>> 
>> Isn't it ETHA_DEV_*? I must admit I'm a bit confused.
>
>ETHA_*_DEV is the nesting attribute (e.g. ETHA_SETTINGS_DEV), ETHA_DEV_*
>(ETHA_DEV_INDEX and ETHA_DEV_NAME) are in the nest.

Yeah. I wonder why you need to duplicate this. Can this be in top-lever
attr enum that is shared among all commands? It is there anyway and
looks a bit silly to have "DEV" attr separate for every command.
Something like this:

ATTR_IFINDEX
ATTR_IFNAME
ATTR_SOMEOTHER (flags perhaps)
ATTR_CMD_SPECIFIC_NEST_START
  ATTR_CMDX_SOMETHING
  ATTR_CMDX_SOMETHING2
  ATTR_CMDX_SOMETHING3
ATTR_CMD_SPECIFIC_NEST_END

>
>> 
>> 
>> >+
>> >+    ETHA_DEV_INDEX	(u32)		device ifindex
>> >+    ETHA_DEV_NAME	(string)	device name
>> >+
>> >+In device related requests, one of these is sufficient; if both are used, they
>> >+must match (i.e. identify the same device). In device related replies both are
>> 
>> You say this now for the second time. First time this was said in second
>> para.
>
>I'll drop one of them.
>
>> >+List of message types
>> >+---------------------
>> >+
>> >+All constants use ETHNL_CMD_ prefix, usually followed by "GET", "SET" or "ACT"
>> 
>> Why "usually"? Why not "always"?
>
>Right, it's always. And if it changes one day, the sentence will have to
>be rewritten anyway.

Okay.

>
>> >+Messages of type "get" are used by userspace to request information and
>> >+usually do not contain any attributes (that may be added later for dump
>> >+filtering). Kernel response is in the form of corresponding "set" message;
>> 
>> Okay. Do we want reply to "*_cmd_something_get" command to be
>> "*_cmd_something_set". That sounds odd. Why reply has to be "cmd"? Why
>> not something like "reply" or "response"?
>> This should work for both "doit/dumpit" and notifications.
>
>As stated right below, the aim is to use the same format for replies to
>GET requests as userspace uses for related SET requests. We could use
>different id (genlmsghdr::cmd) but that seemed like a waste for no actual
>gain.

I understand. I just wonder if the replies/notifications could use the
same name, not having "set" in it. I know we have it like this in many
netlink ifaces, it is however confusing to users. So once we are doing
this from scratch, we can do it differently.

>
>> >+the same message can be also used to set (some of) the parameters, except for
>> >+messages marked as "response only" in the table above. "Get" messages with
>> >+NLM_F_DUMP flags and no device identification dump the information for all
>> >+devices supporting the request.
>
>> >+
>> >+enum {
>> >+	ETHNL_CMD_NOOP,
>> >+
>> 
>> Usually headers have something like:
>> /* add new commands above here */
>> here.
>
>OK
>
>> >diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
>> >index 3ebfab2bca66..f30e0da88be5 100644
>> >--- a/net/ethtool/Makefile
>> >+++ b/net/ethtool/Makefile
>> >@@ -1,3 +1,7 @@
>> > # SPDX-License-Identifier: GPL-2.0
>> > 
>> >-obj-y		+= ioctl.o
>> >+obj-y				+= ioctl.o
>> >+
>> >+obj-$(CONFIG_ETHTOOL_NETLINK)	+= ethtool_nl.o
>> 
>> Why? I believe this should be always build-in same as ioctl.
>
>I would like to make the ioctl interface optional as well, eventually.
>As someone noted in one of the earlier discussions, there may be some
>special minimalistic setups where ethtool interface may be of no use.

Okay, fair enough.

>
>> >+struct genl_family ethtool_genl_family = {
>> >+	.hdrsize	= 0,
>> 
>> No need to set 0.
>
>OK
>
>> >+
>> >+extern struct genl_family ethtool_genl_family;
>> 
>> Why? You need this just within "netlink.c", don't you?
>
>In the submitted part, yes. But one of the later patches adds specific
>notify handler (different from ethnl_std_notify()) which is not in
>netlink.c and needs to use pointer to ethtool_genl_family for a call to
>genlmsg_put() and genlmsg_multicast().
>
>But I can make it static for now and change to extern when it's needed.

Please do.

>
>Michal
>