On Sunday, 30 December 2018 17.57.54 CET Linus Lüssing wrote:
On Fri, Dec 07, 2018 at 02:58:28PM +0100, Sven Eckelmann wrote:
+/**
- batadv_get_softif_from_info() - Retrieve soft interface from genl
attributes
- @net: the applicable net namespace
- @info: receiver information
- Return: Pointer to soft interface on success, error pointer on error
- */
+static struct batadv_priv * +batadv_get_softif_from_info(struct net *net, struct genl_info *info)
Since this returns a batadv_priv, shouldn't it better be called batadv_get_batpriv_from_info() or batadv_get_bat_priv_from_info() maybe?
OK
+{
- struct net_device *soft_iface;
- int ifindex;
- if (!info->attrs[BATADV_ATTR_MESH_IFINDEX])
return ERR_PTR(-EINVAL);- ifindex = nla_get_u32(info->attrs[BATADV_ATTR_MESH_IFINDEX]);
- soft_iface = dev_get_by_index(net, ifindex);
- if (!soft_iface)
return ERR_PTR(-ENODEV);- if (!batadv_softif_is_valid(soft_iface))
goto err_put_softif;- return netdev_priv(soft_iface);
+err_put_softif:
- dev_put(soft_iface);
- return ERR_PTR(-EINVAL);
+}
Is holding a reference to bat_priv->soft_iface really necessary (and releasing it in batadv_post_doit() )? If we are able to retrieve a valid bat_priv then this bat_priv itself should hold a reference to to soft_iface, shouldn't it?
No, we don't hold any explicit reference to bat_priv itself. The bat_priv is a part of the memory of the soft_iface. So we most hold a reference for soft_iface because we don't prevent that it is removed in the meantime (the netlink interface is registered per module and not per meshif/softif).
[...]
- if (ops->internal_flags & BATADV_FLAG_NEED_MESH) {
bat_priv = batadv_get_softif_from_info(genl_info_net(info),info);Would it look nicer to store genl_info_net(info) in a temporary variable so that its shorter and the newline for the second parameter could be avoided?
Ok
if (IS_ERR(bat_priv))return PTR_ERR(bat_priv);info->user_ptr[0] = bat_priv;Would it make sense to wrap this private data access into something somehow? Conceptually similar to what we do not with skb private data already for instance. There we use BATADV_SKB_CB() for instance.
Please not, we only have two pointers and I definitely don't want to force specific positions (we need three at the moment - but max two per command). You should compare it with the nl80211 code.
Kind regards, Sven