Re: [PATCH] Accounting rework: ct_extend + 64bit counters (ver. 2)

[Patrick McHardy <kaber@trash.net>]

Krzysztof Oledzki wrote:
> On Fri, 6 Jun 2008, Patrick McHardy wrote:
> 
>> Looks pretty good, just one or two minor things below. What would
>> be nice though is to reduce patch size a bit, two suggestions for
>> this are:
>>
>> - move seq_print_acct() changes to seperate cleanup patch
> 
> But those are not only cleanup changes but changes mainly required by 
> ct_extend implementation. The only cleanup change is that there is now 
> one function instead of two identical. Should I first change both 
> seq_print_counters into seq_print_acct and then merge them in an 
> additional post-patch?

If it makes sense, sure. Those were just suggestions, if
they don't make sense, feel free to ignore them :)

>>>  struct hlist_head *nf_ct_alloc_hashtable(unsigned int *sizep, int 
>>> *vmalloced)
>>> @@ -1165,6 +1174,10 @@ int __init nf_conntrack_init(void)
>>>      if (ret < 0)
>>>          goto out_fini_expect;
>>>  +    ret = nf_conntrack_acct_init();
>>> +    if (ret < 0)
>>> +        goto out_fini_acct;
>>> +
>>>      /* For use by REJECT target */
>>>      rcu_assign_pointer(ip_ct_attach, nf_conntrack_attach);
>>>      rcu_assign_pointer(nf_ct_destroy, destroy_conntrack);
>>> @@ -1177,6 +1190,7 @@ int __init nf_conntrack_init(void)
>>>       return ret;
>>>  +out_fini_acct:
>>
>> nf_conntrack_helper_fini(); ?
> 
> Hm... "helper"? You mean "acct"? 

No, I mean helper. If acct initialization fails, the helper
initialization was the last thing that succeeded and you need
to properly clean it up again.

> There is no nf_conntrack_acct_fini as 
> it seems to be currently unnecessary. So, this empty out_fini_acct seems 
> to be redundant here but I decided that adding it is cleaner than 
> calling "goto out_fini_*expect*" from the nf_conntrack_*acct*_init 
> error-path.

Good point, even though accounting and ct_extend are unloaded
together, for cleanness you should also unregister the extension
you've registered.