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:

quoted

Tue, Mar 26, 2019 at 02:24:27PM CET, mkubecek@suse.cz wrote:

quoted

On Tue, Mar 26, 2019 at 01:09:09PM +0100, Jiri Pirko wrote:

quoted

Mon, Mar 25, 2019 at 06:08:09PM CET, mkubecek@suse.cz wrote:

quoted

+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.

quoted

quoted

quoted

quoted

+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