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

[Jiri Pirko]

Wed, Mar 27, 2019 at 10:26:04AM CET, mkubecek@xxxxxxx wrote:
>On Tue, Mar 26, 2019 at 02:42:51PM +0100, Jiri Pirko wrote:
>> Tue, Mar 26, 2019 at 02:24:27PM CET, mkubecek@xxxxxxx wrote:
>> >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:
>> >> >+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