[PATCH] xfrm6: move define/ifdef check order

[PATCH] xfrm6: move define/ifdef check order

[Randy.Dunlap]

From: Randy Dunlap <rdunlap@xenotime.net>

The first check for #ifdef XFRM6_TUNNEL_SPI_MAGIC needs to come after
the optional #define of it, otherwise the variable won't be there
for the rest of the code to use.

Shouldn't that CONFIG_IPV6_XFRM6_TUNNEL_DEBUG be part of Kconfig
or have its name changed?

Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
---
 net/ipv6/xfrm6_tunnel.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

--- linux-2617-g20.orig/net/ipv6/xfrm6_tunnel.c
+++ linux-2617-g20/net/ipv6/xfrm6_tunnel.c
@@ -52,6 +52,10 @@
 # define X6TPRINTK3	X6TNOPRINTK
 #endif
 
+#ifdef CONFIG_IPV6_XFRM6_TUNNEL_DEBUG
+# define XFRM6_TUNNEL_SPI_MAGIC 0xdeadbeef
+#endif
+
 /*
  * xfrm_tunnel_spi things are for allocating unique id ("spi") 
  * per xfrm_address_t.
@@ -67,10 +71,6 @@ struct xfrm6_tunnel_spi {
 #endif
 };
 
-#ifdef CONFIG_IPV6_XFRM6_TUNNEL_DEBUG
-# define XFRM6_TUNNEL_SPI_MAGIC 0xdeadbeef
-#endif
-
 static DEFINE_RWLOCK(xfrm6_tunnel_spi_lock);
 
 static u32 xfrm6_tunnel_spi;


---

Re: [PATCH] xfrm6: move define/ifdef check order

[Nicolas DICHTEL]

I point out this issue a long time ago ...
http://marc.theaimsgroup.com/?l=linux-netdev&m=113032946908126&w=2


Regards,
Nicolas

Randy.Dunlap a écrit :
> From: Randy Dunlap <rdunlap@xenotime.net>
>
> The first check for #ifdef XFRM6_TUNNEL_SPI_MAGIC needs to come after
> the optional #define of it, otherwise the variable won't be there
> for the rest of the code to use.
>
> Shouldn't that CONFIG_IPV6_XFRM6_TUNNEL_DEBUG be part of Kconfig
> or have its name changed?
>
> Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
> ---
>  net/ipv6/xfrm6_tunnel.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
>
> --- linux-2617-g20.orig/net/ipv6/xfrm6_tunnel.c
> +++ linux-2617-g20/net/ipv6/xfrm6_tunnel.c
> @@ -52,6 +52,10 @@
>  # define X6TPRINTK3	X6TNOPRINTK
>  #endif
>  
> +#ifdef CONFIG_IPV6_XFRM6_TUNNEL_DEBUG
> +# define XFRM6_TUNNEL_SPI_MAGIC 0xdeadbeef
> +#endif
> +
>  /*
>   * xfrm_tunnel_spi things are for allocating unique id ("spi") 
>   * per xfrm_address_t.
> @@ -67,10 +71,6 @@ struct xfrm6_tunnel_spi {
>  #endif
>  };
>  
> -#ifdef CONFIG_IPV6_XFRM6_TUNNEL_DEBUG
> -# define XFRM6_TUNNEL_SPI_MAGIC 0xdeadbeef
> -#endif
> -
>  static DEFINE_RWLOCK(xfrm6_tunnel_spi_lock);
>  
>  static u32 xfrm6_tunnel_spi;
>
>
> ---
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>

Re: [PATCH] xfrm6: move define/ifdef check order

[David Miller]

From: "Randy.Dunlap" <rdunlap@xenotime.net>
Date: Sun, 2 Jul 2006 14:36:11 -0700

> From: Randy Dunlap <rdunlap@xenotime.net>
> 
> The first check for #ifdef XFRM6_TUNNEL_SPI_MAGIC needs to come after
> the optional #define of it, otherwise the variable won't be there
> for the rest of the code to use.
> 
> Shouldn't that CONFIG_IPV6_XFRM6_TUNNEL_DEBUG be part of Kconfig
> or have its name changed?
> 
> Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>

Yes, indeed.  Nothing named CONFIG_* should be defined explicitly
by the source files, only through Kconfig.

I would rather address that than apply this patch.

Thanks Randy.

xfrm6: move define/ifdef check order and add a Kconfig option

[Randy.Dunlap]

On Mon, 03 Jul 2006 19:46:55 -0700 (PDT) David Miller wrote:

> Yes, indeed.  Nothing named CONFIG_* should be defined explicitly
> by the source files, only through Kconfig.
> 
> I would rather address that than apply this patch.

by adding a Kconfig option or not?  This patch adds one.


From: Randy Dunlap <rdunlap@xenotime.net>

Convert CONFIG_IPV6_XFRM6_TUNNEL_DEBUG (not a Kconfig option)
to CONFIG_INET6_XFRM6_TUNNEL_DEBUG, which is a Kconfig option.

Then #define XFRM6_TUNNEL_SPI_MAGIC before it is first used.

Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
---
 net/ipv6/Kconfig        |    6 ++++++
 net/ipv6/xfrm6_tunnel.c |    7 ++-----
 2 files changed, 8 insertions(+), 5 deletions(-)

--- linux-2617-g20.orig/net/ipv6/xfrm6_tunnel.c
+++ linux-2617-g20/net/ipv6/xfrm6_tunnel.c
@@ -31,8 +31,9 @@
 #include <linux/icmpv6.h>
 #include <linux/mutex.h>
 
-#ifdef CONFIG_IPV6_XFRM6_TUNNEL_DEBUG
+#ifdef CONFIG_INET6_XFRM_TUNNEL_DEBUG
 # define X6TDEBUG	3
+# define XFRM6_TUNNEL_SPI_MAGIC 0xdeadbeef
 #else
 # define X6TDEBUG	1
 #endif
@@ -67,10 +68,6 @@ struct xfrm6_tunnel_spi {
 #endif
 };
 
-#ifdef CONFIG_IPV6_XFRM6_TUNNEL_DEBUG
-# define XFRM6_TUNNEL_SPI_MAGIC 0xdeadbeef
-#endif
-
 static DEFINE_RWLOCK(xfrm6_tunnel_spi_lock);
 
 static u32 xfrm6_tunnel_spi;
--- linux-2617-g20.orig/net/ipv6/Kconfig
+++ linux-2617-g20/net/ipv6/Kconfig
@@ -106,6 +106,12 @@ config INET6_TUNNEL
 	tristate
 	default n
 
+config INET6_XFRM_TUNNEL_DEBUG
+	bool "IPv6: xfrm tunnel debug"
+	depends on INET6_XFRM_TUNNEL
+	---help---
+	  Enable IPv6 tunnel debug code.
+
 config INET6_XFRM_MODE_TRANSPORT
 	tristate "IPv6: IPsec transport mode"
 	depends on IPV6



---

Re: xfrm6: move define/ifdef check order and add a Kconfig option

[David Miller]

From: "Randy.Dunlap" <rdunlap@xenotime.net>
Date: Mon, 3 Jul 2006 21:42:33 -0700

> On Mon, 03 Jul 2006 19:46:55 -0700 (PDT) David Miller wrote:
> 
> > Yes, indeed.  Nothing named CONFIG_* should be defined explicitly
> > by the source files, only through Kconfig.
> > 
> > I would rather address that than apply this patch.
> 
> by adding a Kconfig option or not?  This patch adds one.

After some more consideration, I'm just going to blow away this
debugging code altogether.  It's nothing more than primitive function
call tracing.  It does things like print object pointers which have
no meaning without context.  The improper placement of the definition
of XFRM6_TUNNEL_SPI_MAGIC makes this debugging code even more dubious.

Thanks for bringing this to my attention Randy.

diff-tree a922ba5510530ae8e3c60edc85c56f72347a3c86 (from e9e9290f5c85887baf1123a36ec9fdf56a10cf4b)
Author: David S. Miller <davem@sunset.davemloft.net>
Date:   Mon Jul 24 13:49:06 2006 -0700

    [IPV6] xfrm6_tunnel: Delete debugging code.
    
    It doesn't compile, and it's dubious in several regards:
    
    1) is enabled by non-Kconfig controlled CONFIG_* value
       (noted by Randy Dunlap)
    2) XFRM6_TUNNEL_SPI_MAGIC is defined after it's first use
    3) the debugging messages print object pointer addresses
       which have no meaning without context
    
    So let's just get rid of it.
    
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/ipv6/xfrm6_tunnel.c b/net/ipv6/xfrm6_tunnel.c
index 6b44fe8..c8f9369 100644
--- a/net/ipv6/xfrm6_tunnel.c
+++ b/net/ipv6/xfrm6_tunnel.c
@@ -31,27 +31,6 @@
 #include <linux/icmpv6.h>
 #include <linux/mutex.h>
 
-#ifdef CONFIG_IPV6_XFRM6_TUNNEL_DEBUG
-# define X6TDEBUG	3
-#else
-# define X6TDEBUG	1
-#endif
-
-#define X6TPRINTK(fmt, args...)		printk(fmt, ## args)
-#define X6TNOPRINTK(fmt, args...)	do { ; } while(0)
-
-#if X6TDEBUG >= 1
-# define X6TPRINTK1	X6TPRINTK
-#else
-# define X6TPRINTK1	X6TNOPRINTK
-#endif
-
-#if X6TDEBUG >= 3
-# define X6TPRINTK3	X6TPRINTK
-#else
-# define X6TPRINTK3	X6TNOPRINTK
-#endif
-
 /*
  * xfrm_tunnel_spi things are for allocating unique id ("spi") 
  * per xfrm_address_t.
@@ -62,15 +41,8 @@ struct xfrm6_tunnel_spi {
 	xfrm_address_t addr;
 	u32 spi;
 	atomic_t refcnt;
-#ifdef XFRM6_TUNNEL_SPI_MAGIC
-	u32 magic;
-#endif
 };
 
-#ifdef CONFIG_IPV6_XFRM6_TUNNEL_DEBUG
-# define XFRM6_TUNNEL_SPI_MAGIC 0xdeadbeef
-#endif
-
 static DEFINE_RWLOCK(xfrm6_tunnel_spi_lock);
 
 static u32 xfrm6_tunnel_spi;
@@ -86,43 +58,15 @@ static kmem_cache_t *xfrm6_tunnel_spi_km
 static struct hlist_head xfrm6_tunnel_spi_byaddr[XFRM6_TUNNEL_SPI_BYADDR_HSIZE];
 static struct hlist_head xfrm6_tunnel_spi_byspi[XFRM6_TUNNEL_SPI_BYSPI_HSIZE];
 
-#ifdef XFRM6_TUNNEL_SPI_MAGIC
-static int x6spi_check_magic(const struct xfrm6_tunnel_spi *x6spi,
-			     const char *name)
-{
-	if (unlikely(x6spi->magic != XFRM6_TUNNEL_SPI_MAGIC)) {
-		X6TPRINTK3(KERN_DEBUG "%s(): x6spi object "
-				      "at %p has corrupted magic %08x "
-				      "(should be %08x)\n",
-			   name, x6spi, x6spi->magic, XFRM6_TUNNEL_SPI_MAGIC);
-		return -1;
-	}
-	return 0;
-}
-#else
-static int inline x6spi_check_magic(const struct xfrm6_tunnel_spi *x6spi,
-				    const char *name)
-{
-	return 0;
-}
-#endif
-
-#define X6SPI_CHECK_MAGIC(x6spi) x6spi_check_magic((x6spi), __FUNCTION__)
-
-
 static unsigned inline xfrm6_tunnel_spi_hash_byaddr(xfrm_address_t *addr)
 {
 	unsigned h;
 
-	X6TPRINTK3(KERN_DEBUG "%s(addr=%p)\n", __FUNCTION__, addr);
-
 	h = addr->a6[0] ^ addr->a6[1] ^ addr->a6[2] ^ addr->a6[3];
 	h ^= h >> 16;
 	h ^= h >> 8;
 	h &= XFRM6_TUNNEL_SPI_BYADDR_HSIZE - 1;
 
-	X6TPRINTK3(KERN_DEBUG "%s() = %u\n", __FUNCTION__, h);
-
 	return h;
 }
 
@@ -136,19 +80,13 @@ static int xfrm6_tunnel_spi_init(void)
 {
 	int i;
 
-	X6TPRINTK3(KERN_DEBUG "%s()\n", __FUNCTION__);
-
 	xfrm6_tunnel_spi = 0;
 	xfrm6_tunnel_spi_kmem = kmem_cache_create("xfrm6_tunnel_spi",
 						  sizeof(struct xfrm6_tunnel_spi),
 						  0, SLAB_HWCACHE_ALIGN,
 						  NULL, NULL);
-	if (!xfrm6_tunnel_spi_kmem) {
-		X6TPRINTK1(KERN_ERR
-			   "%s(): failed to allocate xfrm6_tunnel_spi_kmem\n",
-		           __FUNCTION__);
+	if (!xfrm6_tunnel_spi_kmem)
 		return -ENOMEM;
-	}
 
 	for (i = 0; i < XFRM6_TUNNEL_SPI_BYADDR_HSIZE; i++)
 		INIT_HLIST_HEAD(&xfrm6_tunnel_spi_byaddr[i]);
@@ -161,22 +99,16 @@ static void xfrm6_tunnel_spi_fini(void)
 {
 	int i;
 
-	X6TPRINTK3(KERN_DEBUG "%s()\n", __FUNCTION__);
-
 	for (i = 0; i < XFRM6_TUNNEL_SPI_BYADDR_HSIZE; i++) {
 		if (!hlist_empty(&xfrm6_tunnel_spi_byaddr[i]))
-			goto err;
+			return;
 	}
 	for (i = 0; i < XFRM6_TUNNEL_SPI_BYSPI_HSIZE; i++) {
 		if (!hlist_empty(&xfrm6_tunnel_spi_byspi[i]))
-			goto err;
+			return;
 	}
 	kmem_cache_destroy(xfrm6_tunnel_spi_kmem);
 	xfrm6_tunnel_spi_kmem = NULL;
-	return;
-err:
-	X6TPRINTK1(KERN_ERR "%s(): table is not empty\n", __FUNCTION__);
-	return;
 }
 
 static struct xfrm6_tunnel_spi *__xfrm6_tunnel_spi_lookup(xfrm_address_t *saddr)
@@ -184,19 +116,13 @@ static struct xfrm6_tunnel_spi *__xfrm6_
 	struct xfrm6_tunnel_spi *x6spi;
 	struct hlist_node *pos;
 
-	X6TPRINTK3(KERN_DEBUG "%s(saddr=%p)\n", __FUNCTION__, saddr);
-
 	hlist_for_each_entry(x6spi, pos,
 			     &xfrm6_tunnel_spi_byaddr[xfrm6_tunnel_spi_hash_byaddr(saddr)],
 			     list_byaddr) {
-		if (memcmp(&x6spi->addr, saddr, sizeof(x6spi->addr)) == 0) {
-			X6SPI_CHECK_MAGIC(x6spi);
-			X6TPRINTK3(KERN_DEBUG "%s() = %p(%u)\n", __FUNCTION__, x6spi, x6spi->spi);
+		if (memcmp(&x6spi->addr, saddr, sizeof(x6spi->addr)) == 0)
 			return x6spi;
-		}
 	}
 
-	X6TPRINTK3(KERN_DEBUG "%s() = NULL(0)\n", __FUNCTION__);
 	return NULL;
 }
 
@@ -205,8 +131,6 @@ u32 xfrm6_tunnel_spi_lookup(xfrm_address
 	struct xfrm6_tunnel_spi *x6spi;
 	u32 spi;
 
-	X6TPRINTK3(KERN_DEBUG "%s(saddr=%p)\n", __FUNCTION__, saddr);
-
 	read_lock_bh(&xfrm6_tunnel_spi_lock);
 	x6spi = __xfrm6_tunnel_spi_lookup(saddr);
 	spi = x6spi ? x6spi->spi : 0;
@@ -223,8 +147,6 @@ static u32 __xfrm6_tunnel_alloc_spi(xfrm
 	struct hlist_node *pos;
 	unsigned index;
 
-	X6TPRINTK3(KERN_DEBUG "%s(saddr=%p)\n", __FUNCTION__, saddr);
-
 	if (xfrm6_tunnel_spi < XFRM6_TUNNEL_SPI_MIN ||
 	    xfrm6_tunnel_spi >= XFRM6_TUNNEL_SPI_MAX)
 		xfrm6_tunnel_spi = XFRM6_TUNNEL_SPI_MIN;
@@ -258,18 +180,10 @@ try_next_2:;
 	spi = 0;
 	goto out;
 alloc_spi:
-	X6TPRINTK3(KERN_DEBUG "%s(): allocate new spi for " NIP6_FMT "\n",
-			      __FUNCTION__, 
-			      NIP6(*(struct in6_addr *)saddr));
 	x6spi = kmem_cache_alloc(xfrm6_tunnel_spi_kmem, SLAB_ATOMIC);
-	if (!x6spi) {
-		X6TPRINTK1(KERN_ERR "%s(): kmem_cache_alloc() failed\n", 
-			   __FUNCTION__);
+	if (!x6spi)
 		goto out;
-	}
-#ifdef XFRM6_TUNNEL_SPI_MAGIC
-	x6spi->magic = XFRM6_TUNNEL_SPI_MAGIC;
-#endif
+
 	memcpy(&x6spi->addr, saddr, sizeof(x6spi->addr));
 	x6spi->spi = spi;
 	atomic_set(&x6spi->refcnt, 1);
@@ -278,9 +192,7 @@ alloc_spi:
 
 	index = xfrm6_tunnel_spi_hash_byaddr(saddr);
 	hlist_add_head(&x6spi->list_byaddr, &xfrm6_tunnel_spi_byaddr[index]);
-	X6SPI_CHECK_MAGIC(x6spi);
 out:
-	X6TPRINTK3(KERN_DEBUG "%s() = %u\n", __FUNCTION__, spi);
 	return spi;
 }
 
@@ -289,8 +201,6 @@ u32 xfrm6_tunnel_alloc_spi(xfrm_address_
 	struct xfrm6_tunnel_spi *x6spi;
 	u32 spi;
 
-	X6TPRINTK3(KERN_DEBUG "%s(saddr=%p)\n", __FUNCTION__, saddr);
-
 	write_lock_bh(&xfrm6_tunnel_spi_lock);
 	x6spi = __xfrm6_tunnel_spi_lookup(saddr);
 	if (x6spi) {
@@ -300,8 +210,6 @@ u32 xfrm6_tunnel_alloc_spi(xfrm_address_
 		spi = __xfrm6_tunnel_alloc_spi(saddr);
 	write_unlock_bh(&xfrm6_tunnel_spi_lock);
 
-	X6TPRINTK3(KERN_DEBUG "%s() = %u\n", __FUNCTION__, spi);
-
 	return spi;
 }
 
@@ -312,8 +220,6 @@ void xfrm6_tunnel_free_spi(xfrm_address_
 	struct xfrm6_tunnel_spi *x6spi;
 	struct hlist_node *pos, *n;
 
-	X6TPRINTK3(KERN_DEBUG "%s(saddr=%p)\n", __FUNCTION__, saddr);
-
 	write_lock_bh(&xfrm6_tunnel_spi_lock);
 
 	hlist_for_each_entry_safe(x6spi, pos, n, 
@@ -321,12 +227,6 @@ void xfrm6_tunnel_free_spi(xfrm_address_
 				  list_byaddr)
 	{
 		if (memcmp(&x6spi->addr, saddr, sizeof(x6spi->addr)) == 0) {
-			X6TPRINTK3(KERN_DEBUG "%s(): x6spi object for " NIP6_FMT 
-					      " found at %p\n",
-				   __FUNCTION__, 
-				   NIP6(*(struct in6_addr *)saddr),
-				   x6spi);
-			X6SPI_CHECK_MAGIC(x6spi);
 			if (atomic_dec_and_test(&x6spi->refcnt)) {
 				hlist_del(&x6spi->list_byaddr);
 				hlist_del(&x6spi->list_byspi);
@@ -377,20 +277,14 @@ static int xfrm6_tunnel_err(struct sk_bu
 		case ICMPV6_ADDR_UNREACH:
 		case ICMPV6_PORT_UNREACH:
 		default:
-			X6TPRINTK3(KERN_DEBUG
-				   "xfrm6_tunnel: Destination Unreach.\n");
 			break;
 		}
 		break;
 	case ICMPV6_PKT_TOOBIG:
-			X6TPRINTK3(KERN_DEBUG 
-				   "xfrm6_tunnel: Packet Too Big.\n");
 		break;
 	case ICMPV6_TIME_EXCEED:
 		switch (code) {
 		case ICMPV6_EXC_HOPLIMIT:
-			X6TPRINTK3(KERN_DEBUG
-				   "xfrm6_tunnel: Too small Hoplimit.\n");
 			break;
 		case ICMPV6_EXC_FRAGTIME:
 		default: 
@@ -447,22 +341,14 @@ static struct xfrm6_tunnel xfrm6_tunnel_
 
 static int __init xfrm6_tunnel_init(void)
 {
-	X6TPRINTK3(KERN_DEBUG "%s()\n", __FUNCTION__);
-
-	if (xfrm_register_type(&xfrm6_tunnel_type, AF_INET6) < 0) {
-		X6TPRINTK1(KERN_ERR
-			   "xfrm6_tunnel init: can't add xfrm type\n");
+	if (xfrm_register_type(&xfrm6_tunnel_type, AF_INET6) < 0)
 		return -EAGAIN;
-	}
+
 	if (xfrm6_tunnel_register(&xfrm6_tunnel_handler)) {
-		X6TPRINTK1(KERN_ERR
-			   "xfrm6_tunnel init(): can't add handler\n");
 		xfrm_unregister_type(&xfrm6_tunnel_type, AF_INET6);
 		return -EAGAIN;
 	}
 	if (xfrm6_tunnel_spi_init() < 0) {
-		X6TPRINTK1(KERN_ERR
-			   "xfrm6_tunnel init: failed to initialize spi\n");
 		xfrm6_tunnel_deregister(&xfrm6_tunnel_handler);
 		xfrm_unregister_type(&xfrm6_tunnel_type, AF_INET6);
 		return -EAGAIN;
@@ -472,15 +358,9 @@ static int __init xfrm6_tunnel_init(void
 
 static void __exit xfrm6_tunnel_fini(void)
 {
-	X6TPRINTK3(KERN_DEBUG "%s()\n", __FUNCTION__);
-
 	xfrm6_tunnel_spi_fini();
-	if (xfrm6_tunnel_deregister(&xfrm6_tunnel_handler))
-		X6TPRINTK1(KERN_ERR 
-			   "xfrm6_tunnel close: can't remove handler\n");
-	if (xfrm_unregister_type(&xfrm6_tunnel_type, AF_INET6) < 0)
-		X6TPRINTK1(KERN_ERR
-			   "xfrm6_tunnel close: can't remove xfrm type\n");
+	xfrm6_tunnel_deregister(&xfrm6_tunnel_handler);
+	xfrm_unregister_type(&xfrm6_tunnel_type, AF_INET6);
 }
 
 module_init(xfrm6_tunnel_init);