From: Jiri Pirko <jiri@resnulli.us>
To: Michal Kubecek <mkubecek@suse.cz>
Cc: David Miller <davem@davemloft.net>,
netdev@vger.kernel.org,
Jakub Kicinski <jakub.kicinski@netronome.com>,
Andrew Lunn <andrew@lunn.ch>,
Florian Fainelli <f.fainelli@gmail.com>,
John Linville <linville@tuxdriver.com>,
Stephen Hemminger <stephen@networkplumber.org>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v5 10/22] ethtool: generic handlers for GET requests
Date: Thu, 28 Mar 2019 14:32:18 +0100 [thread overview]
Message-ID: <20190328133218.GN14297@nanopsycho> (raw)
In-Reply-To: <20190327215342.GU26076@unicorn.suse.cz>
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
next prev parent reply other threads:[~2019-03-28 13:32 UTC|newest]
Thread overview: 109+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-25 17:07 [PATCH net-next v5 00/22] ethtool netlink interface, part 1 Michal Kubecek
2019-03-25 17:07 ` [PATCH net-next v5 01/22] rtnetlink: provide permanent hardware address in RTM_NEWLINK Michal Kubecek
2019-03-26 10:08 ` Jiri Pirko
2019-03-26 10:31 ` Michal Kubecek
2019-03-26 11:38 ` Jiri Pirko
2019-03-26 19:46 ` David Miller
2019-03-25 17:08 ` [PATCH net-next v5 02/22] netlink: introduce nla_put_bitfield32() Michal Kubecek
2019-03-26 10:09 ` Jiri Pirko
2019-03-28 1:56 ` Florian Fainelli
2019-03-25 17:08 ` [PATCH net-next v5 03/22] netlink: add strict version of nla_parse_nested() Michal Kubecek
2019-03-26 10:11 ` Jiri Pirko
2019-03-28 1:57 ` Florian Fainelli
2019-03-25 17:08 ` [PATCH net-next v5 04/22] ethtool: move to its own directory Michal Kubecek
2019-03-26 10:14 ` Jiri Pirko
2019-03-28 1:57 ` Florian Fainelli
2019-03-25 17:08 ` [PATCH net-next v5 05/22] ethtool: introduce ethtool netlink interface Michal Kubecek
2019-03-26 12:09 ` Jiri Pirko
2019-03-26 13:24 ` Michal Kubecek
2019-03-26 13:42 ` Jiri Pirko
2019-03-27 9:26 ` Michal Kubecek
2019-03-27 9:50 ` Jiri Pirko
2019-03-28 2:05 ` Florian Fainelli
2019-03-28 8:10 ` Jiri Pirko
2019-03-28 9:37 ` Michal Kubecek
2019-03-28 13:23 ` Jiri Pirko
2019-03-28 17:00 ` Florian Fainelli
2019-03-28 17:28 ` Jiri Pirko
2019-03-26 16:36 ` Jiri Pirko
2019-03-26 17:32 ` Michal Kubecek
2019-03-27 9:26 ` Jiri Pirko
2019-03-25 17:08 ` [PATCH net-next v5 06/22] ethtool: helper functions for " Michal Kubecek
2019-03-26 12:38 ` Jiri Pirko
2019-03-26 14:19 ` Michal Kubecek
2019-03-26 16:22 ` Jiri Pirko
2019-03-25 17:08 ` [PATCH net-next v5 07/22] ethtool: netlink bitset handling Michal Kubecek
2019-03-26 15:59 ` Jiri Pirko
2019-03-26 17:59 ` Michal Kubecek
2019-03-27 9:57 ` Jiri Pirko
2019-03-27 10:19 ` Michal Kubecek
2019-03-25 17:08 ` [PATCH net-next v5 08/22] ethtool: support for netlink notifications Michal Kubecek
2019-03-26 16:34 ` Jiri Pirko
2019-03-26 18:17 ` Michal Kubecek
2019-03-27 9:38 ` Jiri Pirko
2019-03-27 9:51 ` Andrew Lunn
2019-03-27 10:04 ` Jiri Pirko
2019-03-27 10:16 ` Andrew Lunn
2019-03-27 10:41 ` Jiri Pirko
2019-03-27 9:59 ` Michal Kubecek
2019-03-27 10:43 ` Jiri Pirko
2019-03-25 17:08 ` [PATCH net-next v5 09/22] ethtool: implement EVENT notifications Michal Kubecek
2019-03-27 13:04 ` Jiri Pirko
2019-03-27 14:14 ` Michal Kubecek
2019-03-28 2:14 ` Florian Fainelli
2019-03-28 6:41 ` Michal Kubecek
2019-03-28 9:16 ` Jiri Pirko
2019-03-25 17:08 ` [PATCH net-next v5 10/22] ethtool: generic handlers for GET requests Michal Kubecek
2019-03-27 16:35 ` Jiri Pirko
2019-03-27 21:53 ` Michal Kubecek
2019-03-28 13:32 ` Jiri Pirko [this message]
2019-03-25 17:08 ` [PATCH net-next v5 11/22] ethtool: move string arrays into common file Michal Kubecek
2019-03-28 2:17 ` Florian Fainelli
2019-03-25 17:08 ` [PATCH net-next v5 12/22] ethtool: provide string sets with GET_STRSET request Michal Kubecek
2019-03-27 20:12 ` Jiri Pirko
2019-03-27 22:56 ` Michal Kubecek
2019-03-29 8:43 ` Jiri Pirko
2019-03-28 2:25 ` Florian Fainelli
2019-03-28 7:18 ` Michal Kubecek
2019-03-28 13:43 ` Jiri Pirko
2019-03-28 14:04 ` Michal Kubecek
2019-03-28 17:35 ` Jiri Pirko
2019-03-28 18:52 ` Jakub Kicinski
2019-03-28 20:43 ` Michal Kubecek
2019-03-28 21:06 ` Jakub Kicinski
2019-03-29 6:52 ` Jiri Pirko
2019-03-28 22:28 ` Michal Kubecek
2019-03-25 17:08 ` [PATCH net-next v5 13/22] ethtool: provide driver/device information in GET_INFO request Michal Kubecek
2019-03-27 20:14 ` Jiri Pirko
2019-03-27 22:25 ` Michal Kubecek
2019-03-28 2:30 ` Florian Fainelli
2019-03-28 9:21 ` Jiri Pirko
2019-03-28 9:53 ` Michal Kubecek
2019-03-28 16:34 ` Jiri Pirko
2019-03-28 20:09 ` Jakub Kicinski
2019-03-28 22:46 ` Michal Kubecek
2019-03-29 18:48 ` Jakub Kicinski
2019-03-29 18:53 ` Florian Fainelli
2019-03-25 17:08 ` [PATCH net-next v5 14/22] ethtool: provide timestamping " Michal Kubecek
2019-03-28 3:36 ` Florian Fainelli
2019-03-28 10:03 ` Michal Kubecek
2019-03-25 17:08 ` [PATCH net-next v5 15/22] ethtool: provide link mode names as a string set Michal Kubecek
2019-03-28 3:52 ` Florian Fainelli
2019-03-25 17:08 ` [PATCH net-next v5 16/22] ethtool: provide link settings and link modes in GET_SETTINGS request Michal Kubecek
2019-03-28 3:44 ` Florian Fainelli
2019-03-28 10:04 ` Michal Kubecek
2019-03-25 17:08 ` [PATCH net-next v5 17/22] ethtool: set link settings and link modes with SET_SETTINGS request Michal Kubecek
2019-03-25 17:08 ` [PATCH net-next v5 18/22] ethtool: provide link state in GET_SETTINGS request Michal Kubecek
2019-03-25 17:08 ` [PATCH net-next v5 19/22] ethtool: provide WoL information " Michal Kubecek
2019-03-28 3:42 ` Florian Fainelli
2019-03-28 10:10 ` Michal Kubecek
2019-03-25 17:08 ` [PATCH net-next v5 20/22] ethtool: set WoL settings with SET_SETTINGS request Michal Kubecek
2019-03-28 3:49 ` Florian Fainelli
2019-03-25 17:08 ` [PATCH net-next v5 21/22] ethtool: provide message level in GET_SETTINGS request Michal Kubecek
2019-03-28 3:46 ` Florian Fainelli
2019-03-25 17:09 ` [PATCH net-next v5 22/22] ethtool: set message level with SET_SETTINGS request Michal Kubecek
2019-03-28 3:48 ` Florian Fainelli
2019-03-27 13:09 ` [PATCH net-next v5 00/22] ethtool netlink interface, part 1 Jiri Pirko
2019-03-27 14:28 ` Michal Kubecek
2019-03-27 15:19 ` Jiri Pirko
2019-03-27 19:12 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190328133218.GN14297@nanopsycho \
--to=jiri@resnulli.us \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=f.fainelli@gmail.com \
--cc=jakub.kicinski@netronome.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linville@tuxdriver.com \
--cc=mkubecek@suse.cz \
--cc=netdev@vger.kernel.org \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).