Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 10 Oct 2019 17:25:59 +0200
From:   Jiri Pirko <jiri@resnulli.us>
To:     Michal Kubecek <mkubecek@suse.cz>
Cc:     David Miller <davem@davemloft.net>, netdev@vger.kernel.org,
        Jakub Kicinski <jakub.kicinski@netronome.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        John Linville <linville@tuxdriver.com>,
        Stephen Hemminger <stephen@networkplumber.org>,
        Johannes Berg <johannes@sipsolutions.net>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v7 13/17] ethtool: add standard notification
 handler
Message-ID: <20191010152559.GA2994@nanopsycho>
References: <cover.1570654310.git.mkubecek@suse.cz>
 <ac2fef494116db9d4679f4501f1be6a7898ef724.1570654310.git.mkubecek@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ac2fef494116db9d4679f4501f1be6a7898ef724.1570654310.git.mkubecek@suse.cz>
User-Agent: Mutt/1.11.4 (2019-03-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Wed, Oct 09, 2019 at 10:59:40PM CEST, mkubecek@suse.cz wrote:
>The ethtool netlink notifications have the same format as related GET
>replies so that if generic GET handling framework is used to process GET
>requests, its callbacks and instance of struct get_request_ops can be
>also used to compose corresponding notification message.
>
>Provide function ethnl_std_notify() to be used as notification handler in
>ethnl_notify_handlers table.
>
>Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
>---
> net/ethtool/netlink.c | 89 +++++++++++++++++++++++++++++++++++++++++++
> net/ethtool/netlink.h |  3 +-
> 2 files changed, 91 insertions(+), 1 deletion(-)
>
>diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
>index 47b6aefa0bf9..dc52d912e0dd 100644
>--- a/net/ethtool/netlink.c
>+++ b/net/ethtool/netlink.c
>@@ -7,6 +7,7 @@
> static struct genl_family ethtool_genl_family;
> 
> static bool ethnl_ok __read_mostly;
>+static u32 ethnl_bcast_seq;
> 
> #define __LINK_MODE_NAME(speed, type, duplex) \
> 	#speed "base" #type "/" #duplex
>@@ -257,6 +258,18 @@ struct sk_buff *ethnl_reply_init(size_t payload, struct net_device *dev, u8 cmd,
> 	return NULL;
> }
> 
>+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.


>+
> /* GET request helpers */
> 
> /**
>@@ -588,6 +601,82 @@ static int ethnl_get_done(struct netlink_callback *cb)
> 	return 0;
> }
> 
>+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 ?


>+
>+/* generic notification handler */
>+static void ethnl_std_notify(struct net_device *dev, unsigned int cmd,

Better "common" comparing to "standard", I believe.


>+			     const void *data)
>+{
>+	struct ethnl_reply_data *reply_data;
>+	const struct get_request_ops *ops;
>+	struct ethnl_req_info *req_info;
>+	struct sk_buff *skb;
>+	void *reply_payload;
>+	int reply_len;
>+	int ret;
>+
>+	ops = ethnl_std_notify_to_ops(cmd);
>+	if (!ops)
>+		return;
>+	req_info = kzalloc(ops->req_info_size, GFP_KERNEL);
>+	if (!req_info)
>+		return;
>+	reply_data = kmalloc(ops->reply_data_size, GFP_KERNEL);
>+	if (!reply_data) {
>+		kfree(req_info);
>+		return;
>+	}
>+
>+	req_info->dev = dev;
>+	req_info->global_flags |= ETHTOOL_GFLAG_COMPACT_BITSETS;
>+
>+	ethnl_init_reply_data(reply_data, ops, dev);
>+	ret = ops->prepare_data(req_info, reply_data, NULL);
>+	if (ret < 0)
>+		goto err_cleanup;
>+	reply_len = ops->reply_size(req_info, reply_data);
>+	if (ret < 0)
>+		goto err_cleanup;
>+	ret = -ENOMEM;
>+	skb = genlmsg_new(reply_len, GFP_KERNEL);
>+	if (!skb)
>+		goto err_cleanup;
>+	reply_payload = ethnl_bcastmsg_put(skb, cmd);
>+	if (!reply_payload)
>+		goto err_skb;
>+	ret = ethnl_fill_reply_header(skb, dev, ops->hdr_attr);
>+	if (ret < 0)
>+		goto err_msg;
>+	ret = ops->fill_reply(skb, req_info, reply_data);
>+	if (ret < 0)
>+		goto err_msg;
>+	if (ops->cleanup_data)
>+		ops->cleanup_data(reply_data);
>+
>+	genlmsg_end(skb, reply_payload);
>+	kfree(reply_data);
>+	kfree(req_info);
>+	ethnl_multicast(skb, dev);
>+	return;
>+
>+err_msg:
>+	WARN_ONCE(ret == -EMSGSIZE,
>+		  "calculated message payload length (%d) not sufficient\n",
>+		  reply_len);
>+err_skb:
>+	nlmsg_free(skb);
>+err_cleanup:
>+	if (ops->cleanup_data)
>+		ops->cleanup_data(reply_data);
>+	kfree(reply_data);
>+	kfree(req_info);
>+	return;
>+}
>+
> /* notifications */
> 
> typedef void (*ethnl_notify_handler_t)(struct net_device *dev, unsigned int cmd,
>diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
>index a0ae47bebe51..23e82a4dd265 100644
>--- a/net/ethtool/netlink.h
>+++ b/net/ethtool/netlink.h
>@@ -316,7 +316,8 @@ static inline void ethnl_after_ops(struct net_device *dev)
>  * infrastructure. When used, a pointer to an instance of this structure is to
>  * be added to &get_requests array and generic handlers ethnl_get_doit(),
>  * ethnl_get_dumpit(), ethnl_get_start() and ethnl_get_done() used in
>- * @ethnl_genl_ops
>+ * @ethnl_genl_ops; ethnl_std_notify() can be used in @ethnl_notify_handlers
>+ * to send notifications of the corresponding type.
>  */
> struct get_request_ops {
> 	u8			request_cmd;
>-- 
>2.23.0
>