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

[Jiri Pirko]

Wed, Mar 27, 2019 at 10:53:42PM CET, mkubecek@xxxxxxx wrote:
>On Wed, Mar 27, 2019 at 05:35:07PM +0100, Jiri Pirko wrote:
>> Mon, Mar 25, 2019 at 06:08:24PM CET, mkubecek@xxxxxxx 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@xxxxxxx>
>> 
>> [...]
>> 
>> >+/**
>> >+ * 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 :(
>
>I'm a bit surprised by this position because so far my experience with
>linux networking code seemed to suggest that using simple wrappers and
>helpers is how things are supposed to be done. And while following a
>chain of such wrappers (often each in a different file) in a code I was
>reading for the first time could be frustrating at times, mostly I had
>to admit that this style has its merits. After all, genetlink itself is
>full of simple wrappers around netlink functions.
>
>Let me point out one thing: most of these helpers and wrappers are not
>artificial, they haven't been written in advance with an idea that they
>might be useful (the patch series does not, of course, reflect the
>development history); most of them were written when I realized I'm
>writing the same or almost the same code again and again.
>
>So when I caught myself writing
>
>  ... = nla_nest_start(skb, ... | NLA_F_NESTED);
>
>for the third or fourth time and I realized that every nla_nest_start()
>call in the code will have this bitwise or, I felt it would deserve
>a helper. (If I expected some objection, it was rather the optical
>asymmetry of ethnl_nest_start() being closed with nla_nest_end().)
>It would be much nicer to have it in nla_nest_start() but unfortunately
>it's too late for that.

Okay, this wrapper I can understand, but it certainly isn't ethnl
specific wrapper. It is generic, many others might use it:
$ git grep NLA_F_NESTED |grep nla_nest_start


>
>And it's exactly the same case with get_request_ops. For quite long
>(until after RFC v2), this framework didn't exist and code for get
>request processing (both doit and dumpit) and notifications was written
>separately for each message type. Realizing that big part of each new
>file is in fact an exact copy of the previous one with some string
>replacements and that it's going to be like that for most of the future
>files, that led me to identifying which parts are specific to message
>type and which are generic.
>
>If I have to get rid of get_request_ops, it will only result in having
>multiple copies of functions which would replace ethnl_get_doit(),
>ethnl_get_dumpit() and ethnl_std_notify(). They would be slightly
>simpler but would look the same except for "info" in one being replaced
>by "strset" in second, "settings" in third etc. Later, there would be
>one more copy for stats, one for tunables etc.
>
>I don't think the generic code can be handled just by pre_doit and
>post_doit as the generic and message specific part are interleaved and
>the generic parts are also different for do requests, dump requests and
>notifications.

What do you mean by "interleaved"? I guess you can make the attrs to be
formatted in the way it is not interleaved. Like what I suggested, to
have top-level generic attr enum and one of the generic attrs would nest
the cmd-specific attrs. That way, pre_doit can work with the generic
attrs, doit op can work with cmd-specific attrs. Similar to devlink
code.

>
>Michal