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

[Michal Kubecek]

On Thu, Mar 28, 2019 at 09:10:10AM +0100, Jiri Pirko wrote:
> Thu, Mar 28, 2019 at 03:05:14AM CET, f.fainelli@xxxxxxxxx wrote:
> >
> >
> >On 3/27/2019 2:50 AM, Jiri Pirko wrote:
> >> 
> >> 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.

Michal