net: santize headers for iproute2

PATCH In review Stephen Hemminger 4 participants 11 messages Dec 21, 2007 – Dec 31, 2007

Subsystems (3)

Networking Network drivers TCP

lore

Share & tools

lore

Stephen Hemminger

author

2007-12-21 18:11 UTC #0 lore raw

PATCH 1/1 net: santize headers for iproute2

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 <[email protected]>

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

include/Kbuild +1 -0

@@ -4,5 +4,6 @@ header-y += sound/
 header-y += mtd/
 header-y += rdma/
 header-y += video/
+header-y += net/
 
 header-y += asm-$(ARCH)/

include/net/Kbuild new file +2 -0

@@ -0,0 +1,2 @@
+header-y += tcp_states.h
+header-y += veth.h

David Miller

maintainer

2007-12-25 06:05 UTC #1 lore raw

From: Stephen Hemminger <[email protected]>
Date: Fri, 21 Dec 2007 09:43:36 -0800

Quoted 5 lines

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 <[email protected]>

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.

Stephen Hemminger

2007-12-25 20:44 UTC #2 lore raw

Quoted 7 lines

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.

Signature · 1 line

Stephen Hemminger <[email protected]>

David Miller

maintainer

2007-12-25 23:02 UTC #3 lore raw

From: Stephen Hemminger <[email protected]>
Date: Tue, 25 Dec 2007 12:43:23 -0800

Quoted 4 lines

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.

Stephen Hemminger

2007-12-26 01:14 UTC #4 lore raw

Quoted 8 lines · On Tue, 25 Dec 2007 15:02:38 -0800 (PST) David Miller <[email protected]>…

From: Stephen Hemminger <[email protected]>
Date: Tue, 25 Dec 2007 12:43:23 -0800
Quoted 4 lines
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.

Quoted 7 lines

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:

include/linux/tcp.h +6 -1

@@ -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 <[email protected]>

David Miller

maintainer

2007-12-30 03:22 UTC #5 lore raw

From: Stephen Hemminger <[email protected]>
Date: Tue, 25 Dec 2007 17:13:10 -0800

Quoted 2 lines

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.

Stephen Hemminger

2007-12-31 18:57 UTC #6 lore raw

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.

Stephen Hemminger

author

2007-12-25 20:47 UTC #7 lore raw

PATCH 1/1 veth: move veth.h to include/linux

In reply to David Miller

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 <[email protected]>

drivers/net/veth.c +1 -1

@@ -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"

include/linux/veth.h new file +12 -0

@@ -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

include/net/veth.h deleted +0 ~~-12~~

@@ -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

include/linux/Kbuild +1 -0

@@ -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

Sam Ravnborg

2007-12-25 22:21 UTC #8 lore raw

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

Quoted 49 lines · 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 <[email protected]>

drivers/net/veth.c +1 -1

@@ -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"

include/linux/veth.h new file +12 -0

@@ -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

include/net/veth.h deleted +0 ~~-12~~

@@ -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

include/linux/Kbuild unifdef-y += unistd.h · line 348

Quoted 7 lines

include/linux/Kbuild +1 -0

@@ -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

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Stephen Hemminger

author

2007-12-26 01:15 UTC #9 lore raw

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

Quoted 61 lines · On Tue, 25 Dec 2007 23:23:35 +0100 Sam Ravnborg <[email protected]> wrote:

Quoted 49 lines · 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 <[email protected]>
drivers/net/veth.c +1 -1
@@ -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"
include/linux/veth.h new file +12 -0
@@ -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
include/net/veth.h deleted +0 ~~-12~~
@@ -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
include/linux/Kbuild unifdef-y += unistd.h · line 348
Quoted 7 lines
include/linux/Kbuild +1 -0
@@ -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.

Quoted 5 lines

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...

Signature · 1 line

Stephen Hemminger <[email protected]>

David Miller

maintainer

2007-12-26 01:19 UTC #10 lore raw

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

From: Stephen Hemminger <[email protected]>
Date: Tue, 25 Dec 2007 17:14:15 -0800

Quoted 6 lines

Quoted 3 lines · On Tue, 25 Dec 2007 23:23:35 +0100 Sam Ravnborg <[email protected]> wrote:
Someone will argue that you should use header-y += because the file has no conditionals removed by unifdef.

Quoted 7 lines

Quoted 5 lines
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.