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?
- keep counters as packets/bytes struct to reduce rename
noise in connlimit
I thought "struct nf_ct_ext_type" requires to pass a FQ struct in
.len/.align. OK, will check.
- maybe (hardly worth it) pull out the netlink.h change
No problem, I can add it in an additional pre-patch if you like.
Krzysztof Oledzki wrote:
+int nf_conntrack_acct_init(void)
+{
+ int ret;
+
+#ifdef CONFIG_NF_CT_ACCT
+ printk(KERN_ERR "CONFIG_NF_CT_ACCT is deprecated and will be removed
soon.\n");
+#endif
This should point out what people are supposed to do
(use the new runtime option). KERN_WARN or NOTICE
seems more appropriate.
Ack.
+
+ ret = nf_ct_extend_register(&acct_extend);
+ if (ret < 0) {
+ printk(KERN_ERR "nf_conntrack_acct: Unable to register
extension\n");
+ return ret;
+ }
+
+#ifdef CONFIG_SYSCTL
+ acct_sysctl_header =
register_sysctl_paths(nf_net_netfilter_sysctl_path, acct_sysctl_table);
+#endif
Please stay under the 80 character limit.
Ack.
Also should check whether registration succeeded.
Ack.
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"? 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.
Thanks.
Best regards,
Krzysztof Olędzki
