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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 27 Mar 2019 17:35:07 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Michal Kubecek <mkubecek@...e.cz>
Cc:     David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        John Linville <linville@...driver.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v5 10/22] ethtool: generic handlers for GET
 requests

Mon, Mar 25, 2019 at 06:08:24PM CET, mkubecek@...e.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@...e.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
>