| Message ID | ac2fef494116db9d4679f4501f1be6a7898ef724.1570654310.git.mkubecek@suse.cz |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | ethtool netlink interface, part 1 | expand |
Wed, Oct 09, 2019 at 10:59:40PM CEST, mkubecek@suse.cz wrote: >The ethtool netlink notifications have the same format as related GET >replies so that if generic GET handling framework is used to process GET >requests, its callbacks and instance of struct get_request_ops can be >also used to compose corresponding notification message. > >Provide function ethnl_std_notify() to be used as notification handler in >ethnl_notify_handlers table. > >Signed-off-by: Michal Kubecek <mkubecek@suse.cz> >--- > net/ethtool/netlink.c | 89 +++++++++++++++++++++++++++++++++++++++++++ > net/ethtool/netlink.h | 3 +- > 2 files changed, 91 insertions(+), 1 deletion(-) > >diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c >index 47b6aefa0bf9..dc52d912e0dd 100644 >--- a/net/ethtool/netlink.c >+++ b/net/ethtool/netlink.c >@@ -7,6 +7,7 @@ > static struct genl_family ethtool_genl_family; > > static bool ethnl_ok __read_mostly; >+static u32 ethnl_bcast_seq; > > #define __LINK_MODE_NAME(speed, type, duplex) \ > #speed "base" #type "/" #duplex >@@ -257,6 +258,18 @@ struct sk_buff *ethnl_reply_init(size_t payload, struct net_device *dev, u8 cmd, > return NULL; > } > >+static void *ethnl_bcastmsg_put(struct sk_buff *skb, u8 cmd) >+{ >+ return genlmsg_put(skb, 0, ++ethnl_bcast_seq, ðtool_genl_family, 0, >+ cmd); >+} >+ >+static int ethnl_multicast(struct sk_buff *skb, struct net_device *dev) >+{ >+ return genlmsg_multicast_netns(ðtool_genl_family, dev_net(dev), skb, >+ 0, ETHNL_MCGRP_MONITOR, GFP_KERNEL); >+} No need for these 2 helpers. Just put the code directly into ethnl_std_notify() and make the code easier to read. >+ > /* GET request helpers */ > > /** >@@ -588,6 +601,82 @@ static int ethnl_get_done(struct netlink_callback *cb) > return 0; > } > >+static const struct get_request_ops *ethnl_std_notify_to_ops(unsigned int cmd) >+{ >+ WARN_ONCE(1, "unexpected notification type %u\n", cmd); >+ return NULL; >+} Why this isn't a table similar to get_requests ? >+ >+/* generic notification handler */ >+static void ethnl_std_notify(struct net_device *dev, unsigned int cmd, Better "common" comparing to "standard", I believe. >+ const void *data) >+{ >+ struct ethnl_reply_data *reply_data; >+ const struct get_request_ops *ops; >+ struct ethnl_req_info *req_info; >+ struct sk_buff *skb; >+ void *reply_payload; >+ int reply_len; >+ int ret; >+ >+ ops = ethnl_std_notify_to_ops(cmd); >+ if (!ops) >+ return; >+ req_info = kzalloc(ops->req_info_size, GFP_KERNEL); >+ if (!req_info) >+ return; >+ reply_data = kmalloc(ops->reply_data_size, GFP_KERNEL); >+ if (!reply_data) { >+ kfree(req_info); >+ return; >+ } >+ >+ req_info->dev = dev; >+ req_info->global_flags |= ETHTOOL_GFLAG_COMPACT_BITSETS; >+ >+ ethnl_init_reply_data(reply_data, ops, dev); >+ ret = ops->prepare_data(req_info, reply_data, NULL); >+ if (ret < 0) >+ goto err_cleanup; >+ reply_len = ops->reply_size(req_info, reply_data); >+ if (ret < 0) >+ goto err_cleanup; >+ ret = -ENOMEM; >+ skb = genlmsg_new(reply_len, GFP_KERNEL); >+ if (!skb) >+ goto err_cleanup; >+ reply_payload = ethnl_bcastmsg_put(skb, cmd); >+ if (!reply_payload) >+ goto err_skb; >+ ret = ethnl_fill_reply_header(skb, dev, ops->hdr_attr); >+ if (ret < 0) >+ goto err_msg; >+ ret = ops->fill_reply(skb, req_info, reply_data); >+ if (ret < 0) >+ goto err_msg; >+ if (ops->cleanup_data) >+ ops->cleanup_data(reply_data); >+ >+ genlmsg_end(skb, reply_payload); >+ kfree(reply_data); >+ kfree(req_info); >+ ethnl_multicast(skb, dev); >+ return; >+ >+err_msg: >+ WARN_ONCE(ret == -EMSGSIZE, >+ "calculated message payload length (%d) not sufficient\n", >+ reply_len); >+err_skb: >+ nlmsg_free(skb); >+err_cleanup: >+ if (ops->cleanup_data) >+ ops->cleanup_data(reply_data); >+ kfree(reply_data); >+ kfree(req_info); >+ return; >+} >+ > /* notifications */ > > typedef void (*ethnl_notify_handler_t)(struct net_device *dev, unsigned int cmd, >diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h >index a0ae47bebe51..23e82a4dd265 100644 >--- a/net/ethtool/netlink.h >+++ b/net/ethtool/netlink.h >@@ -316,7 +316,8 @@ static inline void ethnl_after_ops(struct net_device *dev) > * infrastructure. When used, a pointer to an instance of this structure is to > * be added to &get_requests array and generic handlers ethnl_get_doit(), > * ethnl_get_dumpit(), ethnl_get_start() and ethnl_get_done() used in >- * @ethnl_genl_ops >+ * @ethnl_genl_ops; ethnl_std_notify() can be used in @ethnl_notify_handlers >+ * to send notifications of the corresponding type. > */ > struct get_request_ops { > u8 request_cmd; >-- >2.23.0 >
On Thu, Oct 10, 2019 at 05:25:59PM +0200, Jiri Pirko wrote: > Wed, Oct 09, 2019 at 10:59:40PM CEST, mkubecek@suse.cz wrote: > >+static void *ethnl_bcastmsg_put(struct sk_buff *skb, u8 cmd) > >+{ > >+ return genlmsg_put(skb, 0, ++ethnl_bcast_seq, ðtool_genl_family, 0, > >+ cmd); > >+} > >+ > >+static int ethnl_multicast(struct sk_buff *skb, struct net_device *dev) > >+{ > >+ return genlmsg_multicast_netns(ðtool_genl_family, dev_net(dev), skb, > >+ 0, ETHNL_MCGRP_MONITOR, GFP_KERNEL); > >+} > > No need for these 2 helpers. Just put the code directly into > ethnl_std_notify() and make the code easier to read. In later patches (not submitted yet), these two will be also called by other notification handlers. > >+static const struct get_request_ops *ethnl_std_notify_to_ops(unsigned int cmd) > >+{ > >+ WARN_ONCE(1, "unexpected notification type %u\n", cmd); > >+ return NULL; > >+} > > Why this isn't a table similar to get_requests ? It's a relic of earlier version before splitting the complex message types when the table was rather sparse. I'll change it to a lookup table to make it consistent with the rest of the code. > >+ > >+/* generic notification handler */ > >+static void ethnl_std_notify(struct net_device *dev, unsigned int cmd, > > Better "common" comparing to "standard", I believe. That's similar to ethnl_std_parse(), the idea is that this is the standard handler for notifications which are triggered without additional data and the message is the same as reply to corresponding "GET" request (which is generated by the standard ethnl_get_doit() handler). Notifications for actions and notifications for SET commands which cannot be generated this standard way will have to use their own (nonstandard) handler. Michal
Thu, Oct 10, 2019 at 08:17:43PM CEST, mkubecek@suse.cz wrote: >On Thu, Oct 10, 2019 at 05:25:59PM +0200, Jiri Pirko wrote: >> Wed, Oct 09, 2019 at 10:59:40PM CEST, mkubecek@suse.cz wrote: [...] >> >+ >> >+/* generic notification handler */ >> >+static void ethnl_std_notify(struct net_device *dev, unsigned int cmd, >> >> Better "common" comparing to "standard", I believe. > >That's similar to ethnl_std_parse(), the idea is that this is the >standard handler for notifications which are triggered without >additional data and the message is the same as reply to corresponding >"GET" request (which is generated by the standard ethnl_get_doit() >handler). Notifications for actions and notifications for SET commands >which cannot be generated this standard way will have to use their own >(nonstandard) handler. So "default"? The "standard" sounds rather weird to me. It isn't any "standard" :)
On Friday, 11 October 2019 7:56 Jiri Pirko wrote: > Thu, Oct 10, 2019 at 08:17:43PM CEST, mkubecek@suse.cz wrote: > >On Thu, Oct 10, 2019 at 05:25:59PM +0200, Jiri Pirko wrote: > >> Wed, Oct 09, 2019 at 10:59:40PM CEST, mkubecek@suse.cz wrote: > [...] > > >> >+ > >> >+/* generic notification handler */ > >> >+static void ethnl_std_notify(struct net_device *dev, unsigned int > >> >cmd,>> > >> Better "common" comparing to "standard", I believe. > > > >That's similar to ethnl_std_parse(), the idea is that this is the > >standard handler for notifications which are triggered without > >additional data and the message is the same as reply to corresponding > >"GET" request (which is generated by the standard ethnl_get_doit() > >handler). Notifications for actions and notifications for SET > >commands which cannot be generated this standard way will have to > >use their own (nonstandard) handler. > > So "default"? The "standard" sounds rather weird to me. It isn't any > "standard" :) Yes, "default" sounds fine. Michal
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c index 47b6aefa0bf9..dc52d912e0dd 100644 --- a/net/ethtool/netlink.c +++ b/net/ethtool/netlink.c @@ -7,6 +7,7 @@ static struct genl_family ethtool_genl_family; static bool ethnl_ok __read_mostly; +static u32 ethnl_bcast_seq; #define __LINK_MODE_NAME(speed, type, duplex) \ #speed "base" #type "/" #duplex @@ -257,6 +258,18 @@ struct sk_buff *ethnl_reply_init(size_t payload, struct net_device *dev, u8 cmd, return NULL; } +static void *ethnl_bcastmsg_put(struct sk_buff *skb, u8 cmd) +{ + return genlmsg_put(skb, 0, ++ethnl_bcast_seq, ðtool_genl_family, 0, + cmd); +} + +static int ethnl_multicast(struct sk_buff *skb, struct net_device *dev) +{ + return genlmsg_multicast_netns(ðtool_genl_family, dev_net(dev), skb, + 0, ETHNL_MCGRP_MONITOR, GFP_KERNEL); +} + /* GET request helpers */ /** @@ -588,6 +601,82 @@ static int ethnl_get_done(struct netlink_callback *cb) return 0; } +static const struct get_request_ops *ethnl_std_notify_to_ops(unsigned int cmd) +{ + WARN_ONCE(1, "unexpected notification type %u\n", cmd); + return NULL; +} + +/* generic notification handler */ +static void ethnl_std_notify(struct net_device *dev, unsigned int cmd, + const void *data) +{ + struct ethnl_reply_data *reply_data; + const struct get_request_ops *ops; + struct ethnl_req_info *req_info; + struct sk_buff *skb; + void *reply_payload; + int reply_len; + int ret; + + ops = ethnl_std_notify_to_ops(cmd); + if (!ops) + return; + req_info = kzalloc(ops->req_info_size, GFP_KERNEL); + if (!req_info) + return; + reply_data = kmalloc(ops->reply_data_size, GFP_KERNEL); + if (!reply_data) { + kfree(req_info); + return; + } + + req_info->dev = dev; + req_info->global_flags |= ETHTOOL_GFLAG_COMPACT_BITSETS; + + ethnl_init_reply_data(reply_data, ops, dev); + ret = ops->prepare_data(req_info, reply_data, NULL); + if (ret < 0) + goto err_cleanup; + reply_len = ops->reply_size(req_info, reply_data); + if (ret < 0) + goto err_cleanup; + ret = -ENOMEM; + skb = genlmsg_new(reply_len, GFP_KERNEL); + if (!skb) + goto err_cleanup; + reply_payload = ethnl_bcastmsg_put(skb, cmd); + if (!reply_payload) + goto err_skb; + ret = ethnl_fill_reply_header(skb, dev, ops->hdr_attr); + if (ret < 0) + goto err_msg; + ret = ops->fill_reply(skb, req_info, reply_data); + if (ret < 0) + goto err_msg; + if (ops->cleanup_data) + ops->cleanup_data(reply_data); + + genlmsg_end(skb, reply_payload); + kfree(reply_data); + kfree(req_info); + ethnl_multicast(skb, dev); + return; + +err_msg: + WARN_ONCE(ret == -EMSGSIZE, + "calculated message payload length (%d) not sufficient\n", + reply_len); +err_skb: + nlmsg_free(skb); +err_cleanup: + if (ops->cleanup_data) + ops->cleanup_data(reply_data); + kfree(reply_data); + kfree(req_info); + return; +} + /* notifications */ typedef void (*ethnl_notify_handler_t)(struct net_device *dev, unsigned int cmd, diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h index a0ae47bebe51..23e82a4dd265 100644 --- a/net/ethtool/netlink.h +++ b/net/ethtool/netlink.h @@ -316,7 +316,8 @@ static inline void ethnl_after_ops(struct net_device *dev) * infrastructure. When used, a pointer to an instance of this structure is to * be added to &get_requests array and generic handlers ethnl_get_doit(), * ethnl_get_dumpit(), ethnl_get_start() and ethnl_get_done() used in - * @ethnl_genl_ops + * @ethnl_genl_ops; ethnl_std_notify() can be used in @ethnl_notify_handlers + * to send notifications of the corresponding type. */ struct get_request_ops { u8 request_cmd;
The ethtool netlink notifications have the same format as related GET replies so that if generic GET handling framework is used to process GET requests, its callbacks and instance of struct get_request_ops can be also used to compose corresponding notification message. Provide function ethnl_std_notify() to be used as notification handler in ethnl_notify_handlers table. Signed-off-by: Michal Kubecek <mkubecek@suse.cz> --- net/ethtool/netlink.c | 89 +++++++++++++++++++++++++++++++++++++++++++ net/ethtool/netlink.h | 3 +- 2 files changed, 91 insertions(+), 1 deletion(-)