Messages in this thread | | | Date | Thu, 28 Mar 2019 14:32:18 +0100 | From | Jiri Pirko <> | Subject | Re: [PATCH net-next v5 10/22] ethtool: generic handlers for GET requests |
| |
Wed, Mar 27, 2019 at 10:53:42PM CET, mkubecek@suse.cz wrote: >On Wed, Mar 27, 2019 at 05:35:07PM +0100, Jiri Pirko wrote: >> 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 ðnl_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
| |