Tue, Jul 02, 2019 at 01:50:24PM CEST, mkube...@suse.cz 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;
>+}
[...]
