[nf-next PATCH v2] ipvs: fix build errors related to config option combinations

[nf-next PATCH v2] ipvs: fix build errors related to config option combinations

[Jesper Dangaard Brouer]

Fix two build error introduced by commit 63dca2c0:
 "ipvs: Fix faulty IPv6 extension header handling in IPVS"

First build error was fairly trivial and can occur, when
CONFIG_IP_VS_IPV6 is disabled.

The second build error was tricky, and can occur when deselecting
both all Netfilter and IPVS, but selecting CONFIG_IPV6.  This is
caused by "kernel/sysctl_binary.c" including "net/ip_vs.h", which
includes "linux/netfilter_ipv6/ip6_tables.h" causing include
of "include/linux/netfilter/x_tables.h" which then cannot find
the typedef nf_hookfn.

Fix this by only including "linux/netfilter_ipv6/ip6_tables.h" in
case of CONFIG_IP_VS_IPV6 as its already used to guard the usage
of ipv6_find_hdr().

Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Reported-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---

 include/net/ip_vs.h |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
index a681ad6..68c69d5 100644
--- a/include/net/ip_vs.h
+++ b/include/net/ip_vs.h
@@ -22,7 +22,7 @@
 #include <linux/ip.h>
 #include <linux/ipv6.h>			/* for struct ipv6hdr */
 #include <net/ipv6.h>
-#if IS_ENABLED(CONFIG_IPV6)
+#if IS_ENABLED(CONFIG_IP_VS_IPV6)
 #include <linux/netfilter_ipv6/ip6_tables.h>
 #endif
 #if IS_ENABLED(CONFIG_NF_CONNTRACK)
@@ -212,8 +212,9 @@ ip_vs_fill_iph_addr_only(int af, const struct sk_buff *skb,
 			(struct ipv6hdr *)skb_network_header(skb);
 		iphdr->saddr.in6 = iph->saddr;
 		iphdr->daddr.in6 = iph->daddr;
-	} else {
+	} else
 #endif
+	{
 		const struct iphdr *iph =
 			(struct iphdr *)skb_network_header(skb);
 		iphdr->saddr.ip = iph->saddr;

Re: [nf-next PATCH v2] ipvs: fix build errors related to config option combinations

[Simon Horman]

On Mon, Oct 22, 2012 at 09:22:45PM +0200, Jesper Dangaard Brouer wrote:
> Fix two build error introduced by commit 63dca2c0:
>  "ipvs: Fix faulty IPv6 extension header handling in IPVS"
> 
> First build error was fairly trivial and can occur, when
> CONFIG_IP_VS_IPV6 is disabled.
> 
> The second build error was tricky, and can occur when deselecting
> both all Netfilter and IPVS, but selecting CONFIG_IPV6.  This is
> caused by "kernel/sysctl_binary.c" including "net/ip_vs.h", which
> includes "linux/netfilter_ipv6/ip6_tables.h" causing include
> of "include/linux/netfilter/x_tables.h" which then cannot find
> the typedef nf_hookfn.
> 
> Fix this by only including "linux/netfilter_ipv6/ip6_tables.h" in
> case of CONFIG_IP_VS_IPV6 as its already used to guard the usage
> of ipv6_find_hdr().

Thanks, I have verified both of these fixes and I will send a pull
request ASAP.

I do wonder how we might get earlier test coverage of these kinds of problems.

> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> Reported-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
> 
>  include/net/ip_vs.h |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
> index a681ad6..68c69d5 100644
> --- a/include/net/ip_vs.h
> +++ b/include/net/ip_vs.h
> @@ -22,7 +22,7 @@
>  #include <linux/ip.h>
>  #include <linux/ipv6.h>			/* for struct ipv6hdr */
>  #include <net/ipv6.h>
> -#if IS_ENABLED(CONFIG_IPV6)
> +#if IS_ENABLED(CONFIG_IP_VS_IPV6)
>  #include <linux/netfilter_ipv6/ip6_tables.h>
>  #endif
>  #if IS_ENABLED(CONFIG_NF_CONNTRACK)
> @@ -212,8 +212,9 @@ ip_vs_fill_iph_addr_only(int af, const struct sk_buff *skb,
>  			(struct ipv6hdr *)skb_network_header(skb);
>  		iphdr->saddr.in6 = iph->saddr;
>  		iphdr->daddr.in6 = iph->daddr;
> -	} else {
> +	} else
>  #endif
> +	{
>  		const struct iphdr *iph =
>  			(struct iphdr *)skb_network_header(skb);
>  		iphdr->saddr.ip = iph->saddr;
> 

Re: [nf-next PATCH v2] ipvs: fix build errors related to config option combinations

[Pablo Neira Ayuso]

On Tue, Oct 23, 2012 at 09:33:15AM +0900, Simon Horman wrote:
> On Mon, Oct 22, 2012 at 09:22:45PM +0200, Jesper Dangaard Brouer wrote:
> > Fix two build error introduced by commit 63dca2c0:
> >  "ipvs: Fix faulty IPv6 extension header handling in IPVS"
> > 
> > First build error was fairly trivial and can occur, when
> > CONFIG_IP_VS_IPV6 is disabled.
> > 
> > The second build error was tricky, and can occur when deselecting
> > both all Netfilter and IPVS, but selecting CONFIG_IPV6.  This is
> > caused by "kernel/sysctl_binary.c" including "net/ip_vs.h", which
> > includes "linux/netfilter_ipv6/ip6_tables.h" causing include
> > of "include/linux/netfilter/x_tables.h" which then cannot find
> > the typedef nf_hookfn.
> > 
> > Fix this by only including "linux/netfilter_ipv6/ip6_tables.h" in
> > case of CONFIG_IP_VS_IPV6 as its already used to guard the usage
> > of ipv6_find_hdr().
> 
> Thanks, I have verified both of these fixes and I will send a pull
> request ASAP.
> 
> I do wonder how we might get earlier test coverage of these kinds of problems.

David already mentioned that we (Netfilter/IPVS) should aim to reduce
the amount of ifdef pollution in the code. Sometimes it is possible in
a nice way by encapsulating code that depends on the feature in one
single file.

Sometimes it also requires some more thinking to make it that way (not
as easy as adding ifdef).

I think if we aim to that, we can avoid this sort of problems.

Re: [nf-next PATCH v2] ipvs: fix build errors related to config option combinations

[Pablo Neira Ayuso]

On Tue, Oct 23, 2012 at 10:42:41AM +0200, Pablo Neira Ayuso wrote:
> On Tue, Oct 23, 2012 at 09:33:15AM +0900, Simon Horman wrote:
> > On Mon, Oct 22, 2012 at 09:22:45PM +0200, Jesper Dangaard Brouer wrote:
> > > Fix two build error introduced by commit 63dca2c0:
> > >  "ipvs: Fix faulty IPv6 extension header handling in IPVS"
> > > 
> > > First build error was fairly trivial and can occur, when
> > > CONFIG_IP_VS_IPV6 is disabled.
> > > 
> > > The second build error was tricky, and can occur when deselecting
> > > both all Netfilter and IPVS, but selecting CONFIG_IPV6.  This is
> > > caused by "kernel/sysctl_binary.c" including "net/ip_vs.h", which
> > > includes "linux/netfilter_ipv6/ip6_tables.h" causing include
> > > of "include/linux/netfilter/x_tables.h" which then cannot find
> > > the typedef nf_hookfn.
> > > 
> > > Fix this by only including "linux/netfilter_ipv6/ip6_tables.h" in
> > > case of CONFIG_IP_VS_IPV6 as its already used to guard the usage
> > > of ipv6_find_hdr().
> > 
> > Thanks, I have verified both of these fixes and I will send a pull
> > request ASAP.
> > 
> > I do wonder how we might get earlier test coverage of these kinds of problems.
> 
> David already mentioned that we (Netfilter/IPVS) should aim to reduce
> the amount of ifdef pollution in the code. Sometimes it is possible in
> a nice way by encapsulating code that depends on the feature in one
> single file.
> 
> Sometimes it also requires some more thinking to make it that way (not
> as easy as adding ifdef).
> 
> I think if we aim to that, we can avoid this sort of problems.

By looking at all those CONFIG_IP_VS_IPV6. I'd suggest to abstract
generic layer 3 operations and thus move all IPv4 and IPv6 to the
corresponding files (eg. ipvs_l3proto_ipv4.c and
ipvs_l3proto_ipv6.c).