[RFC 0/2] generic netlink doit generalisation

[RFC 0/2] generic netlink doit generalisation

[Johannes Berg]

nl80211 does a lot of boilerplate work
for each generic netlink doit() operation,
this is an attempt to reduce it.

If you think this looks good, I'd like to
merge it through John's wireless tree.

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 1/2] genetlink: introduce pre_doit/post_doit hooks

[Johannes Berg]

[-- Attachment #1: genl-more-boilerplate.patch --]
[-- Type: text/plain, Size: 4233 bytes --]

From: Johannes Berg <johannes.berg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Each family may have some amount of boilerplate
locking code that applies to most, or even all,
commands.

This allows a family to handle such things in
a more generic way, by allowing it to
 a) include private flags in each operation
 b) specify a pre_doit hook that is called,
    before an operation's doit() callback and
    may return an error directly,
 c) specify a post_doit hook that can undo
    locking or similar things done by pre_doit,
    and finally
 d) include two private pointers in each info
    struct passed between all these operations
    including doit(). (It's two because I'll
    need two in nl80211 -- can be extended.)

Signed-off-by: Johannes Berg <johannes.berg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 include/net/genetlink.h |   18 ++++++++++++++++++
 net/netlink/genetlink.c |   14 +++++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

--- wireless-testing.orig/include/net/genetlink.h	2010-09-30 22:19:03.000000000 +0200
+++ wireless-testing/include/net/genetlink.h	2010-09-30 22:34:10.000000000 +0200
@@ -20,6 +20,9 @@ struct genl_multicast_group {
 	u32			id;
 };
 
+struct genl_ops;
+struct genl_info;
+
 /**
  * struct genl_family - generic netlink family
  * @id: protocol family idenfitier
@@ -29,6 +32,10 @@ struct genl_multicast_group {
  * @maxattr: maximum number of attributes supported
  * @netnsok: set to true if the family can handle network
  *	namespaces and should be presented in all of them
+ * @pre_doit: called before an operation's doit callback, it may
+ *	do additional, common, filtering and return an error
+ * @post_doit: called after an operation's doit callback, it may
+ *	undo operations done by pre_doit, for example release locks
  * @attrbuf: buffer to store parsed attributes
  * @ops_list: list of all assigned operations
  * @family_list: family list
@@ -41,6 +48,12 @@ struct genl_family {
 	unsigned int		version;
 	unsigned int		maxattr;
 	bool			netnsok;
+	int			(*pre_doit)(struct genl_ops *ops,
+					    struct sk_buff *skb,
+					    struct genl_info *info);
+	void			(*post_doit)(struct genl_ops *ops,
+					     struct sk_buff *skb,
+					     struct genl_info *info);
 	struct nlattr **	attrbuf;	/* private */
 	struct list_head	ops_list;	/* private */
 	struct list_head	family_list;	/* private */
@@ -55,6 +68,8 @@ struct genl_family {
  * @genlhdr: generic netlink message header
  * @userhdr: user specific header
  * @attrs: netlink attributes
+ * @_net: network namespace
+ * @user_ptr: user pointers
  */
 struct genl_info {
 	u32			snd_seq;
@@ -66,6 +81,7 @@ struct genl_info {
 #ifdef CONFIG_NET_NS
 	struct net *		_net;
 #endif
+	void *			user_ptr[2];
 };
 
 static inline struct net *genl_info_net(struct genl_info *info)
@@ -81,6 +97,7 @@ static inline void genl_info_net_set(str
 /**
  * struct genl_ops - generic netlink operations
  * @cmd: command identifier
+ * @internal_flags: flags used by the family
  * @flags: flags
  * @policy: attribute validation policy
  * @doit: standard command callback
@@ -90,6 +107,7 @@ static inline void genl_info_net_set(str
  */
 struct genl_ops {
 	u8			cmd;
+	u8			internal_flags;
 	unsigned int		flags;
 	const struct nla_policy	*policy;
 	int		       (*doit)(struct sk_buff *skb,
--- wireless-testing.orig/net/netlink/genetlink.c	2010-09-30 22:19:56.000000000 +0200
+++ wireless-testing/net/netlink/genetlink.c	2010-09-30 22:22:28.000000000 +0200
@@ -547,8 +547,20 @@ static int genl_rcv_msg(struct sk_buff *
 	info.userhdr = nlmsg_data(nlh) + GENL_HDRLEN;
 	info.attrs = family->attrbuf;
 	genl_info_net_set(&info, net);
+	memset(&info.user_ptr, 0, sizeof(info.user_ptr));
 
-	return ops->doit(skb, &info);
+	if (family->pre_doit) {
+		err = family->pre_doit(ops, skb, &info);
+		if (err)
+			return err;
+	}
+
+	err = ops->doit(skb, &info);
+
+	if (family->post_doit)
+		family->post_doit(ops, skb, &info);
+
+	return err;
 }
 
 static void genl_rcv(struct sk_buff *skb)


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 2/2] nl80211: use the new genetlink pre/post_doit hooks

[Johannes Berg]

[-- Attachment #1: nl80211-use-pre-post.patch --]
[-- Type: text/plain, Size: 16023 bytes --]

From: Johannes Berg <johannes.berg@intel.com>

This makes nl80211 use the new genetlink
pre_doit/post_doit hooks for locking and
checking the interface/wiphy index.

This is only partial and serves to demonstrate
the code improvement.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/wireless/nl80211.c |  329 +++++++++++++++++++------------------------------
 1 file changed, 130 insertions(+), 199 deletions(-)

--- wireless-testing.orig/net/wireless/nl80211.c	2010-09-30 23:05:34.000000000 +0200
+++ wireless-testing/net/wireless/nl80211.c	2010-09-30 23:08:40.000000000 +0200
@@ -23,6 +23,11 @@
 #include "nl80211.h"
 #include "reg.h"
 
+static int nl80211_pre_doit(struct genl_ops *ops, struct sk_buff *skb,
+			    struct genl_info *info);
+static void nl80211_post_doit(struct genl_ops *ops, struct sk_buff *skb,
+			      struct genl_info *info);
+
 /* the netlink family */
 static struct genl_family nl80211_fam = {
 	.id = GENL_ID_GENERATE,	/* don't bother with a hardcoded ID */
@@ -31,6 +36,8 @@ static struct genl_family nl80211_fam =
 	.version = 1,		/* no particular meaning now */
 	.maxattr = NL80211_ATTR_MAX,
 	.netnsok = true,
+	.pre_doit = nl80211_pre_doit,
+	.post_doit = nl80211_post_doit,
 };
 
 /* internal helper: get rdev and dev */
@@ -703,28 +710,18 @@ static int nl80211_dump_wiphy(struct sk_
 static int nl80211_get_wiphy(struct sk_buff *skb, struct genl_info *info)
 {
 	struct sk_buff *msg;
-	struct cfg80211_registered_device *dev;
-
-	dev = cfg80211_get_dev_from_info(info);
-	if (IS_ERR(dev))
-		return PTR_ERR(dev);
+	struct cfg80211_registered_device *dev = info->user_ptr[0];
 
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!msg)
-		goto out_err;
-
-	if (nl80211_send_wiphy(msg, info->snd_pid, info->snd_seq, 0, dev) < 0)
-		goto out_free;
+		return -ENOMEM;
 
-	cfg80211_unlock_rdev(dev);
+	if (nl80211_send_wiphy(msg, info->snd_pid, info->snd_seq, 0, dev) < 0) {
+		nlmsg_free(msg);
+		return -ENOBUFS;
+	}
 
 	return genlmsg_reply(msg, info);
-
- out_free:
-	nlmsg_free(msg);
- out_err:
-	cfg80211_unlock_rdev(dev);
-	return -ENOBUFS;
 }
 
 static const struct nla_policy txq_params_policy[NL80211_TXQ_ATTR_MAX + 1] = {
@@ -1137,33 +1134,20 @@ static int nl80211_dump_interface(struct
 static int nl80211_get_interface(struct sk_buff *skb, struct genl_info *info)
 {
 	struct sk_buff *msg;
-	struct cfg80211_registered_device *dev;
-	struct net_device *netdev;
-	int err;
-
-	err = get_rdev_dev_by_info_ifindex(info, &dev, &netdev);
-	if (err)
-		return err;
+	struct cfg80211_registered_device *dev = info->user_ptr[0];
+	struct net_device *netdev = info->user_ptr[1];
 
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!msg)
-		goto out_err;
+		return -ENOMEM;
 
 	if (nl80211_send_iface(msg, info->snd_pid, info->snd_seq, 0,
-			       dev, netdev) < 0)
-		goto out_free;
-
-	dev_put(netdev);
-	cfg80211_unlock_rdev(dev);
+			       dev, netdev) < 0) {
+		nlmsg_free(msg);
+		return -ENOBUFS;
+	}
 
 	return genlmsg_reply(msg, info);
-
- out_free:
-	nlmsg_free(msg);
- out_err:
-	dev_put(netdev);
-	cfg80211_unlock_rdev(dev);
-	return -ENOBUFS;
 }
 
 static const struct nla_policy mntr_flags_policy[NL80211_MNTR_FLAG_MAX + 1] = {
@@ -1223,39 +1207,29 @@ static int nl80211_valid_4addr(struct cf
 
 static int nl80211_set_interface(struct sk_buff *skb, struct genl_info *info)
 {
-	struct cfg80211_registered_device *rdev;
+	struct cfg80211_registered_device *rdev = info->user_ptr[0];
 	struct vif_params params;
 	int err;
 	enum nl80211_iftype otype, ntype;
-	struct net_device *dev;
+	struct net_device *dev = info->user_ptr[1];
 	u32 _flags, *flags = NULL;
 	bool change = false;
 
 	memset(&params, 0, sizeof(params));
 
-	rtnl_lock();
-
-	err = get_rdev_dev_by_info_ifindex(info, &rdev, &dev);
-	if (err)
-		goto unlock_rtnl;
-
 	otype = ntype = dev->ieee80211_ptr->iftype;
 
 	if (info->attrs[NL80211_ATTR_IFTYPE]) {
 		ntype = nla_get_u32(info->attrs[NL80211_ATTR_IFTYPE]);
 		if (otype != ntype)
 			change = true;
-		if (ntype > NL80211_IFTYPE_MAX) {
-			err = -EINVAL;
-			goto unlock;
-		}
+		if (ntype > NL80211_IFTYPE_MAX)
+			return -EINVAL;
 	}
 
 	if (info->attrs[NL80211_ATTR_MESH_ID]) {
-		if (ntype != NL80211_IFTYPE_MESH_POINT) {
-			err = -EINVAL;
-			goto unlock;
-		}
+		if (ntype != NL80211_IFTYPE_MESH_POINT)
+			return -EINVAL;
 		params.mesh_id = nla_data(info->attrs[NL80211_ATTR_MESH_ID]);
 		params.mesh_id_len = nla_len(info->attrs[NL80211_ATTR_MESH_ID]);
 		change = true;
@@ -1266,20 +1240,18 @@ static int nl80211_set_interface(struct
 		change = true;
 		err = nl80211_valid_4addr(rdev, dev, params.use_4addr, ntype);
 		if (err)
-			goto unlock;
+			return err;
 	} else {
 		params.use_4addr = -1;
 	}
 
 	if (info->attrs[NL80211_ATTR_MNTR_FLAGS]) {
-		if (ntype != NL80211_IFTYPE_MONITOR) {
-			err = -EINVAL;
-			goto unlock;
-		}
+		if (ntype != NL80211_IFTYPE_MONITOR)
+			return -EINVAL;
 		err = parse_monitor_flags(info->attrs[NL80211_ATTR_MNTR_FLAGS],
 					  &_flags);
 		if (err)
-			goto unlock;
+			return err;
 
 		flags = &_flags;
 		change = true;
@@ -1293,17 +1265,12 @@ static int nl80211_set_interface(struct
 	if (!err && params.use_4addr != -1)
 		dev->ieee80211_ptr->use_4addr = params.use_4addr;
 
- unlock:
-	dev_put(dev);
-	cfg80211_unlock_rdev(rdev);
- unlock_rtnl:
-	rtnl_unlock();
 	return err;
 }
 
 static int nl80211_new_interface(struct sk_buff *skb, struct genl_info *info)
 {
-	struct cfg80211_registered_device *rdev;
+	struct cfg80211_registered_device *rdev = info->user_ptr[0];
 	struct vif_params params;
 	int err;
 	enum nl80211_iftype type = NL80211_IFTYPE_UNSPECIFIED;
@@ -1320,19 +1287,9 @@ static int nl80211_new_interface(struct
 			return -EINVAL;
 	}
 
-	rtnl_lock();
-
-	rdev = cfg80211_get_dev_from_info(info);
-	if (IS_ERR(rdev)) {
-		err = PTR_ERR(rdev);
-		goto unlock_rtnl;
-	}
-
 	if (!rdev->ops->add_virtual_intf ||
-	    !(rdev->wiphy.interface_modes & (1 << type))) {
-		err = -EOPNOTSUPP;
-		goto unlock;
-	}
+	    !(rdev->wiphy.interface_modes & (1 << type)))
+		return -EOPNOTSUPP;
 
 	if (type == NL80211_IFTYPE_MESH_POINT &&
 	    info->attrs[NL80211_ATTR_MESH_ID]) {
@@ -1344,7 +1301,7 @@ static int nl80211_new_interface(struct
 		params.use_4addr = !!nla_get_u8(info->attrs[NL80211_ATTR_4ADDR]);
 		err = nl80211_valid_4addr(rdev, NULL, params.use_4addr, type);
 		if (err)
-			goto unlock;
+			return err;
 	}
 
 	err = parse_monitor_flags(type == NL80211_IFTYPE_MONITOR ?
@@ -1354,38 +1311,18 @@ static int nl80211_new_interface(struct
 		nla_data(info->attrs[NL80211_ATTR_IFNAME]),
 		type, err ? NULL : &flags, &params);
 
- unlock:
-	cfg80211_unlock_rdev(rdev);
- unlock_rtnl:
-	rtnl_unlock();
 	return err;
 }
 
 static int nl80211_del_interface(struct sk_buff *skb, struct genl_info *info)
 {
-	struct cfg80211_registered_device *rdev;
-	int err;
-	struct net_device *dev;
-
-	rtnl_lock();
+	struct cfg80211_registered_device *rdev = info->user_ptr[0];
+	struct net_device *dev = info->user_ptr[1];
 
-	err = get_rdev_dev_by_info_ifindex(info, &rdev, &dev);
-	if (err)
-		goto unlock_rtnl;
-
-	if (!rdev->ops->del_virtual_intf) {
-		err = -EOPNOTSUPP;
-		goto out;
-	}
-
-	err = rdev->ops->del_virtual_intf(&rdev->wiphy, dev);
+	if (!rdev->ops->del_virtual_intf)
+		return -EOPNOTSUPP;
 
- out:
-	cfg80211_unlock_rdev(rdev);
-	dev_put(dev);
- unlock_rtnl:
-	rtnl_unlock();
-	return err;
+	return rdev->ops->del_virtual_intf(&rdev->wiphy, dev);
 }
 
 struct get_key_cookie {
@@ -1438,9 +1375,9 @@ static void get_key_callback(void *c, st
 
 static int nl80211_get_key(struct sk_buff *skb, struct genl_info *info)
 {
-	struct cfg80211_registered_device *rdev;
+	struct cfg80211_registered_device *rdev = info->user_ptr[0];
 	int err;
-	struct net_device *dev;
+	struct net_device *dev = info->user_ptr[1];
 	u8 key_idx = 0;
 	u8 *mac_addr = NULL;
 	struct get_key_cookie cookie = {
@@ -1458,30 +1395,17 @@ static int nl80211_get_key(struct sk_buf
 	if (info->attrs[NL80211_ATTR_MAC])
 		mac_addr = nla_data(info->attrs[NL80211_ATTR_MAC]);
 
-	rtnl_lock();
-
-	err = get_rdev_dev_by_info_ifindex(info, &rdev, &dev);
-	if (err)
-		goto unlock_rtnl;
-
-	if (!rdev->ops->get_key) {
-		err = -EOPNOTSUPP;
-		goto out;
-	}
+	if (!rdev->ops->get_key)
+		return -EOPNOTSUPP;
 
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
-	if (!msg) {
-		err = -ENOMEM;
-		goto out;
-	}
+	if (!msg)
+		return -ENOMEM;
 
 	hdr = nl80211hdr_put(msg, info->snd_pid, info->snd_seq, 0,
 			     NL80211_CMD_NEW_KEY);
-
-	if (IS_ERR(hdr)) {
-		err = PTR_ERR(hdr);
-		goto free_msg;
-	}
+	if (IS_ERR(hdr))
+		return PTR_ERR(hdr);
 
 	cookie.msg = msg;
 	cookie.idx = key_idx;
@@ -1501,28 +1425,21 @@ static int nl80211_get_key(struct sk_buf
 		goto nla_put_failure;
 
 	genlmsg_end(msg, hdr);
-	err = genlmsg_reply(msg, info);
-	goto out;
+	return genlmsg_reply(msg, info);
 
  nla_put_failure:
 	err = -ENOBUFS;
  free_msg:
 	nlmsg_free(msg);
- out:
-	cfg80211_unlock_rdev(rdev);
-	dev_put(dev);
- unlock_rtnl:
-	rtnl_unlock();
-
 	return err;
 }
 
 static int nl80211_set_key(struct sk_buff *skb, struct genl_info *info)
 {
-	struct cfg80211_registered_device *rdev;
+	struct cfg80211_registered_device *rdev = info->user_ptr[0];
 	struct key_parse key;
 	int err;
-	struct net_device *dev;
+	struct net_device *dev = info->user_ptr[1];
 	int (*func)(struct wiphy *wiphy, struct net_device *netdev,
 		    u8 key_index);
 
@@ -1537,21 +1454,13 @@ static int nl80211_set_key(struct sk_buf
 	if (!key.def && !key.defmgmt)
 		return -EINVAL;
 
-	rtnl_lock();
-
-	err = get_rdev_dev_by_info_ifindex(info, &rdev, &dev);
-	if (err)
-		goto unlock_rtnl;
-
 	if (key.def)
 		func = rdev->ops->set_default_key;
 	else
 		func = rdev->ops->set_default_mgmt_key;
 
-	if (!func) {
-		err = -EOPNOTSUPP;
-		goto out;
-	}
+	if (!func)
+		return -EOPNOTSUPP;
 
 	wdev_lock(dev->ieee80211_ptr);
 	err = nl80211_key_allowed(dev->ieee80211_ptr);
@@ -1568,21 +1477,14 @@ static int nl80211_set_key(struct sk_buf
 #endif
 	wdev_unlock(dev->ieee80211_ptr);
 
- out:
-	cfg80211_unlock_rdev(rdev);
-	dev_put(dev);
-
- unlock_rtnl:
-	rtnl_unlock();
-
 	return err;
 }
 
 static int nl80211_new_key(struct sk_buff *skb, struct genl_info *info)
 {
-	struct cfg80211_registered_device *rdev;
+	struct cfg80211_registered_device *rdev = info->user_ptr[0];
 	int err;
-	struct net_device *dev;
+	struct net_device *dev = info->user_ptr[1];
 	struct key_parse key;
 	u8 *mac_addr = NULL;
 
@@ -1596,21 +1498,11 @@ static int nl80211_new_key(struct sk_buf
 	if (info->attrs[NL80211_ATTR_MAC])
 		mac_addr = nla_data(info->attrs[NL80211_ATTR_MAC]);
 
-	rtnl_lock();
-
-	err = get_rdev_dev_by_info_ifindex(info, &rdev, &dev);
-	if (err)
-		goto unlock_rtnl;
-
-	if (!rdev->ops->add_key) {
-		err = -EOPNOTSUPP;
-		goto out;
-	}
+	if (!rdev->ops->add_key)
+		return -EOPNOTSUPP;
 
-	if (cfg80211_validate_key_settings(rdev, &key.p, key.idx, mac_addr)) {
-		err = -EINVAL;
-		goto out;
-	}
+	if (cfg80211_validate_key_settings(rdev, &key.p, key.idx, mac_addr))
+		return -EINVAL;
 
 	wdev_lock(dev->ieee80211_ptr);
 	err = nl80211_key_allowed(dev->ieee80211_ptr);
@@ -1619,12 +1511,6 @@ static int nl80211_new_key(struct sk_buf
 					 mac_addr, &key.p);
 	wdev_unlock(dev->ieee80211_ptr);
 
- out:
-	cfg80211_unlock_rdev(rdev);
-	dev_put(dev);
- unlock_rtnl:
-	rtnl_unlock();
-
 	return err;
 }
 
@@ -5085,43 +4971,24 @@ nl80211_attr_cqm_policy[NL80211_ATTR_CQM
 static int nl80211_set_cqm_rssi(struct genl_info *info,
 				s32 threshold, u32 hysteresis)
 {
-	struct cfg80211_registered_device *rdev;
+	struct cfg80211_registered_device *rdev = info->user_ptr[0];
 	struct wireless_dev *wdev;
-	struct net_device *dev;
-	int err;
+	struct net_device *dev = info->user_ptr[1];
 
 	if (threshold > 0)
 		return -EINVAL;
 
-	rtnl_lock();
-
-	err = get_rdev_dev_by_info_ifindex(info, &rdev, &dev);
-	if (err)
-		goto unlock_rtnl;
-
 	wdev = dev->ieee80211_ptr;
 
-	if (!rdev->ops->set_cqm_rssi_config) {
-		err = -EOPNOTSUPP;
-		goto unlock_rdev;
-	}
+	if (!rdev->ops->set_cqm_rssi_config)
+		return -EOPNOTSUPP;
 
 	if (wdev->iftype != NL80211_IFTYPE_STATION &&
-	    wdev->iftype != NL80211_IFTYPE_P2P_CLIENT) {
-		err = -EOPNOTSUPP;
-		goto unlock_rdev;
-	}
-
-	err = rdev->ops->set_cqm_rssi_config(wdev->wiphy, dev,
-					     threshold, hysteresis);
-
- unlock_rdev:
-	cfg80211_unlock_rdev(rdev);
-	dev_put(dev);
- unlock_rtnl:
-	rtnl_unlock();
+	    wdev->iftype != NL80211_IFTYPE_P2P_CLIENT)
+		return -EOPNOTSUPP;
 
-	return err;
+	return rdev->ops->set_cqm_rssi_config(wdev->wiphy, dev,
+					      threshold, hysteresis);
 }
 
 static int nl80211_set_cqm(struct sk_buff *skb, struct genl_info *info)
@@ -5155,6 +5022,54 @@ out:
 	return err;
 }
 
+#define NL80211_FLAG_NEED_WIPHY		0x01
+#define NL80211_FLAG_NEED_NETDEV	0x02
+#define NL80211_FLAG_NEED_RTNL		0x04
+
+static int nl80211_pre_doit(struct genl_ops *ops, struct sk_buff *skb,
+			    struct genl_info *info)
+{
+	struct cfg80211_registered_device *rdev;
+	struct net_device *dev;
+	int err;
+	bool rtnl = ops->internal_flags & NL80211_FLAG_NEED_RTNL;
+
+	if (rtnl)
+		rtnl_lock();
+
+	if (ops->internal_flags & NL80211_FLAG_NEED_WIPHY) {
+		rdev = cfg80211_get_dev_from_info(info);
+		if (IS_ERR(rdev)) {
+			if (rtnl)
+				rtnl_unlock();
+			return PTR_ERR(rdev);
+		}
+		info->user_ptr[0] = rdev;
+	} else if (ops->internal_flags & NL80211_FLAG_NEED_NETDEV) {
+		err = get_rdev_dev_by_info_ifindex(info, &rdev, &dev);
+		if (err) {
+			if (rtnl)
+				rtnl_unlock();
+			return err;
+		}
+		info->user_ptr[0] = rdev;
+		info->user_ptr[1] = dev;
+	}
+
+	return 0;
+}
+
+static void nl80211_post_doit(struct genl_ops *ops, struct sk_buff *skb,
+			      struct genl_info *info)
+{
+	if (info->user_ptr[0])
+		cfg80211_unlock_rdev(info->user_ptr[0]);
+	if (info->user_ptr[1])
+		dev_put(info->user_ptr[1]);
+	if (ops->internal_flags & NL80211_FLAG_NEED_RTNL)
+		rtnl_unlock();
+}
+
 static struct genl_ops nl80211_ops[] = {
 	{
 		.cmd = NL80211_CMD_GET_WIPHY,
@@ -5162,6 +5077,7 @@ static struct genl_ops nl80211_ops[] = {
 		.dumpit = nl80211_dump_wiphy,
 		.policy = nl80211_policy,
 		/* can be retrieved by unprivileged users */
+		.internal_flags = NL80211_FLAG_NEED_WIPHY,
 	},
 	{
 		.cmd = NL80211_CMD_SET_WIPHY,
@@ -5175,42 +5091,55 @@ static struct genl_ops nl80211_ops[] = {
 		.dumpit = nl80211_dump_interface,
 		.policy = nl80211_policy,
 		/* can be retrieved by unprivileged users */
+		.internal_flags = NL80211_FLAG_NEED_NETDEV,
 	},
 	{
 		.cmd = NL80211_CMD_SET_INTERFACE,
 		.doit = nl80211_set_interface,
 		.policy = nl80211_policy,
 		.flags = GENL_ADMIN_PERM,
+		.internal_flags = NL80211_FLAG_NEED_NETDEV |
+				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
 		.cmd = NL80211_CMD_NEW_INTERFACE,
 		.doit = nl80211_new_interface,
 		.policy = nl80211_policy,
 		.flags = GENL_ADMIN_PERM,
+		.internal_flags = NL80211_FLAG_NEED_WIPHY |
+				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
 		.cmd = NL80211_CMD_DEL_INTERFACE,
 		.doit = nl80211_del_interface,
 		.policy = nl80211_policy,
 		.flags = GENL_ADMIN_PERM,
+		.internal_flags = NL80211_FLAG_NEED_NETDEV |
+				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
 		.cmd = NL80211_CMD_GET_KEY,
 		.doit = nl80211_get_key,
 		.policy = nl80211_policy,
 		.flags = GENL_ADMIN_PERM,
+		.internal_flags = NL80211_FLAG_NEED_NETDEV |
+				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
 		.cmd = NL80211_CMD_SET_KEY,
 		.doit = nl80211_set_key,
 		.policy = nl80211_policy,
 		.flags = GENL_ADMIN_PERM,
+		.internal_flags = NL80211_FLAG_NEED_NETDEV |
+				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
 		.cmd = NL80211_CMD_NEW_KEY,
 		.doit = nl80211_new_key,
 		.policy = nl80211_policy,
 		.flags = GENL_ADMIN_PERM,
+		.internal_flags = NL80211_FLAG_NEED_NETDEV |
+				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
 		.cmd = NL80211_CMD_DEL_KEY,
@@ -5464,6 +5393,8 @@ static struct genl_ops nl80211_ops[] = {
 		.doit = nl80211_set_cqm,
 		.policy = nl80211_policy,
 		.flags = GENL_ADMIN_PERM,
+		.internal_flags = NL80211_FLAG_NEED_NETDEV |
+				  NL80211_FLAG_NEED_RTNL,
 	},
 	{
 		.cmd = NL80211_CMD_SET_CHANNEL,

Re: [RFC 0/2] generic netlink doit generalisation

[Johannes Berg]

On Thu, 2010-09-30 at 23:10 +0200, Johannes Berg wrote:
> nl80211 does a lot of boilerplate work
> for each generic netlink doit() operation,
> this is an attempt to reduce it.

Here are the completed versions of the nl80211 cleanups:

http://johannes.sipsolutions.net/patches/kernel/all/LATEST/NNN-nl80211-use-pre-post.patch
http://johannes.sipsolutions.net/patches/kernel/all/LATEST/NNN-nl80211-generic-running.patch

The combined diffstat of these two is

 net/wireless/nl80211.c | 1756 +++++++++++++++----------------------------------
 1 file changed, 541 insertions(+), 1215 deletions(-)

and they reduce code size by >4k on my x86_64 compile.

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/2] genetlink: introduce pre_doit/post_doit hooks

[Julian Calaby]

On Fri, Oct 1, 2010 at 07:10, Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org> wrote:
> From: Johannes Berg <johannes.berg-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>
> Each family may have some amount of boilerplate
> locking code that applies to most, or even all,
> commands.
>
> This allows a family to handle such things in
> a more generic way, by allowing it to
>  a) include private flags in each operation
>  b) specify a pre_doit hook that is called,
>    before an operation's doit() callback and
>    may return an error directly,
>  c) specify a post_doit hook that can undo
>    locking or similar things done by pre_doit,
>    and finally
>  d) include two private pointers in each info
>    struct passed between all these operations
>    including doit(). (It's two because I'll
>    need two in nl80211 -- can be extended.)

Stupid question:

Why not have a priv struct rather than an arbitrary array of two pointers?

Thanks,

-- 

Julian Calaby

Email: julian.calaby-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/2] genetlink: introduce pre_doit/post_doit hooks

[Johannes Berg]

On Fri, 2010-10-01 at 08:41 +1000, Julian Calaby wrote:

> >  d) include two private pointers in each info
> >    struct passed between all these operations
> >    including doit(). (It's two because I'll
> >    need two in nl80211 -- can be extended.)
> 
> Stupid question:
> 
> Why not have a priv struct rather than an arbitrary array of two pointers?

It'd have to be dynamically allocated, and the "arbitrary" array of two
pointers can be on the stack.

johannes

Re: [RFC 1/2] genetlink: introduce pre_doit/post_doit hooks

[Johannes Berg]

On Fri, 2010-10-01 at 00:44 +0200, Johannes Berg wrote:
> On Fri, 2010-10-01 at 08:41 +1000, Julian Calaby wrote:
> 
> > >  d) include two private pointers in each info
> > >    struct passed between all these operations
> > >    including doit(). (It's two because I'll
> > >    need two in nl80211 -- can be extended.)
> > 
> > Stupid question:
> > 
> > Why not have a priv struct rather than an arbitrary array of two pointers?
> 
> It'd have to be dynamically allocated, and the "arbitrary" array of two
> pointers can be on the stack.

Maybe I should elaborate -- a priv struct basically means just a single
pointer, and then I'd have to allocate something to hold two pointers in
nl80211 and assign it to that single pointer.

johannes

Re: [RFC 1/2] genetlink: introduce pre_doit/post_doit hooks

[Johannes Berg]

On Fri, 2010-10-01 at 00:47 +0200, Johannes Berg wrote:
> On Fri, 2010-10-01 at 00:44 +0200, Johannes Berg wrote:
> > On Fri, 2010-10-01 at 08:41 +1000, Julian Calaby wrote:
> > 
> > > >  d) include two private pointers in each info
> > > >    struct passed between all these operations
> > > >    including doit(). (It's two because I'll
> > > >    need two in nl80211 -- can be extended.)
> > > 
> > > Stupid question:
> > > 
> > > Why not have a priv struct rather than an arbitrary array of two pointers?
> > 
> > It'd have to be dynamically allocated, and the "arbitrary" array of two
> > pointers can be on the stack.
> 
> Maybe I should elaborate -- a priv struct basically means just a single
> pointer, and then I'd have to allocate something to hold two pointers in
> nl80211 and assign it to that single pointer.

Come to think of it -- I could get away with a single pointer, since, if
both are assigned,

user_ptr[0] == wiphy_to_rdev(((netdev *)user_ptr[1])->ieee80211_ptr->wiphy)

but that's a lot of pointy things, and some functions only have the rdev
so it gets more complex. I think allowing two private pointers is a
decent compromise.

johannes

Re: [RFC 1/2] genetlink: introduce pre_doit/post_doit hooks

[Julian Calaby]

On Fri, Oct 1, 2010 at 08:49, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Fri, 2010-10-01 at 00:47 +0200, Johannes Berg wrote:
>> On Fri, 2010-10-01 at 00:44 +0200, Johannes Berg wrote:
>> > On Fri, 2010-10-01 at 08:41 +1000, Julian Calaby wrote:
>> >
>> > > >  d) include two private pointers in each info
>> > > >    struct passed between all these operations
>> > > >    including doit(). (It's two because I'll
>> > > >    need two in nl80211 -- can be extended.)
>> > >
>> > > Stupid question:
>> > >
>> > > Why not have a priv struct rather than an arbitrary array of two pointers?
>> >
>> > It'd have to be dynamically allocated, and the "arbitrary" array of two
>> > pointers can be on the stack.
>>
>> Maybe I should elaborate -- a priv struct basically means just a single
>> pointer, and then I'd have to allocate something to hold two pointers in
>> nl80211 and assign it to that single pointer.
>
> Come to think of it -- I could get away with a single pointer, since, if
> both are assigned,
>
> user_ptr[0] == wiphy_to_rdev(((netdev *)user_ptr[1])->ieee80211_ptr->wiphy)
>
> but that's a lot of pointy things, and some functions only have the rdev
> so it gets more complex. I think allowing two private pointers is a
> decent compromise.

Come to think of it -- if someone wanted to have a massive structure
with 10 pointers and a set of random data structures, then they could
easily create their priv struct and assign it to user_ptr[0], hence
rendering my argument null and void.

Thanks,

-- 

Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/

Re: [RFC 1/2] genetlink: introduce pre_doit/post_doit hooks

[Johannes Berg]

On Fri, 2010-10-01 at 08:51 +1000, Julian Calaby wrote:

> > Come to think of it -- I could get away with a single pointer, since, if
> > both are assigned,
> >
> > user_ptr[0] == wiphy_to_rdev(((netdev *)user_ptr[1])->ieee80211_ptr->wiphy)
> >
> > but that's a lot of pointy things, and some functions only have the rdev
> > so it gets more complex. I think allowing two private pointers is a
> > decent compromise.
> 
> Come to think of it -- if someone wanted to have a massive structure
> with 10 pointers and a set of random data structures, then they could
> easily create their priv struct and assign it to user_ptr[0], hence
> rendering my argument null and void.

Oh, well, I thought your argument was that it was arbitrary and not
really necessary :-)

Also, this rather cheap, it just needs a bit more stack space in a place
that isn't typically deeply nested. So if some protocol came around and
needed three pointers, I'd probably advocate just bumping it to three.
At some point I might draw a line (10 is probably too much).

But you're right, of course, they can just use the first one and put
something dynamically allocated into that, if really needed.

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/2] genetlink: introduce pre_doit/post_doit hooks

[Julian Calaby]

On Fri, Oct 1, 2010 at 08:55, Johannes Berg <johannes-cdvu00un1VgdHxzADdlk8Q@public.gmane.org> wrote:
> On Fri, 2010-10-01 at 08:51 +1000, Julian Calaby wrote:
>
>> > Come to think of it -- I could get away with a single pointer, since, if
>> > both are assigned,
>> >
>> > user_ptr[0] == wiphy_to_rdev(((netdev *)user_ptr[1])->ieee80211_ptr->wiphy)
>> >
>> > but that's a lot of pointy things, and some functions only have the rdev
>> > so it gets more complex. I think allowing two private pointers is a
>> > decent compromise.
>>
>> Come to think of it -- if someone wanted to have a massive structure
>> with 10 pointers and a set of random data structures, then they could
>> easily create their priv struct and assign it to user_ptr[0], hence
>> rendering my argument null and void.
>
> Oh, well, I thought your argument was that it was arbitrary and not
> really necessary :-)

My argument was more that someone's likely to come up with a scheme
that requires more than 2 pointers, so why not accommodate them from
start with an element for a priv struct - but that requires a new
struct, allocating and freeing it, as well as heap space for it, and a
pointer on the stack to it. (though I'm sure there'll be some sensible
way to make them persistent, but that's another issue)

> Also, this rather cheap, it just needs a bit more stack space in a place
> that isn't typically deeply nested. So if some protocol came around and
> needed three pointers, I'd probably advocate just bumping it to three.
> At some point I might draw a line (10 is probably too much).

I was considering pointing out that a compromise might need to be
made, but I figured you'd already thought of that =)

> But you're right, of course, they can just use the first one and put
> something dynamically allocated into that, if really needed.

Exactly.

Thanks,

-- 

Julian Calaby

Email: julian.calaby-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html