Thu, Mar 28, 2019 at 06:00:20PM CET, f.fainelli@gmail.com wrote:

On 3/28/19 6:23 AM, Jiri Pirko wrote:

quoted

Thu, Mar 28, 2019 at 10:37:46AM CET, mkubecek@suse.cz wrote:

quoted

On Thu, Mar 28, 2019 at 09:10:10AM +0100, Jiri Pirko wrote:

quoted

Thu, Mar 28, 2019 at 03:05:14AM CET, f.fainelli@gmail.com wrote:

quoted

On 3/27/2019 2:50 AM, Jiri Pirko wrote:

quoted

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?

We could even name the notification explicitly with: ETHTOOL_MSG_NOTIF
or ETHTOOL_MSG_NTF just so we spell out exactly what those messages are.

Sound good. Something like:

ETHTOOL_MSG_FOO_GET
ETHTOOL_MSG_FOO_GET_RPLY /* kernel->userspace replies to get */
ETHTOOL_MSG_FOO_SET
ETHTOOL_MSG_FOO_ACT
ETHTOOL_MSG_FOO_NTF /* kernel->userspace async messages - notifications */

The names sound fine to me and having different message ids would still
allow processing messages by the same handler easily.

But there is one potential issue I would like to point out: this way we
spend 4 message ids for a get/set pair rather than 2. These message ids
(genlmsghdr::cmd) are u8, i.e. the resource is not as infinite as one
would wish. There are 80 ioctl commands (43 "get" and 29 "set") at the
moment.

Netlink API should be less greedy in general. I already combined some
ioctl commands into one netlink request type and with an easy way to add
new attributes to existing commands, we won't need to add new commands
as often (certainly not in a way which left us with 9 "get" and 9 "set"
ioctl commands for netdev features). So even with 4 ids per get/set
pair, we might be safe for reasonably long time. But it's still
something to keep in mind.

There are still 16 bits reserve in genl msg header:
struct genlmsghdr {
        __u8    cmd;
        __u8    version;
        __u16   reserved;
};

And you know not all message IDs will be making sense depending on the
direction, so aliasing specific message IDs to an existing value should
be fine?

You are right, good idea. There can be 2 enums one for in, one for out.