Re: [PATCH net-next v7 13/17] ethtool: add standard notification handler

[Michal Kubecek]

On Thu, Oct 10, 2019 at 05:25:59PM +0200, Jiri Pirko wrote:
> Wed, Oct 09, 2019 at 10:59:40PM CEST, mkube...@suse.cz wrote:
> >+static void *ethnl_bcastmsg_put(struct sk_buff *skb, u8 cmd)
> >+{
> >+    return genlmsg_put(skb, 0, ++ethnl_bcast_seq, &ethtool_genl_family, 0,
> >+                       cmd);
> >+}
> >+
> >+static int ethnl_multicast(struct sk_buff *skb, struct net_device *dev)
> >+{
> >+    return genlmsg_multicast_netns(&ethtool_genl_family, dev_net(dev), skb,
> >+                                   0, ETHNL_MCGRP_MONITOR, GFP_KERNEL);
> >+}
> 
> No need for these 2 helpers. Just put the code directly into
> ethnl_std_notify() and make the code easier to read.

In later patches (not submitted yet), these two will be also called by
other notification handlers.

> >+static const struct get_request_ops *ethnl_std_notify_to_ops(unsigned int 
> >cmd)
> >+{
> >+    WARN_ONCE(1, "unexpected notification type %u\n", cmd);
> >+    return NULL;
> >+}
> 
> Why this isn't a table similar to get_requests ?

It's a relic of earlier version before splitting the complex message
types when the table was rather sparse. I'll change it to a lookup table
to make it consistent with the rest of the code.

> >+
> >+/* generic notification handler */
> >+static void ethnl_std_notify(struct net_device *dev, unsigned int cmd,
> 
> Better "common" comparing to "standard", I believe.

That's similar to ethnl_std_parse(), the idea is that this is the
standard handler for notifications which are triggered without
additional data and the message is the same as reply to corresponding
"GET" request (which is generated by the standard ethnl_get_doit()
handler). Notifications for actions and notifications for SET commands
which cannot be generated this standard way will have to use their own
(nonstandard) handler.

Michal