Re: [PATCH 01/17] netfilter: add struct nf_proto_net for register l4proto sysctl

[Gao feng <gaofeng@cn.fujitsu.com>]

于 2012年05月24日 17:58, Pablo Neira Ayuso 写道:
> 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@cn.fujitus.com>
>>>>
>>>> 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@xmission.com>
>>>> Signed-off-by: Gao feng <gaofeng@cn.fujitus.com>
>>>> ---
>>>>  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?

Of cause, if you prefer to impletment it in this way.