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

[Michal Kubecek]

On Tue, Mar 26, 2019 at 01:09:09PM +0100, Jiri Pirko wrote:
> Mon, Mar 25, 2019 at 06:08:09PM CET, mkubecek@xxxxxxx 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.

> 
> 
> >+
> >+    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.

> >+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.

> >+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.

> >+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.

Michal