Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 27 Mar 2019 10:50:23 +0100
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
Message-ID: <20190327095023.GC6979@nanopsycho>
References: <cover.1553532199.git.mkubecek@suse.cz>
 <8795d07d3315b232b4e7ebc7d109c9aa3185e555.1553532199.git.mkubecek@suse.cz>
 <20190326120909.GF2230@nanopsycho>
 <20190326132427.GK26076@unicorn.suse.cz>
 <20190326134251.GA5181@nanopsycho.orion>
 <20190327092604.GP26076@unicorn.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190327092604.GP26076@unicorn.suse.cz>
User-Agent: Mutt/1.11.3 (2019-02-01)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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