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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 10 Oct 2019 20:17:43 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     netdev@...r.kernel.org
Cc:     Jiri Pirko <jiri@...nulli.us>, David Miller <davem@...emloft.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        John Linville <linville@...driver.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Johannes Berg <johannes@...solutions.net>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v7 13/17] ethtool: add standard notification
 handler

On Thu, Oct 10, 2019 at 05:25:59PM +0200, Jiri Pirko wrote:
> Wed, Oct 09, 2019 at 10:59:40PM CEST, mkubecek@...e.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