On Thu, May 24, 2012 at 09:35:50AM +0800, Gao feng wrote: > Hi pablo: > > 于 2012年05月23日 18:12, Pablo Neira Ayuso 写道: > > On Mon, May 14, 2012 at 04:52:11PM +0800, Gao feng wrote: > >> From: Gao feng <gaofeng@xxxxxxxxxxxxxx> > >> > >> the struct nf_proto_net stroes proto's ctl_table_header and ctl_table, > >> nf_ct_l4proto_(un)register_sysctl use it to register sysctl. > >> > >> there are some changes for struct nf_conntrack_l4proto: > >> - add field compat to identify if this proto should do compat. > >> - the net_id field is used to store the pernet_operations id > >> that belones to l4proto. > >> - init_net will be used to initial the proto's pernet data > >> > >> and add init_net for struct nf_conntrack_l3proto too. > > > > This patchset looks bette but there are still things that we have to > > resolve. > > > > The first one (regarding this patch 1/17) changes in: > > * include/net/netfilter/nf_conntrack_l4proto.h > > * include/net/netns/conntrack.h > > > > should be included in: > > [PATCH] netfilter: add namespace support for l4proto > > > > And changes in: > > * include/net/netfilter/nf_conntrack_l3proto.h > > > > should be included in: > > [PATCH] netfilter: add namespace support for l3proto > > > > I already told you. A patch that adds a structure without using it, > > is not good. The structure has to go together with the code uses it. > > > > It seams this patch should be merged to "netfilter: add namespace support for l4proto" > the struct nf_proto_net is first used there. > > > More comments below. > > > >> Acked-by: Eric W. Biederman <ebiederm@xxxxxxxxxxxx> > >> Signed-off-by: Gao feng <gaofeng@xxxxxxxxxxxxxx> > >> --- > >> include/net/netfilter/nf_conntrack_l3proto.h | 3 +++ > >> include/net/netfilter/nf_conntrack_l4proto.h | 6 ++++++ > >> include/net/netns/conntrack.h | 12 ++++++++++++ > >> 3 files changed, 21 insertions(+), 0 deletions(-) > >> > >> diff --git a/include/net/netfilter/nf_conntrack_l3proto.h b/include/net/netfilter/nf_conntrack_l3proto.h > >> index 9699c02..9766005 100644 > >> --- a/include/net/netfilter/nf_conntrack_l3proto.h > >> +++ b/include/net/netfilter/nf_conntrack_l3proto.h > >> @@ -69,6 +69,9 @@ struct nf_conntrack_l3proto { > >> struct ctl_table *ctl_table; > >> #endif /* CONFIG_SYSCTL */ > >> > >> + /* Init l3proto pernet data */ > >> + int (*init_net)(struct net *net); > >> + > >> /* Module (if any) which this is connected to. */ > >> struct module *me; > >> }; > >> diff --git a/include/net/netfilter/nf_conntrack_l4proto.h b/include/net/netfilter/nf_conntrack_l4proto.h > >> index 3b572bb..a90eab5 100644 > >> --- a/include/net/netfilter/nf_conntrack_l4proto.h > >> +++ b/include/net/netfilter/nf_conntrack_l4proto.h > >> @@ -22,6 +22,8 @@ struct nf_conntrack_l4proto { > >> /* L4 Protocol number. */ > >> u_int8_t l4proto; > >> > >> + u_int8_t compat; > > > > I don't see why we need this new field. > > > > It seems to be set to 1 in each structure that has set: > > > > .ctl_compat_table > > > > to non-NULL. So, it's redundant. > > > > Moreover, you already know from the protocol tracker itself if you > > have to allocate the compat ctl table or not. > > > > In other words: You set compat to 1 for nf_conntrack_l4proto_generic. > > Then, you pass that compat value to generic_init_net via ->inet_net > > again, but this information (that determines if the compat has to be > > done or not) is already in the scope of the protocol tracker. > > > > because some protocols such l4proto_tcp6 and l4proto_tcp use the same init_net > function. the l4proto_tcp6 doesn't need compat sysctl, so we should use this new > field to identify if we should kmemdup compat_sysctl_table. Then, could you use two init_net functions? one for TCP for IPv4 and another for TCP for IPv6? > and beacuse protocols will have pernet ctl_compat_table and ctl_table,the .ctl_compat_table > field will be deleted in patch 15/17. so we should the new field compat. > > actually, we don't need to pass compat value for generic_init_net,beacuse > we know l4proto_generic need compat. But consider there are l4proto_tcp(6), and in order to keep > code readable,I prefer to add compat field and pass it to init_net. > > > You have to fix this. > > > >> + > >> /* Try to fill in the third arg: dataoff is offset past network protocol > >> hdr. Return true if possible. */ > >> bool (*pkt_to_tuple)(const struct sk_buff *skb, unsigned int dataoff, > >> @@ -103,6 +105,10 @@ struct nf_conntrack_l4proto { > >> struct ctl_table *ctl_compat_table; > >> #endif > >> #endif > >> + int *net_id; > >> + /* Init l4proto pernet data */ > >> + int (*init_net)(struct net *net, u_int8_t compat); > >> + > >> /* Protocol name */ > >> const char *name; > >> > >> diff --git a/include/net/netns/conntrack.h b/include/net/netns/conntrack.h > >> index a053a19..1f53038 100644 > >> --- a/include/net/netns/conntrack.h > >> +++ b/include/net/netns/conntrack.h > >> @@ -8,6 +8,18 @@ > >> struct ctl_table_header; > >> struct nf_conntrack_ecache; > >> > >> +struct nf_proto_net { > >> +#ifdef CONFIG_SYSCTL > >> + struct ctl_table_header *ctl_table_header; > >> + struct ctl_table *ctl_table; > >> +#ifdef CONFIG_NF_CONNTRACK_PROC_COMPAT > >> + struct ctl_table_header *ctl_compat_header; > >> + struct ctl_table *ctl_compat_table; > >> +#endif > >> +#endif > >> + unsigned int users; > >> +}; > >> + > >> struct netns_ct { > >> atomic_t count; > >> unsigned int expect_count; > > -- > > To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in > > the body of a message to majordomo@xxxxxxxxxxxxxxx > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > -- > To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html