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

Michal Kubecek
2019-07-04T09:28:30Z

On Thu, Jul 04, 2019 at 10:49:13AM +0200, Jiri Pirko wrote:
> Tue, Jul 02, 2019 at 01:50:24PM CEST, mkubecek@suse.cz wrote:
>
> [...]
>
>
> >+/* The structure holding data for unified processing GET requests consists of
> >+ * two parts: request info and reply data. Request info is related to client
> >+ * request and for dump request it stays constant through all processing;
> >+ * reply data contains data for composing a reply message. When processing
> >+ * a dump request, request info is filled only once but reply data is filled
> >+ * from scratch for each reply message.
> >+ *
> >+ * +-----------------+-----------------+------------------+-----------------+
> >+ * | common_req_info | specific info | ethnl_reply_data | specific data |
> >+ * +-----------------+-----------------+------------------+-----------------+
> >+ * |<---------- request info --------->|<----------- reply data ----------->|
> >+ *
> >+ * Request info always starts at offset 0 with struct ethnl_req_info which
> >+ * holds information from parsing the common header. It may be followed by
> >+ * other members for request attributes specific for current message type.
> >+ * Reply data starts with struct ethnl_reply_data which may be followed by
> >+ * other members holding data needed to compose a message.
> >+ */
> >+
>
> [...]
>
>
> >+/**
> >+ * struct get_request_ops - unified handling of GET requests
> >+ * @request_cmd: command id for request (GET)
> >+ * @reply_cmd: command id for reply (GET_REPLY)
> >+ * @hdr_attr: attribute type for request header
> >+ * @max_attr: maximum (top level) attribute type
> >+ * @data_size: total length of data structure
> >+ * @repdata_offset: offset of "reply data" part (struct ethnl_reply_data)
>
> For example, this looks quite scarry for me. You have one big chunk of
> data (according to the scheme above) specific for cmd with reply starting
> at arbitrary offset.

We can split it into two structures, one for request related data with
struct ethnl_req_info embedded at offset 0 and one for reply related
data with struct ethnl_reply_data embedded at offset 0. It would be
probably more convenient to have pointer to request info from reply data
then. The code would get a bit simpler in few places at the expense of
an extra kmalloc().

Michal