Messages in this thread | | | Date | Tue, 26 Mar 2019 14:42:51 +0100 | From | Jiri Pirko <> | Subject | Re: [PATCH net-next v5 05/22] ethtool: introduce ethtool netlink interface |
| |
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 >
| |