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

[Michal Kubecek]

On Thu, Jul 04, 2019 at 10:49:13AM +0200, Jiri Pirko wrote:
> Tue, Jul 02, 2019 at 01:50:24PM CEST, mkubecek@xxxxxxx 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