Messages in this thread | | | Date | Thu, 28 Mar 2019 10:37:46 +0100 | From | Michal Kubecek <> | Subject | Re: [PATCH net-next v5 05/22] ethtool: introduce ethtool netlink interface |
| |
On Thu, Mar 28, 2019 at 09:10:10AM +0100, Jiri Pirko wrote: > Thu, Mar 28, 2019 at 03:05:14AM CET, f.fainelli@gmail.com 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
| |