[PATCH] wireless.h: improve userland include-ability

[PATCH] wireless.h: improve userland include-ability

[John W. Linville]

From: Kirill A. Shutemov <kirill@shutemov.name>

This patch partially reverts commit 2218228392080f0ca2fc2974604e79f57b12c436
("Make linux/wireless.h be able to compile") while still making it
easier to include wireless.h in userland apps.

Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 include/linux/wireless.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/include/linux/wireless.h b/include/linux/wireless.h
index 0a9b5b4..9c0b89c 100644
--- a/include/linux/wireless.h
+++ b/include/linux/wireless.h
@@ -70,8 +70,14 @@
 /***************************** INCLUDES *****************************/
 
 #include <linux/types.h>		/* for __u* and __s* typedefs */
+
+/* This header is used in user-space, therefore need to be sanitised
+ * for that purpose. Those includes are usually not compatible with glibc.
+ * To know which includes to use in user-space, check iwlib.h. */
+#ifdef __KERNEL__
 #include <linux/socket.h>		/* for "struct sockaddr" et al	*/
 #include <linux/if.h>			/* for IFNAMSIZ and co... */
+#endif	/* __KERNEL__ */
 
 /***************************** VERSION *****************************/
 /*
-- 
1.5.5.1

Re: [PATCH] wireless.h: improve userland include-ability

[David Miller]

From: "John W. Linville" <linville@tuxdriver.com>
Date: Tue,  3 Jun 2008 13:51:37 -0400

> From: Kirill A. Shutemov <kirill@shutemov.name>
> 
> This patch partially reverts commit 2218228392080f0ca2fc2974604e79f57b12c436
> ("Make linux/wireless.h be able to compile") while still making it
> easier to include wireless.h in userland apps.
> 
> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
> Signed-off-by: John W. Linville <linville@tuxdriver.com>

I not thrilled about this patch.

This means the user has to figure out what include file to add
to get IFNAMSIZ and things like that which wireless.h needs.

That is non-intuitive and magic.  These headers should be entirely
self-contained so that if you include linux/wireless.h it should
just work and there should be no dependencies not handlind by
wireless.h doing the necessary includes.

+/* This header is used in user-space, therefore need to be sanitised
+ * for that purpose. Those includes are usually not compatible with glibc.
+ * To know which includes to use in user-space, check iwlib.h. */

Yeah, go check so me magic userspace tool header to see what magic is
needed just to include a core networking header file correctly.

No, thanks.

Re: [PATCH] wireless.h: improve userland include-ability

[David Woodhouse]

On Tue, 2008-06-03 at 12:05 -0700, David Miller wrote:
> Yeah, go check so me magic userspace tool header to see what magic is
> needed just to include a core networking header file correctly.
> 
> No, thanks.

Just to clarify: this is only a problem for userspace, and you're
objecting to that? Just including <linux/wireless.h> within the kernel
will continue to work.

We've traditionally got away with saying 'caveat emptor' when userspace
includes kernel headers -- you _have_ to include the right
prerequisites, because the kernel doesn't do it for you.

I'm happy enough to change that, but it means seeing stuff like...
	#ifndef __KERNEL__
	#include <sys/foo.h>
	#endif 
... in kernel headers. Is that what you're intending, or am I
misunderstanding your objection?

I believe that the main reason for this patch was that <linux/socket.h>
and <linux/if.h> both have problems when you include them in the same C
file as the 'proper' glibc equivalent? Is that something we can address,
instead of just dropping those includes?

-- 
dwmw2

Re: [PATCH] wireless.h: improve userland include-ability

[Kirill A. Shutemov]

[-- Attachment #1: Type: text/plain, Size: 1752 bytes --]

On Wed, Jun 04, 2008 at 02:52:09PM +0100, David Woodhouse wrote:
> On Tue, 2008-06-03 at 12:05 -0700, David Miller wrote:
> > Yeah, go check so me magic userspace tool header to see what magic is
> > needed just to include a core networking header file correctly.
> > 
> > No, thanks.
> 
> Just to clarify: this is only a problem for userspace, and you're
> objecting to that? Just including <linux/wireless.h> within the kernel
> will continue to work.
> 
> We've traditionally got away with saying 'caveat emptor' when userspace
> includes kernel headers -- you _have_ to include the right
> prerequisites, because the kernel doesn't do it for you.
> 
> I'm happy enough to change that, but it means seeing stuff like...
> 	#ifndef __KERNEL__
> 	#include <sys/foo.h>
> 	#endif 
> ... in kernel headers. Is that what you're intending, or am I
> misunderstanding your objection?
> 
> I believe that the main reason for this patch was that <linux/socket.h>
> and <linux/if.h> both have problems when you include them in the same C
> file as the 'proper' glibc equivalent? Is that something we can address,
> instead of just dropping those includes?

What do you think about hack linke this:

#include <linux/types.h>                /* for __u* and __s* typedefs */
#if defined(__KERNEL__) || !defined(_SYS_SOCKET_H)
#include <linux/socket.h>               /* for "struct sockaddr" et al  */
#endif
#if defined(__KERNEL__) || !defined(_NET_IF_H)
#include <linux/if.h>                   /* for IFNAMSIZ and co... */
#endif

It avoids conflict between glibc's and kernel's headers if glibc's headers 
include first.

-- 
Regards,  Kirill A. Shutemov
 + Belarus, Minsk
 + ALT Linux Team, http://www.altlinux.com/

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] wireless.h: improve userland include-ability

[David Woodhouse]

On Wed, 2008-06-04 at 17:23 +0300, Kirill A. Shutemov wrote:
> What do you think about hack like this:
> 
> #include <linux/types.h>                /* for __u* and __s* typedefs */
> #if defined(__KERNEL__) || !defined(_SYS_SOCKET_H)
> #include <linux/socket.h>               /* for "struct sockaddr" et al  */
> #endif
> #if defined(__KERNEL__) || !defined(_NET_IF_H)
> #include <linux/if.h>                   /* for IFNAMSIZ and co... */
> #endif
> 
> It avoids conflict between glibc's and kernel's headers if glibc's headers 
> include first.

Anything which is dependent on ordering just makes matters worse, imho.

-- 
dwmw2

Re: [PATCH] wireless.h: improve userland include-ability

[David Woodhouse]

On Wed, 2008-06-04 at 15:26 +0100, David Woodhouse wrote:
> On Wed, 2008-06-04 at 17:23 +0300, Kirill A. Shutemov wrote:
> > What do you think about hack like this:
> > 
> > #include <linux/types.h>                /* for __u* and __s* typedefs */
> > #if defined(__KERNEL__) || !defined(_SYS_SOCKET_H)
> > #include <linux/socket.h>               /* for "struct sockaddr" et al  */
> > #endif
> > #if defined(__KERNEL__) || !defined(_NET_IF_H)
> > #include <linux/if.h>                   /* for IFNAMSIZ and co... */
> > #endif
> > 
> > It avoids conflict between glibc's and kernel's headers if glibc's headers 
> > include first.
> 
> Anything which is dependent on ordering just makes matters worse, imho.

This kind of thing might work. It think it's fine as long as _some_
userspace header gets included first (which then includes <features.h>
and thus defines __KERNEL_STRICT_NAMES.

#ifndef __KERNEL__
#include <sys/socket.h>
#endif
#include <linux/types.h>
#include <linux/socket.h>
#include <linux/if.h>

-- 
dwmw2

Re: [PATCH] wireless.h: improve userland include-ability

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Wed, 04 Jun 2008 14:52:09 +0100

> On Tue, 2008-06-03 at 12:05 -0700, David Miller wrote:
> > Yeah, go check so me magic userspace tool header to see what magic is
> > needed just to include a core networking header file correctly.
> > 
> > No, thanks.
> 
> Just to clarify: this is only a problem for userspace, and you're
> objecting to that?

Yes.

> I'm happy enough to change that, but it means seeing stuff like...
> 	#ifndef __KERNEL__
> 	#include <sys/foo.h>
> 	#endif 
> ... in kernel headers. Is that what you're intending, or am I
> misunderstanding your objection?

I think this kind of construct shouldn't be necessary in the first
place.

> I believe that the main reason for this patch was that <linux/socket.h>
> and <linux/if.h> both have problems when you include them in the same C
> file as the 'proper' glibc equivalent? Is that something we can address,
> instead of just dropping those includes?

You will need to find a way to make it work cleanly if you want
me to take the change in :-)

Re: [PATCH] wireless.h: improve userland include-ability

[David Woodhouse]

On Wed, 2008-06-04 at 08:02 -0700, David Miller wrote:
> You will need to find a way to make it work cleanly if you want
> me to take the change in :-)

I believe the fundamental issue is that we have to include <features.h>
before we include <linux/types.h>, because of the way that
__KERNEL_STRICT_NAMES is handled.

Since fairly much any userspace header will include the former, while
fairly much any kernel header will include the latter, that basically
means the rule is "include some userspace header before kernel headers".

If we really want to fix _that_, perhaps we could observe that glibc's
<features.h> actually has the following construct:

	#undef  __KERNEL_STRICT_NAMES
	/* Suppress kernel-name space pollution unless user expressedly
	   asks for it.  */
	#ifndef _LOOSE_KERNEL_NAMES
	# define __KERNEL_STRICT_NAMES
	#endif

... and we could make the kernel's own headers act on
_LOOSE_KERNEL_NAMES directly, instead of the absence of
__KERNEL_STRICT_NAMES?

We also have both <linux/if.h> and <net/if.h> defining struct ifmap,
struct ifreq and struct ifconf. Should those be hidden in #ifdef
__KERNEL__ (or _LOOSE_KERNEL_NAMES) too?

diff --git a/include/linux/types.h b/include/linux/types.h
index d4a9ce6..3edcda5 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -11,7 +11,7 @@
 #include <linux/posix_types.h>
 #include <asm/types.h>
 
-#ifndef __KERNEL_STRICT_NAMES
+#if defined(__KERNEL__) || defined(_LOOSE_KERNEL_NAMES)
 
 typedef __u32 __kernel_dev_t;
 
@@ -159,7 +159,7 @@ typedef unsigned long blkcnt_t;
 #define pgoff_t unsigned long
 #endif
 
-#endif /* __KERNEL_STRICT_NAMES */
+#endif /* !_LOOSE_KERNEL_NAMES */
 
 /*
  * Below are truly Linux-specific types that should never collide with
diff --git a/include/asm-arm/statfs.h b/include/asm-arm/statfs.h
index a02e6a8..9e54d51 100644
--- a/include/asm-arm/statfs.h
+++ b/include/asm-arm/statfs.h
@@ -1,7 +1,7 @@
 #ifndef _ASMARM_STATFS_H
 #define _ASMARM_STATFS_H
 
-#ifndef __KERNEL_STRICT_NAMES
+#if defined(__KERNEL__) || defined(_LOOSE_KERNEL_NAMES)
 # include <linux/types.h>
 typedef __kernel_fsid_t	fsid_t;
 #endif
diff --git a/include/asm-generic/statfs.h b/include/asm-generic/statfs.h
index 1d01043..bb7195a 100644
--- a/include/asm-generic/statfs.h
+++ b/include/asm-generic/statfs.h
@@ -1,7 +1,7 @@
 #ifndef _GENERIC_STATFS_H
 #define _GENERIC_STATFS_H
 
-#ifndef __KERNEL_STRICT_NAMES
+#if defined(__KERNEL__) || defined(_LOOSE_KERNEL_NAMES)
 # include <linux/types.h>
 typedef __kernel_fsid_t	fsid_t;
 #endif
diff --git a/include/asm-ia64/statfs.h b/include/asm-ia64/statfs.h
index 8110979..0f340c0 100644
--- a/include/asm-ia64/statfs.h
+++ b/include/asm-ia64/statfs.h
@@ -8,7 +8,7 @@
  *	David Mosberger-Tang <davidm@hpl.hp.com>, Hewlett-Packard Co
  */
 
-#ifndef __KERNEL_STRICT_NAMES
+#if defined(__KERNEL__) || defined(_LOOSE_KERNEL_NAMES)
 # include <linux/types.h>
 typedef __kernel_fsid_t	fsid_t;
 #endif
diff --git a/include/asm-mips/statfs.h b/include/asm-mips/statfs.h
index c3ddf97..d82a9a8 100644
--- a/include/asm-mips/statfs.h
+++ b/include/asm-mips/statfs.h
@@ -11,7 +11,7 @@
 #include <linux/posix_types.h>
 #include <asm/sgidefs.h>
 
-#ifndef __KERNEL_STRICT_NAMES
+#if defined(__KERNEL__) || defined(_LOOSE_KERNEL_NAMES)
 
 #include <linux/types.h>
 
diff --git a/include/asm-parisc/statfs.h b/include/asm-parisc/statfs.h
index 1d2b813..734acd5 100644
--- a/include/asm-parisc/statfs.h
+++ b/include/asm-parisc/statfs.h
@@ -1,7 +1,7 @@
 #ifndef _PARISC_STATFS_H
 #define _PARISC_STATFS_H
 
-#ifndef __KERNEL_STRICT_NAMES
+#if defined(__KERNEL__) || defined(_LOOSE_KERNEL_NAMES)
 
 #include <linux/types.h>
 
diff --git a/include/asm-powerpc/statfs.h b/include/asm-powerpc/statfs.h
index 6702402..aa160a0 100644
--- a/include/asm-powerpc/statfs.h
+++ b/include/asm-powerpc/statfs.h
@@ -7,7 +7,7 @@
 #include <asm-generic/statfs.h>
 #else
 
-#ifndef __KERNEL_STRICT_NAMES
+#if defined(__KERNEL__) || defined(_LOOSE_KERNEL_NAMES)
 #include <linux/types.h>
 typedef __kernel_fsid_t	fsid_t;
 #endif
diff --git a/include/asm-s390/statfs.h b/include/asm-s390/statfs.h
index 099a455..3ba543f 100644
--- a/include/asm-s390/statfs.h
+++ b/include/asm-s390/statfs.h
@@ -13,7 +13,7 @@
 #include <asm-generic/statfs.h>
 #else
 
-#ifndef __KERNEL_STRICT_NAMES
+#if defined(__KERNEL__) || defined(_LOOSE_KERNEL_NAMES)
 
 #include <linux/types.h>
 
diff --git a/include/asm-sparc64/statfs.h b/include/asm-sparc64/statfs.h
index 79b3c89..70d994c 100644
--- a/include/asm-sparc64/statfs.h
+++ b/include/asm-sparc64/statfs.h
@@ -1,7 +1,7 @@
 #ifndef _SPARC64_STATFS_H
 #define _SPARC64_STATFS_H
 
-#ifndef __KERNEL_STRICT_NAMES
+#if defined(__KERNEL__) || defined(_LOOSE_KERNEL_NAMES)
 
 #include <linux/types.h>
 
diff --git a/include/asm-x86/statfs.h b/include/asm-x86/statfs.h
index 7c651aa..2615eed 100644
--- a/include/asm-x86/statfs.h
+++ b/include/asm-x86/statfs.h
@@ -5,7 +5,7 @@
 #include <asm-generic/statfs.h>
 #else
 
-#ifndef __KERNEL_STRICT_NAMES
+#if defined(__KERNEL__) || defined(_LOOSE_KERNEL_NAMES)
 
 #include <linux/types.h>
 

-- 
dwmw2

Re: [PATCH] wireless.h: improve userland include-ability

[David Miller]

From: David Woodhouse <dwmw2@infradead.org>
Date: Wed, 04 Jun 2008 16:26:12 +0100

> -#ifndef __KERNEL_STRICT_NAMES
> +#if defined(__KERNEL__) || defined(_LOOSE_KERNEL_NAMES)

This reminds me that I noticed recently that unifdef doesn't
handle multiple conditions in an #if test involving
__KERNEL__ properly.  So, for example, for something else
in wireless.h I was working on I had to use:

#ifdef __KERNEL__
#ifdef CONFIG_COMPAT

instead of the more usual:

#if defined(__KERNEL__) && defined(CONFIG_COMPAT)

otherwise I'd get check headers errors during the build.

Re: [PATCH] wireless.h: improve userland include-ability

[David Woodhouse]

On Wed, 2008-06-04 at 08:47 -0700, David Miller wrote:
> From: David Woodhouse <dwmw2@infradead.org>
> Date: Wed, 04 Jun 2008 16:26:12 +0100
> 
> > -#ifndef __KERNEL_STRICT_NAMES
> > +#if defined(__KERNEL__) || defined(_LOOSE_KERNEL_NAMES)
> 
> This reminds me that I noticed recently that unifdef doesn't
> handle multiple conditions in an #if test involving
> __KERNEL__ properly.  So, for example, for something else
> in wireless.h I was working on I had to use:
> 
> #ifdef __KERNEL__
> #ifdef CONFIG_COMPAT
> 
> instead of the more usual:
> 
> #if defined(__KERNEL__) && defined(CONFIG_COMPAT)
> 
> otherwise I'd get check headers errors during the build.

Yeah; that's one of the main reasons it hasn't been done yet.

-- 
dwmw2

Re: [PATCH] wireless.h: improve userland include-ability

[Pavel Roskin]

On Wed, 2008-06-04 at 14:52 +0100, David Woodhouse wrote:
> On Tue, 2008-06-03 at 12:05 -0700, David Miller wrote:
> > Yeah, go check so me magic userspace tool header to see what magic is
> > needed just to include a core networking header file correctly.
> > 
> > No, thanks.
> 
> Just to clarify: this is only a problem for userspace, and you're
> objecting to that? Just including <linux/wireless.h> within the kernel
> will continue to work.
> 
> We've traditionally got away with saying 'caveat emptor' when userspace
> includes kernel headers -- you _have_ to include the right
> prerequisites, because the kernel doesn't do it for you.

That's true, and several programs do it already.  Changing the includes
would require changes in those programs.  Besides, sys/types.h is still
included, so it's a half-measure.  If we are going to break things,
let's go all the way and disable sys/types.h for userspace, and possibly
some structures that the userspace is not supposed to be using.  But I'm
afraid there is still a lot of stuff the userspace should know,
including IFNAMESIZ.  Alternatively, let's sanitize other headers so
that they can be mixed with libc headers with no consequences.

Hand picking which headers to include in userspace reeks of a hack meant
to satisfy special needs of some userspace software, while risking to
break other userspace code.

Finally, wireless extensions are not under development.  If should be
perfectly safe to take the latest wireless.h, copy it to the userspace
program and rearrange the headers to the heart's content.

> I believe that the main reason for this patch was that <linux/socket.h>
> and <linux/if.h> both have problems when you include them in the same C
> file as the 'proper' glibc equivalent? Is that something we can address,
> instead of just dropping those includes?

I don't see such problems in my software, but if others do, they may be
in a better position to suggest patches.

-- 
Regards,
Pavel Roskin

Re: [PATCH] wireless.h: improve userland include-ability

[John W. Linville]

On Wed, Jun 04, 2008 at 11:24:15AM -0400, Pavel Roskin wrote:
> On Wed, 2008-06-04 at 14:52 +0100, David Woodhouse wrote:

> > We've traditionally got away with saying 'caveat emptor' when userspace
> > includes kernel headers -- you _have_ to include the right
> > prerequisites, because the kernel doesn't do it for you.
> 
> That's true, and several programs do it already.  Changing the includes
> would require changes in those programs.  Besides, sys/types.h is still
> included, so it's a half-measure.  If we are going to break things,
> let's go all the way and disable sys/types.h for userspace, and possibly
> some structures that the userspace is not supposed to be using.  But I'm

...which is how it is in 2.6.25.  The change in 2.6.26 broke userland
apps that included both net/if.h and linux/wireless.h.  The intent
here was to correct that error while gaining the primary benefit of
not requiring separate inclusion of linux/types.h.

If commit 2218228392080f0ca2fc2974604e79f57b12c436 had never
occured, then this new patch would simply be similar to
commit c8942f1f0a7e2160ebf2e51ba89e50ee5895a1e7 and commit
a1a61a435b3cc157830b7d42b175151ae5eabdd3 (both of which are
Signed-off-by: David S. Miller <davem@davemloft.net>), and it is very
similar to commit 80a9492a33dd7d852465625022d56ff76d62174d as well.

> afraid there is still a lot of stuff the userspace should know,
> including IFNAMESIZ.  Alternatively, let's sanitize other headers so
> that they can be mixed with libc headers with no consequences.

Patches welcome.

John
-- 
John W. Linville
linville@tuxdriver.com