[PATCH] act: grab a reference when adding an action to a filter

[Thomas Haller]

On Mon, 2014-04-14 at 18:39 -0700, Cong Wang wrote:
> When we add an action to a filter, its lifetime becomes
> same with the filter. So in case user frees it before
> us, we could just grab a reference here.
> 
> Signed-off-by: Cong Wang <xiyou.wangcong at gmail.com>
> ---
>  lib/route/cls/basic.c | 2 ++
>  lib/route/cls/u32.c   | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/lib/route/cls/basic.c b/lib/route/cls/basic.c
> index 5a67fae..ae93d31 100644
> --- a/lib/route/cls/basic.c
> +++ b/lib/route/cls/basic.c
> @@ -228,6 +228,8 @@ int rtnl_basic_add_action(struct rtnl_cls *cls, struct rtnl_act *act)
>  		return -NLE_NOMEM;
>  
>  	b->b_mask |= BASIC_ATTR_ACTION;
> +	/* In case user frees it */
> +	nl_object_get(OBJ_CAST(act));
>  	return rtnl_act_append(&b->b_act, act);
>  }
>  
> diff --git a/lib/route/cls/u32.c b/lib/route/cls/u32.c
> index 52ab263..d342131 100644
> --- a/lib/route/cls/u32.c
> +++ b/lib/route/cls/u32.c
> @@ -470,6 +470,8 @@ int rtnl_u32_add_action(struct rtnl_cls *cls, struct rtnl_act *act)
>  		return -NLE_NOMEM;
>  
>  	u->cu_mask |= U32_ATTR_ACTION;
> +	/* In case user frees it */
> +	nl_object_get(OBJ_CAST(act));
>  	return rtnl_act_append(&u->cu_act, act);
>  }
>  


How about adding a function rtnl_act_get() -- analog to the existing
rtnl_act_put() function?



Again this is a behavioral change.

I think the behavior should be analog to other functions in libnl. Also
note, that now rtnl_u32_del_action() removes the object and ownership
goes back to the caller. I think this is inconsistent.
If course rtnl_u32_del_action() releasing the reference might mean, that
the action is gone when the function returns (but I would kind of expect
that).

Even if the behavior of the function is inconsistent, breaking API is so
annoying, that we might not fix this? What do you think? We could
justify the API break it by saying that pre-libnl3.2.25 these functions
were new and simply buggy.

In any case, it should be explicitly mentioned in the documentation and
you could add NL_CAPABILITY_RTNL_U32_ADD_ACTION_OWN_REFERENCE (or
similarly named) to indicate this API change.


The same applies to basic.c.



Unrelated to this, looking at u32_clone(), it does not create a deep
copy of cu_act (which it maybe should??) nor does it get additional
references to the objects. It should do either one.


Thomas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/libnl/attachments/20140415/65cce0ec/attachment.sig>