diff mbox

[RFC,v2,02/11] br_netfilter: default sysctl settings in init_brnf_net

Message ID 5370C519.4040300@parallels.com
State Superseded
Headers show

Commit Message

Vasily Averin May 12, 2014, 12:56 p.m. UTC
Signed-off-by: Vasily Averin <vvs@openvz.org>
---
 net/bridge/br_netfilter.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

Comments

Patrick McHardy May 12, 2014, 2:07 p.m. UTC | #1
On Mon, May 12, 2014 at 04:56:57PM +0400, Vasily Averin wrote:
> 
> Signed-off-by: Vasily Averin <vvs@openvz.org>
> ---
>  net/bridge/br_netfilter.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
> index 2acf7fa..3e81fd6 100644
> --- a/net/bridge/br_netfilter.c
> +++ b/net/bridge/br_netfilter.c
> @@ -64,6 +64,18 @@ static int brnf_pass_vlan_indev __read_mostly = 0;
>  #define brnf_pass_vlan_indev 0
>  #endif
>  
> +#ifdef CONFIG_SYSCTL
> +static struct brnf_net init_brnf_net = {
> +	.hdr			= NULL,
> +	.call_arptables		= brnf_call_arptables,
> +	.call_iptables		= brnf_call_iptables,
> +	.call_ip6tables		= brnf_call_ip6tables,
> +	.filter_vlan_tagged	= brnf_filter_vlan_tagged,
> +	.filter_pppoe_tagged	= brnf_filter_pppoe_tagged,
> +	.pass_vlan_indev	= brnf_pass_vlan_indev,
> +};
> +#endif

These patches are split in an unnecessary excessive fashion and are hard
to review. The rule is one patch per logical change, not one patch per
file. Introducing a structure and not using it might be acceptable, but
adding a structure definition and not using it is just stupid.

Also introducing all these init_brnf_net conversion that are completely
replaced afterwards is just useless noise. Please combine them in a more
reasonable fashion and resend.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vasily Averin May 12, 2014, 4:31 p.m. UTC | #2
Dear Patrick,
thank you for feedback.
Frankly speaking I still badly understand how it's better to split this patch set.
Finally I've decided to combine v2 patches to 2 parts (1-8 and 9-11). 
Could you please explain how to it better?

This patch set enables per network namespace managemnt for br_netfiltes sysctls,
it allows to enable processing br-nf-call hooks in ones network namespaces 
and keep it disabled in another ones.

v3: patches are merged into more large chunks 
v2: removed extra overhead for CONFIG_SYSCTL=n

Vasily Averin (2):
  br_netfilter: common structure for sysctl flags
  br_netfilter: per-netns copy of structure for sysctl flags

 net/bridge/br_netfilter.c |  155 ++++++++++++++++++++++++++++++++++-----------
 net/bridge/br_private.h   |   13 ++++
 2 files changed, 130 insertions(+), 38 deletions(-)
Pablo Neira Ayuso May 29, 2014, 12:28 p.m. UTC | #3
Hi Vasily,

On Mon, May 12, 2014 at 08:31:46PM +0400, Vasily Averin wrote:
> Dear Patrick,
> thank you for feedback.
> Frankly speaking I still badly understand how it's better to split this patch set.
> Finally I've decided to combine v2 patches to 2 parts (1-8 and 9-11). 
> Could you please explain how to it better?
> 
> This patch set enables per network namespace managemnt for br_netfiltes sysctls,
> it allows to enable processing br-nf-call hooks in ones network namespaces 
> and keep it disabled in another ones.
> 
> v3: patches are merged into more large chunks 
> v2: removed extra overhead for CONFIG_SYSCTL=n

Any further concern with this v3 patchset? Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vasily Averin May 30, 2014, 10:04 a.m. UTC | #4
On 05/29/2014 04:28 PM, Pablo Neira Ayuso wrote:
> Hi Vasily,
> 
> On Mon, May 12, 2014 at 08:31:46PM +0400, Vasily Averin wrote:
>> This patch set enables per network namespace managemnt for br_netfiltes sysctls,
>> it allows to enable processing br-nf-call hooks in ones network namespaces 
>> and keep it disabled in another ones.
>>
>> v3: patches are merged into more large chunks 
>> v2: removed extra overhead for CONFIG_SYSCTL=n
> 
> Any further concern with this v3 patchset? Thanks.

Dear Pablo,
I'm going to re-make this patchset to add ability to inherit settings from new parent's net-namespace.
Unfortunately right now I'm quite busy by relocation, therefore this task is delayed.
Also I would like say thanks to Bart and Maciej Zenczykowski for feedback.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index 2acf7fa..3e81fd6 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -64,6 +64,18 @@  static int brnf_pass_vlan_indev __read_mostly = 0;
 #define brnf_pass_vlan_indev 0
 #endif
 
+#ifdef CONFIG_SYSCTL
+static struct brnf_net init_brnf_net = {
+	.hdr			= NULL,
+	.call_arptables		= brnf_call_arptables,
+	.call_iptables		= brnf_call_iptables,
+	.call_ip6tables		= brnf_call_ip6tables,
+	.filter_vlan_tagged	= brnf_filter_vlan_tagged,
+	.filter_pppoe_tagged	= brnf_filter_pppoe_tagged,
+	.pass_vlan_indev	= brnf_pass_vlan_indev,
+};
+#endif
+
 #define IS_IP(skb) \
 	(!vlan_tx_tag_present(skb) && skb->protocol == htons(ETH_P_IP))