Messages in this thread | | | Date | Wed, 27 Mar 2019 10:50:23 +0100 | From | Jiri Pirko <> | Subject | Re: [PATCH net-next v5 05/22] ethtool: introduce ethtool netlink interface |
| |
Wed, Mar 27, 2019 at 10:26:04AM CET, mkubecek@suse.cz wrote: >On Tue, Mar 26, 2019 at 02:42:51PM +0100, Jiri Pirko wrote: >> 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: >> >> >+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 > >I would rather prefer the opposite: > >ATTR_HEADER > ATTR_IFINDEX > ATTR_IFNAME > ATTR_INFO_MASK > ATTR_PER_QUEUE >ATTR_CMDX_SOMETHING >ATTR_CMDX_SOMETHING2 >ATTR_CMDX_SOMETHING3 >... > >This way the "header" with universal attributes (not specfic to >a particular message type) would be kept together at the beginning even >after we need to add some more later and command specific attributes >would still have fixed numbers (starting from 2). I'll think about it >some more and check what would be pros and cons of the two variants >when parsing and generating the messages.
Okay, so what you suggest is per-cmd top level attr enum. That leads to duplications of common attributes: You would have to have HEADER attr defined in every cmd enum:
enum cmdx { ATTR_CMDX_HEADER ATTR_CMDX_SOMETHING ATTR_CMDX_SOMETHING2 ATTR_CMDX_SOMETHING3 };
enum cmdy { ATTR_CMDY_HEADER ATTR_CMDY_SOMETHING ATTR_CMDY_SOMETHING2 ATTR_CMDY_SOMETHING3 };
That is odd. TC has it and I hate it there :)
I think that the rtnetlink example is better. The generic things are in generic top level enum. Then you have IFLA_LINKINFO with per-type enums.
> >> >> >+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. > >How about > > ETHTOOL_MSG_GET_FOO for get requests > ETHTOOL_MSG_FOO for get replies, notifications and set requests > ETHTOOL_MSG_ACT_FOO for actions (renegotiation, reset, blinking, ...) > >?
Why don't you have ETHTOOL_MSG_SET_FOO for set? I think that for kerne->userspace the ETHTOOL_MSG_FOO if fine. I would change the ordering of words thought, but it is cosmetics: ETHTOOL_MSG_FOO /* kernel->userspace messages - replies, notifications */ ETHTOOL_MSG_FOO_GET ETHTOOL_MSG_FOO_SET ETHTOOL_MSG_FOO_ACT
What do you think?
> >Michal
| |