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

[Jiri Pirko <jiri@resnulli.us>]

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
>