[PATCH] net: santize headers for iproute2

[PATCH] net: santize headers for iproute2

[Stephen Hemminger]

iproute2 source uses headers that result from "make headers_install".
The header files net/tcp_states.h and net/veth.h are both being used
but are not processed. 

Signed-off-by: Stephen Hemminger <stephen.hemminger@vyatta.com>

---
This patch is not "urgent" but it would be good to get it in 2.6.24.

--- a/include/Kbuild	2007-12-21 09:33:36.000000000 -0800
+++ b/include/Kbuild	2007-12-21 09:33:42.000000000 -0800
@@ -4,5 +4,6 @@ header-y += sound/
 header-y += mtd/
 header-y += rdma/
 header-y += video/
+header-y += net/
 
 header-y += asm-$(ARCH)/
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ b/include/net/Kbuild	2007-12-21 09:32:20.000000000 -0800
@@ -0,0 +1,2 @@
+header-y += tcp_states.h
+header-y += veth.h

Re: [PATCH] net: santize headers for iproute2

[David Miller]

From: Stephen Hemminger <stephen.hemminger@vyatta.com>
Date: Fri, 21 Dec 2007 09:43:36 -0800

> iproute2 source uses headers that result from "make headers_install".
> The header files net/tcp_states.h and net/veth.h are both being used
> but are not processed. 
> 
> Signed-off-by: Stephen Hemminger <stephen.hemminger@vyatta.com>

The convention is to place user visible interfaces in header
files under include/linux/ and purely kernel internal bits
in header files under include/net/

Therefore exporting net/*.h headers is not right.

net/veth.h only defines user visible interfaces, so it
belongs under include/linux and added to include/linux/Kbuild,
and I would happily take a patch implementing that.

net/tcp_states.h is not movable to include/linux/ and thus to
userspace, it defines things that are present already in existing
userland header files.  If you look, <netinet/tcp.h> defines these
state values, as an enumeration.  If you need the TCPF_* flag bit
versions, sorry... you'll need to find some other way to get those
into userspace.

Re: [PATCH] net: santize headers for iproute2

[Stephen Hemminger]

> net/tcp_states.h is not movable to include/linux/ and thus to
> userspace, it defines things that are present already in existing
> userland header files.  If you look, <netinet/tcp.h> defines these
> state values, as an enumeration.  If you need the TCPF_* flag bit
> versions, sorry... you'll need to find some other way to get those
> into userspace.

The problem is that iproute ss.c needs linux/tcp.h to get the tcp_info
structure definition but linux/tcp.h has incompatible definitions with netinet/tcp.h. 
I propose that the enum be moved from net/tcp_states.h to linux/tcp.h.

-- 
Stephen Hemminger <stephen.hemminger@vyatta.com>

[PATCH] veth: move veth.h to include/linux

[Stephen Hemminger]

Move veth.h from net/ to linux/ since it is a user api, and
add it to user header processing Kbuild.

Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

--- a/drivers/net/veth.c	2007-12-25 12:30:56.000000000 -0800
+++ b/drivers/net/veth.c	2007-12-25 12:31:38.000000000 -0800
@@ -15,7 +15,7 @@
 
 #include <net/dst.h>
 #include <net/xfrm.h>
-#include <net/veth.h>
+#include <linux/veth.h>
 
 #define DRV_NAME	"veth"
 #define DRV_VERSION	"1.0"
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ b/include/linux/veth.h	2007-10-16 16:48:20.000000000 -0700
@@ -0,0 +1,12 @@
+#ifndef __NET_VETH_H_
+#define __NET_VETH_H_
+
+enum {
+	VETH_INFO_UNSPEC,
+	VETH_INFO_PEER,
+
+	__VETH_INFO_MAX
+#define VETH_INFO_MAX	(__VETH_INFO_MAX - 1)
+};
+
+#endif
--- a/include/net/veth.h	2007-12-25 12:33:49.000000000 -0800
+++ /dev/null	1970-01-01 00:00:00.000000000 +0000
@@ -1,12 +0,0 @@
-#ifndef __NET_VETH_H_
-#define __NET_VETH_H_
-
-enum {
-	VETH_INFO_UNSPEC,
-	VETH_INFO_PEER,
-
-	__VETH_INFO_MAX
-#define VETH_INFO_MAX	(__VETH_INFO_MAX - 1)
-};
-
-#endif
--- a/include/linux/Kbuild	2007-12-25 12:44:59.000000000 -0800
+++ b/include/linux/Kbuild	2007-12-25 12:45:31.000000000 -0800
@@ -342,6 +342,7 @@ unifdef-y += unistd.h
 unifdef-y += usbdevice_fs.h
 unifdef-y += user.h
 unifdef-y += utsname.h
+unifdef-y += veth.h
 unifdef-y += videodev2.h
 unifdef-y += videodev.h
 unifdef-y += virtio_config.h

Re: [PATCH] veth: move veth.h to include/linux

[Sam Ravnborg]

On Tue, Dec 25, 2007 at 12:46:56PM -0800, Stephen Hemminger wrote:
> Move veth.h from net/ to linux/ since it is a user api, and
> add it to user header processing Kbuild.
> 
> Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
> 
> --- a/drivers/net/veth.c	2007-12-25 12:30:56.000000000 -0800
> +++ b/drivers/net/veth.c	2007-12-25 12:31:38.000000000 -0800
> @@ -15,7 +15,7 @@
>  
>  #include <net/dst.h>
>  #include <net/xfrm.h>
> -#include <net/veth.h>
> +#include <linux/veth.h>
>  
>  #define DRV_NAME	"veth"
>  #define DRV_VERSION	"1.0"
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ b/include/linux/veth.h	2007-10-16 16:48:20.000000000 -0700
> @@ -0,0 +1,12 @@
> +#ifndef __NET_VETH_H_
> +#define __NET_VETH_H_
> +
> +enum {
> +	VETH_INFO_UNSPEC,
> +	VETH_INFO_PEER,
> +
> +	__VETH_INFO_MAX
> +#define VETH_INFO_MAX	(__VETH_INFO_MAX - 1)
> +};
> +
> +#endif
> --- a/include/net/veth.h	2007-12-25 12:33:49.000000000 -0800
> +++ /dev/null	1970-01-01 00:00:00.000000000 +0000
> @@ -1,12 +0,0 @@
> -#ifndef __NET_VETH_H_
> -#define __NET_VETH_H_
> -
> -enum {
> -	VETH_INFO_UNSPEC,
> -	VETH_INFO_PEER,
> -
> -	__VETH_INFO_MAX
> -#define VETH_INFO_MAX	(__VETH_INFO_MAX - 1)
> -};
> -
> -#endif
> --- a/include/linux/Kbuild	2007-12-25 12:44:59.000000000 -0800
> +++ b/include/linux/Kbuild	2007-12-25 12:45:31.000000000 -0800
> @@ -342,6 +342,7 @@ unifdef-y += unistd.h
>  unifdef-y += usbdevice_fs.h
>  unifdef-y += user.h
>  unifdef-y += utsname.h
> +unifdef-y += veth.h
>  unifdef-y += videodev2.h
>  unifdef-y += videodev.h
>  unifdef-y += virtio_config.h

Someone will argue that you should use header-y +=
because the file has no conditionals removed by unifdef.

My personal opinion is that we should kill header-y and
I had patches to greatly improve all this but they got 
lost by accident and I have not yet redone them.

	Sam


> --
> 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] net: santize headers for iproute2

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Tue, 25 Dec 2007 12:43:23 -0800

> The problem is that iproute ss.c needs linux/tcp.h to get the
> tcp_info structure definition but linux/tcp.h has incompatible
> definitions with netinet/tcp.h.  I propose that the enum be moved
> from net/tcp_states.h to linux/tcp.h.

This would be fragile, at best.

We need to solve this in a way such that both linux/tcp.h and
netinet/tcp.h can both be included at the same time by userland.

If we export linux/tcp.h to userspace, we have to make it play nice
with the existing header file namespace.

Re: [PATCH] net: santize headers for iproute2

[Stephen Hemminger]

On Tue, 25 Dec 2007 15:02:38 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@linux-foundation.org>
> Date: Tue, 25 Dec 2007 12:43:23 -0800
> 
> > The problem is that iproute ss.c needs linux/tcp.h to get the
> > tcp_info structure definition but linux/tcp.h has incompatible
> > definitions with netinet/tcp.h.  I propose that the enum be moved
> > from net/tcp_states.h to linux/tcp.h.
> 
> This would be fragile, at best.
> 
> We need to solve this in a way such that both linux/tcp.h and
> netinet/tcp.h can both be included at the same time by userland.
> 
> If we export linux/tcp.h to userspace, we have to make it play nice
> with the existing header file namespace.

Well changing netinet/tcp.h is just not a realistic proposition, it takes
too long to filter from glibc through distro's to be worth it. That means
fixing linux/tcp.h to not conflict.  How do you propose to resolve?

In file included from ss.c:37:
../include/linux/tcp.h:24: error: redefinition of ‘struct tcphdr’
../include/linux/tcp.h:106: error: nested redefinition of ‘enum tcp_ca_state’
../include/linux/tcp.h:106: error: redeclaration of ‘enum tcp_ca_state’
../include/linux/tcp.h:107: error: redeclaration of enumerator ‘TCP_CA_Open’
/usr/include/netinet/tcp.h:179: error: previous definition of ‘TCP_CA_Open’ was her

maybe:

--- a/include/linux/tcp.h	2007-12-25 17:09:26.000000000 -0800
+++ b/include/linux/tcp.h	2007-12-25 17:12:12.000000000 -0800
@@ -21,6 +21,9 @@
 #include <asm/byteorder.h>
 #include <linux/socket.h>
 
+#ifndef __KERNEL__
+#include <netinet/tcp.h>
+#else
 struct tcphdr {
 	__be16	source;
 	__be16	dest;
@@ -55,6 +58,7 @@ struct tcphdr {
 	__sum16	check;
 	__be16	urg_ptr;
 };
+#endif
 
 /*
  *	The union cast uses a gcc extension to avoid aliasing problems
@@ -102,6 +106,7 @@ enum { 
 #define TCPI_OPT_WSCALE		4
 #define TCPI_OPT_ECN		8
 
+#ifdef __KERNEL__
 enum tcp_ca_state
 {
 	TCP_CA_Open = 0,
@@ -115,6 +120,7 @@ enum tcp_ca_state
 	TCP_CA_Loss = 4
 #define TCPF_CA_Loss	(1<<TCP_CA_Loss)
 };
+#endif


-- 
Stephen Hemminger <stephen.hemminger@vyatta.com>

Re: [PATCH] veth: move veth.h to include/linux

[Stephen Hemminger]

On Tue, 25 Dec 2007 23:23:35 +0100
Sam Ravnborg <sam@ravnborg.org> wrote:

> On Tue, Dec 25, 2007 at 12:46:56PM -0800, Stephen Hemminger wrote:
> > Move veth.h from net/ to linux/ since it is a user api, and
> > add it to user header processing Kbuild.
> > 
> > Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
> > 
> > --- a/drivers/net/veth.c	2007-12-25 12:30:56.000000000 -0800
> > +++ b/drivers/net/veth.c	2007-12-25 12:31:38.000000000 -0800
> > @@ -15,7 +15,7 @@
> >  
> >  #include <net/dst.h>
> >  #include <net/xfrm.h>
> > -#include <net/veth.h>
> > +#include <linux/veth.h>
> >  
> >  #define DRV_NAME	"veth"
> >  #define DRV_VERSION	"1.0"
> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ b/include/linux/veth.h	2007-10-16 16:48:20.000000000 -0700
> > @@ -0,0 +1,12 @@
> > +#ifndef __NET_VETH_H_
> > +#define __NET_VETH_H_
> > +
> > +enum {
> > +	VETH_INFO_UNSPEC,
> > +	VETH_INFO_PEER,
> > +
> > +	__VETH_INFO_MAX
> > +#define VETH_INFO_MAX	(__VETH_INFO_MAX - 1)
> > +};
> > +
> > +#endif
> > --- a/include/net/veth.h	2007-12-25 12:33:49.000000000 -0800
> > +++ /dev/null	1970-01-01 00:00:00.000000000 +0000
> > @@ -1,12 +0,0 @@
> > -#ifndef __NET_VETH_H_
> > -#define __NET_VETH_H_
> > -
> > -enum {
> > -	VETH_INFO_UNSPEC,
> > -	VETH_INFO_PEER,
> > -
> > -	__VETH_INFO_MAX
> > -#define VETH_INFO_MAX	(__VETH_INFO_MAX - 1)
> > -};
> > -
> > -#endif
> > --- a/include/linux/Kbuild	2007-12-25 12:44:59.000000000 -0800
> > +++ b/include/linux/Kbuild	2007-12-25 12:45:31.000000000 -0800
> > @@ -342,6 +342,7 @@ unifdef-y += unistd.h
> >  unifdef-y += usbdevice_fs.h
> >  unifdef-y += user.h
> >  unifdef-y += utsname.h
> > +unifdef-y += veth.h
> >  unifdef-y += videodev2.h
> >  unifdef-y += videodev.h
> >  unifdef-y += virtio_config.h
> 
> Someone will argue that you should use header-y +=
> because the file has no conditionals removed by unifdef.
> 
> My personal opinion is that we should kill header-y and
> I had patches to greatly improve all this but they got 
> lost by accident and I have not yet redone them.
> 
> 	Sam

Someone might add kernel code later...

-- 
Stephen Hemminger <stephen.hemminger@vyatta.com>

Re: [PATCH] veth: move veth.h to include/linux

[David Miller]

From: Stephen Hemminger <stephen.hemminger@vyatta.com>
Date: Tue, 25 Dec 2007 17:14:15 -0800

> On Tue, 25 Dec 2007 23:23:35 +0100
> Sam Ravnborg <sam@ravnborg.org> wrote:
> 
> > Someone will argue that you should use header-y +=
> > because the file has no conditionals removed by unifdef.
> > 
> > My personal opinion is that we should kill header-y and
> > I had patches to greatly improve all this but they got 
> > lost by accident and I have not yet redone them.
> > 
> > 	Sam
> 
> Someone might add kernel code later...

I doubt it, I'll therefore modify your patch to use
header-y for now.

Re: [PATCH] net: santize headers for iproute2

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Tue, 25 Dec 2007 17:13:10 -0800

> Well changing netinet/tcp.h is just not a realistic proposition, it takes
> too long to filter from glibc through distro's to be worth it.

But you have to, the ifdef mess you are suggesting is so much
worse.

Look at the reason you even have to do add the ifdefs, it's because
lo' and behold the congestion control defines have already propagated
properly into netinet/tcp.h

And this alone proves that your argument against putting this stuff in
the right place has no basis in reality.

Please just submit a patch to the appropriate place to get the
interfaces you need into netinet/tcp.h included instead of crapping
all over the kernel headers.

In the interum you can put a special header into the iproute2
distribution to handle this until it's sorted.

Re: [PATCH] net: santize headers for iproute2

[Stephen Hemminger]

The Glib 2.7 version of netinet/tcp.h works for building iproute2,
but since iproute needs to build on older versions of glibc;
for now, I'll just put a corrected version of netinet/tcp.h in the iproute2
build tree.