Re: [B.A.T.M.A.N.] [RFC v3 01/19] batman-adv: Move common genl doit code pre/post hooks

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 2700 bytes --]

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


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]