Re: [PATCH net-next v6 09/15] ethtool: generic handlers for GET requests

[Jiri Pirko]

Tue, Jul 02, 2019 at 01:50:24PM CEST, mkubecek@xxxxxxx wrote:

[...]	
	
>+/* generic ->doit() handler for GET type requests */
>+static int ethnl_get_doit(struct sk_buff *skb, struct genl_info *info)

It is very unfortunate for review to introduce function in a patch and
don't use it. In general, this approach is frowned upon. You should use
whatever you introduce in the same patch. I understand it is sometimes
hard.

IIUC, you have one ethnl_get_doit for all possible commands, and you
have this ops to do cmd-specific tasks. That is quite unusual. Plus if
you consider the complicated datastructures connected with this, 
I'm lost from the beginning :( Any particular reason form this indirection?
I don't think any other generic netlink code does that (correct me if
I'm wrong). The nice thing about generic netlink is the fact that
you have separate handlers per cmd.

I don't think you need these ops and indirections. For the common parts,
just have a set of common helpers, as the other generic netlink users
are doing. The code would be much easier to read and follow then.


>+{
>+	const u8 cmd = info->genlhdr->cmd;
>+	const struct get_request_ops *ops;
>+	struct ethnl_req_info *req_info;
>+	struct sk_buff *rskb;
>+	void *reply_payload;
>+	int reply_len;
>+	int ret;
>+
>+	ops = get_requests[cmd];
>+	if (WARN_ONCE(!ops, "cmd %u has no get_request_ops\n", cmd))
>+		return -EOPNOTSUPP;
>+	req_info = ethnl_alloc_get_data(ops);
>+	if (!req_info)
>+		return -ENOMEM;
>+	ret = ethnl_std_parse(req_info, info->nlhdr, genl_info_net(info), ops,
>+			      info->extack, !ops->allow_nodev_do);
>+	if (ret < 0)
>+		goto err_dev;
>+	req_info->privileged = ethnl_is_privileged(skb);
>+	ethnl_init_reply_data(req_info, ops, req_info->dev);
>+
>+	rtnl_lock();
>+	ret = ops->prepare_data(req_info, info);
>+	if (ret < 0)
>+		goto err_rtnl;
>+	reply_len = ops->reply_size(req_info);
>+	if (ret < 0)
>+		goto err_cleanup;
>+	ret = -ENOMEM;
>+	rskb = ethnl_reply_init(reply_len, req_info->dev, ops->reply_cmd,
>+				ops->hdr_attr, info, &reply_payload);
>+	if (!rskb)
>+		goto err_cleanup;
>+	ret = ops->fill_reply(rskb, req_info);
>+	if (ret < 0)
>+		goto err_msg;
>+	rtnl_unlock();
>+
>+	genlmsg_end(rskb, reply_payload);
>+	if (req_info->dev)
>+		dev_put(req_info->dev);
>+	ethnl_free_get_data(ops, req_info);
>+	return genlmsg_reply(rskb, info);
>+
>+err_msg:
>+	WARN_ONCE(ret == -EMSGSIZE,
>+		  "calculated message payload length (%d) not sufficient\n",
>+		  reply_len);
>+	nlmsg_free(rskb);
>+err_cleanup:
>+	ethnl_free_get_data(ops, req_info);
>+err_rtnl:
>+	rtnl_unlock();
>+err_dev:
>+	if (req_info->dev)
>+		dev_put(req_info->dev);
>+	return ret;
>+}

[...]