Re: [PATCH] Drop expectation refcount after unlinking expectation

[Patrick McHardy <kaber@trash.net>]

Harald Welte wrote:
> On Fri, Aug 05, 2005 at 12:21:00PM +0200, Patrick McHardy wrote:
> 
>>Pablo Neira wrote:
>>
>>>This patch comes from the following thread:
>>>
>>>[PATCH 6/7] Fix expectation creation
>>>
>>>In unlink_expect, the expectation is removed from the list so the
>>>refcount must be dropped as well.
>>
>>The patch is fine, but it leaves the somewhat broken function
>>__ip_ct_expect_unlink_destroy around. I'm not sure what the
>>intention of this function is, it looks like a lazy way of
>>killing conntracks without manually dropping the timer refcnt.
>>Anyway it invites people to use it in the wrong place and
>>should die IMO.
> 
> Ok, if it dies we'd have to export unlink_expect() for ctnetlink.
> 
> However, all places calling unlink_expect() also call expect_put()
> immediately after it - with one exception: find_expectation() :(
> 
> Getting back to your comment:  
> 
>>it looks like a lazy way of killing conntracks without manually
>>dropping the timer refcnt.
> 
> Sorry, I can't follow you: Did you mean something like 'lazy way of
> killing expects without manually expiring the timer' ?  
> 
> So your whole objection is to not have timer_del() inside
> __ip_ct_expect_unlink_destroy() ?   this can be changed easily.

My objection was the other way around, unlink_expect drops the
reference taken for the lists, as it should. 
__ip_ct_expect_unlink_destroy() additionally drops a reference
which I assume is the timer reference, but it doesn't delete
the timer. If the caller deletes the timer, he should be the
one to drop the reference. Your suggestion is of course perfectly
fine with me, if the timer is deleted within that function it
can also drop the reference.