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
Date: Wed, 27 Mar 2019 10:50:23 +0100 [thread overview]
Message-ID: <20190327095023.GC6979@nanopsycho> (raw)
In-Reply-To: <20190327092604.GP26076@unicorn.suse.cz>
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
next prev parent reply other threads:[~2019-03-27 9:50 UTC|newest]
Thread overview: 109+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-25 17:07 [PATCH net-next v5 00/22] ethtool netlink interface, part 1 Michal Kubecek
2019-03-25 17:07 ` [PATCH net-next v5 01/22] rtnetlink: provide permanent hardware address in RTM_NEWLINK Michal Kubecek
2019-03-26 10:08 ` Jiri Pirko
2019-03-26 10:31 ` Michal Kubecek
2019-03-26 11:38 ` Jiri Pirko
2019-03-26 19:46 ` David Miller
2019-03-25 17:08 ` [PATCH net-next v5 02/22] netlink: introduce nla_put_bitfield32() Michal Kubecek
2019-03-26 10:09 ` Jiri Pirko
2019-03-28 1:56 ` Florian Fainelli
2019-03-25 17:08 ` [PATCH net-next v5 03/22] netlink: add strict version of nla_parse_nested() Michal Kubecek
2019-03-26 10:11 ` Jiri Pirko
2019-03-28 1:57 ` Florian Fainelli
2019-03-25 17:08 ` [PATCH net-next v5 04/22] ethtool: move to its own directory Michal Kubecek
2019-03-26 10:14 ` Jiri Pirko
2019-03-28 1:57 ` Florian Fainelli
2019-03-25 17:08 ` [PATCH net-next v5 05/22] ethtool: introduce ethtool netlink interface Michal Kubecek
2019-03-26 12:09 ` Jiri Pirko
2019-03-26 13:24 ` Michal Kubecek
2019-03-26 13:42 ` Jiri Pirko
2019-03-27 9:26 ` Michal Kubecek
2019-03-27 9:50 ` Jiri Pirko [this message]
2019-03-28 2:05 ` Florian Fainelli
2019-03-28 8:10 ` Jiri Pirko
2019-03-28 9:37 ` Michal Kubecek
2019-03-28 13:23 ` Jiri Pirko
2019-03-28 17:00 ` Florian Fainelli
2019-03-28 17:28 ` Jiri Pirko
2019-03-26 16:36 ` Jiri Pirko
2019-03-26 17:32 ` Michal Kubecek
2019-03-27 9:26 ` Jiri Pirko
2019-03-25 17:08 ` [PATCH net-next v5 06/22] ethtool: helper functions for " Michal Kubecek
2019-03-26 12:38 ` Jiri Pirko
2019-03-26 14:19 ` Michal Kubecek
2019-03-26 16:22 ` Jiri Pirko
2019-03-25 17:08 ` [PATCH net-next v5 07/22] ethtool: netlink bitset handling Michal Kubecek
2019-03-26 15:59 ` Jiri Pirko
2019-03-26 17:59 ` Michal Kubecek
2019-03-27 9:57 ` Jiri Pirko
2019-03-27 10:19 ` Michal Kubecek
2019-03-25 17:08 ` [PATCH net-next v5 08/22] ethtool: support for netlink notifications Michal Kubecek
2019-03-26 16:34 ` Jiri Pirko
2019-03-26 18:17 ` Michal Kubecek
2019-03-27 9:38 ` Jiri Pirko
2019-03-27 9:51 ` Andrew Lunn
2019-03-27 10:04 ` Jiri Pirko
2019-03-27 10:16 ` Andrew Lunn
2019-03-27 10:41 ` Jiri Pirko
2019-03-27 9:59 ` Michal Kubecek
2019-03-27 10:43 ` Jiri Pirko
2019-03-25 17:08 ` [PATCH net-next v5 09/22] ethtool: implement EVENT notifications Michal Kubecek
2019-03-27 13:04 ` Jiri Pirko
2019-03-27 14:14 ` Michal Kubecek
2019-03-28 2:14 ` Florian Fainelli
2019-03-28 6:41 ` Michal Kubecek
2019-03-28 9:16 ` Jiri Pirko
2019-03-25 17:08 ` [PATCH net-next v5 10/22] ethtool: generic handlers for GET requests Michal Kubecek
2019-03-27 16:35 ` Jiri Pirko
2019-03-27 21:53 ` Michal Kubecek
2019-03-28 13:32 ` Jiri Pirko
2019-03-25 17:08 ` [PATCH net-next v5 11/22] ethtool: move string arrays into common file Michal Kubecek
2019-03-28 2:17 ` Florian Fainelli
2019-03-25 17:08 ` [PATCH net-next v5 12/22] ethtool: provide string sets with GET_STRSET request Michal Kubecek
2019-03-27 20:12 ` Jiri Pirko
2019-03-27 22:56 ` Michal Kubecek
2019-03-29 8:43 ` Jiri Pirko
2019-03-28 2:25 ` Florian Fainelli
2019-03-28 7:18 ` Michal Kubecek
2019-03-28 13:43 ` Jiri Pirko
2019-03-28 14:04 ` Michal Kubecek
2019-03-28 17:35 ` Jiri Pirko
2019-03-28 18:52 ` Jakub Kicinski
2019-03-28 20:43 ` Michal Kubecek
2019-03-28 21:06 ` Jakub Kicinski
2019-03-29 6:52 ` Jiri Pirko
2019-03-28 22:28 ` Michal Kubecek
2019-03-25 17:08 ` [PATCH net-next v5 13/22] ethtool: provide driver/device information in GET_INFO request Michal Kubecek
2019-03-27 20:14 ` Jiri Pirko
2019-03-27 22:25 ` Michal Kubecek
2019-03-28 2:30 ` Florian Fainelli
2019-03-28 9:21 ` Jiri Pirko
2019-03-28 9:53 ` Michal Kubecek
2019-03-28 16:34 ` Jiri Pirko
2019-03-28 20:09 ` Jakub Kicinski
2019-03-28 22:46 ` Michal Kubecek
2019-03-29 18:48 ` Jakub Kicinski
2019-03-29 18:53 ` Florian Fainelli
2019-03-25 17:08 ` [PATCH net-next v5 14/22] ethtool: provide timestamping " Michal Kubecek
2019-03-28 3:36 ` Florian Fainelli
2019-03-28 10:03 ` Michal Kubecek
2019-03-25 17:08 ` [PATCH net-next v5 15/22] ethtool: provide link mode names as a string set Michal Kubecek
2019-03-28 3:52 ` Florian Fainelli
2019-03-25 17:08 ` [PATCH net-next v5 16/22] ethtool: provide link settings and link modes in GET_SETTINGS request Michal Kubecek
2019-03-28 3:44 ` Florian Fainelli
2019-03-28 10:04 ` Michal Kubecek
2019-03-25 17:08 ` [PATCH net-next v5 17/22] ethtool: set link settings and link modes with SET_SETTINGS request Michal Kubecek
2019-03-25 17:08 ` [PATCH net-next v5 18/22] ethtool: provide link state in GET_SETTINGS request Michal Kubecek
2019-03-25 17:08 ` [PATCH net-next v5 19/22] ethtool: provide WoL information " Michal Kubecek
2019-03-28 3:42 ` Florian Fainelli
2019-03-28 10:10 ` Michal Kubecek
2019-03-25 17:08 ` [PATCH net-next v5 20/22] ethtool: set WoL settings with SET_SETTINGS request Michal Kubecek
2019-03-28 3:49 ` Florian Fainelli
2019-03-25 17:08 ` [PATCH net-next v5 21/22] ethtool: provide message level in GET_SETTINGS request Michal Kubecek
2019-03-28 3:46 ` Florian Fainelli
2019-03-25 17:09 ` [PATCH net-next v5 22/22] ethtool: set message level with SET_SETTINGS request Michal Kubecek
2019-03-28 3:48 ` Florian Fainelli
2019-03-27 13:09 ` [PATCH net-next v5 00/22] ethtool netlink interface, part 1 Jiri Pirko
2019-03-27 14:28 ` Michal Kubecek
2019-03-27 15:19 ` Jiri Pirko
2019-03-27 19:12 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190327095023.GC6979@nanopsycho \
--to=jiri@resnulli.us \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=f.fainelli@gmail.com \
--cc=jakub.kicinski@netronome.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linville@tuxdriver.com \
--cc=mkubecek@suse.cz \
--cc=netdev@vger.kernel.org \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).