[PATCH] Bug Fix: cache_include: Fix object ref release after successful object update

[Roopa Prabhu]

On 12/17/12 6:18 AM, Thomas Graf wrote:
> On 12/12/12 at 09:48pm, roopa at cumulusnetworks.com wrote:
>> From: roopa<roopa at cumulusnetworks.com>
>>
>> The current code does a rtnl_link_put on new object instead of
>> old object. This patch fixes it. None of the caches have support
>> for object update, so this should not have affected anyone yet.
>>
>> Signed-off-by: Roopa Prabhu<roopa at cumulusnetworks.com>
>> ---
>>   lib/cache.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/lib/cache.c b/lib/cache.c
>> index 283f1ac..04da99a 100644
>> --- a/lib/cache.c
>> +++ b/lib/cache.c
>> @@ -750,7 +750,7 @@ static int cache_include(struct nl_cache *cache, struct nl_object *obj,
>>   			 */
>>   			if (nl_object_update(old, obj) == 0) {
>>   				cb(cache, old, NL_ACT_CHANGE, data);
>> -				nl_object_put(obj);
>> +				nl_object_put(old);
>>   				return 0;
>>   			}
>
> The nl_object_put(obj) is obviously wrong but do we really need to
> decrement the reference counter of the old object? It was updated
> but needs to stay, right?
>
> I think we don't need to touch any ref counters at all. The ref
> counter of 'obj' will be dec'ed in the parser function of the
> respective object type and the ref counter of 'old' should be
> unchanged as it was only updated.
>
> Do you agree?
>

The put for old here is releasing the reference held due to 
nl_cache_search.

But i see what you are saying, if we want to hold references for the 
updates, then its probably better if we do it in the oo_update function. 
Because updates can be adding or deleting elements from the old object. 
For example in the route case, we will need to increase reference on 
adding the next hop and decrement the ref count on deleting the next 
hop. Otherwise we will keep incrementing the ref count of old in all 
cases. And no one will be decrementing.

Having said that, I don't see a strong reason to hold references
for the updates. If you think we should, one option for me is to respin
the ipv6 route update patch to hold appropriate references, unless
you have any other ideas.

thanks.