Re: [PATCH net-next v5 10/22] ethtool: generic handlers for GET requests

Jiri Pirko
2019-03-27T16:35:12Z

Mon, Mar 25, 2019 at 06:08:24PM CET, mkubecek@suse.cz wrote:
>Some parts of processing GET type requests and related notifications are
>independent of a command. Provide universal functions so that only four
>callbacks need to be defined for each command type:
>
> parse_request() - parse incoming message
> prepare_data() - retrieve data from driver or NIC
> reply_size() - estimate reply message size
> fill_reply() - compose reply message
>
>These callback are defined in an instance of struct get_request_ops.
>
>Signed-off-by: Michal Kubecek <mkubecek@suse.cz>

[...]

>+/**
>+ * struct get_request_ops - unified handling of GET requests
>+ * @request_cmd: command id for request (GET)
>+ * @reply_cmd: command id for reply (SET)
>+ * @dev_attr: attribute type for device specification
>+ * @data_size: total length of data structure
>+ * @repdata_offset: offset of "reply data" part (struct common_reply_data)
>+ * @allow_nodev_do: do not fail if device is not specified for non-dump request
>+ * @parse_request:
>+ * parse request message and fill request info; request info is zero
>+ * initialized on entry except reply_data pointer (which is initialized)
>+ * @prepare_data:
>+ * retrieve data needed to compose a reply message; reply data are zero
>+ * initialized on entry except for @dev
>+ * @reply_size:
>+ * return size of reply message payload without device specification;
>+ * returned size may be bigger than actual reply size but it must suffice
>+ * to hold the reply
>+ * @fill_reply:
>+ * fill reply message payload using the data prepared by @prepare_data()
>+ * @cleanup
>+ * (optional) called when data are no longer needed; use e.g. to free
>+ * any additional data structures allocated in prepare_data() which are
>+ * not part of the main structure
>+ *
>+ * Description of variable parts of GET request handling when using the unified
>+ * infrastructure. When used, a pointer to an instance of this structure is to
>+ * be added to &get_requests array, generic handlers ethnl_get_doit(),
>+ * ethnl_get_dumpit(), ethnl_get_start() and ethnl_get_done() used in
>+ * @ethnl_genl_ops and (optionally) ethnl_std_notify() as notification handler
>+ * in &ethnl_notify_handlers.
>+ */
>+struct get_request_ops {

First of all, please maintain the ethnl prefix. Not only here, but also
for other structs and functions (common_req_info, parse_*, etc).

But I must ask, why do we need this wrappers of ops around ops?
I believe it would be much nicer to use genl ops directly and do the
common work in pre_doit and and post_doit. Please see devlink.c for
examples.

Wrappers are unfortunately quite common in this patchset. Most of the
time, they do things much harder to follow and understand :(

>+ u8 request_cmd;
>+ u8 reply_cmd;
>+ u16 dev_attrtype;
>+ unsigned int data_size;
>+ unsigned int repdata_offset;
>+ bool allow_nodev_do;
>+
>+ int (*parse_request)(struct common_req_info *req_info,
>+ struct sk_buff *skb, struct genl_info *info,
>+ const struct nlmsghdr *nlhdr);
>+ int (*prepare_data)(struct common_req_info *req_info,
>+ struct genl_info *info);
>+ int (*reply_size)(const struct common_req_info *req_info);
>+ int (*fill_reply)(struct sk_buff *skb,
>+ const struct common_req_info *req_info);
>+ void (*cleanup)(struct common_req_info *req_info);
>+};
>+
> #endif /* _NET_ETHTOOL_NETLINK_H */
>--
>2.21.0
>