[Patch v2 2/4] veth: implement io_alloc()

[Thomas Graf]

On 03/28/14 at 06:08pm, Cong Wang wrote:
> Users don't have to call rtnl_link_veth_alloc(), instead
> use generic rtnl_link_set_type().
> 
> Signed-off-by: Cong Wang <xiyou.wangcong at gmail.com>
> ---
>  lib/route/link/veth.c | 39 +++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/route/link/veth.c b/lib/route/link/veth.c
> index 4ce1f10..cf2e513 100644
> --- a/lib/route/link/veth.c
> +++ b/lib/route/link/veth.c
> @@ -140,6 +140,29 @@ static int veth_put_attrs(struct nl_msg *msg, struct rtnl_link *link)
>  	return 0;
>  }
>  
> +static int veth_alloc(struct rtnl_link *link)
> +{
> +	struct rtnl_link *peer;
> +	int err;
> +
> +	if (link->l_info)
> +		return 0;

Can you put a comment above to explain that the return is needed
to break the recursion loop?

> +
> +	if (!(peer = rtnl_link_alloc())) {
> +		rtnl_link_put(link);

This put looks wrong. It could free the link object beneath
the caller.

> +		return -NLE_NOMEM;
> +	}
> +
> +	peer->l_info = link;

You need to acquire a refcnt to link to make sure link is
not freed while you hold that reference.

nl_object_get(OBJ_CAST(link));
peer->l_info = link;

And then release it again in io_free


> +	if ((err = rtnl_link_set_type(peer, "veth")) < 0) {
> +		rtnl_link_put(peer);
> +		return err;
> +	}
> +
> +	link->l_info = peer;
> +	return 0;
> +}
> +
>  static struct rtnl_link_info_ops veth_info_ops = {
>  	.io_name		= "veth",
>  	.io_parse		= veth_parse,
> @@ -147,6 +170,7 @@ static struct rtnl_link_info_ops veth_info_ops = {
>  	    [NL_DUMP_LINE]	= veth_dump_line,
>  	    [NL_DUMP_DETAILS]	= veth_dump_details,
>  	},
> +	.io_alloc		= veth_alloc,
>  	.io_clone		= veth_clone,
>  	.io_put_attrs		= veth_put_attrs,
>  };
> @@ -172,29 +196,16 @@ static struct rtnl_link_info_ops veth_info_ops = {
>   */
>  struct rtnl_link *rtnl_link_veth_alloc(void)
>  {
> -	struct rtnl_link *link, *peer;
> +	struct rtnl_link *link;
>  	int err;
>  
>  	if (!(link = rtnl_link_alloc()))
>  		return NULL;
> -	if (!(peer = rtnl_link_alloc())) {
> -		rtnl_link_put(link);
> -		return NULL;
> -	}
> -
>  	if ((err = rtnl_link_set_type(link, "veth")) < 0) {
> -		rtnl_link_put(peer);
> -		rtnl_link_put(link);
> -		return NULL;
> -	}
> -	if ((err = rtnl_link_set_type(peer, "veth")) < 0) {
> -		rtnl_link_put(peer);
>  		rtnl_link_put(link);
>  		return NULL;
>  	}
>  
> -	link->l_info = peer;
> -	peer->l_info = link;
>  	return link;
>  }

This part looks good.