[PATCH] headers_install.pl: autoconvert asm/inline/volatile to __xxx__

[PATCH] headers_install.pl: autoconvert asm/inline/volatile to __xxx__

[Mike Frysinger]

Headers in userspace should be using the __xxx__ form of the asm, inline,
and volatile keywords.  Since people like to revert these things without
realizing what's going on, have the headers install step autoconvert these
keywords.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 scripts/headers_install.pl |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/scripts/headers_install.pl b/scripts/headers_install.pl
index 7d2b414..b450aec 100644
--- a/scripts/headers_install.pl
+++ b/scripts/headers_install.pl
@@ -36,6 +36,7 @@ foreach my $file (@files) {
 		$line =~ s/\s__attribute_const__\s/ /g;
 		$line =~ s/\s__attribute_const__$//g;
 		$line =~ s/^#include <linux\/compiler.h>//;
+		$line =~ s/(^|\s)(asm|inline|volatile)(\s|[(]|$)\b/$1__$2__$3/;
 		printf OUTFILE "%s", $line;
 	}
 	close OUTFILE;
-- 
1.6.0.6

[PATCH v2] headers_install.pl: autoconvert asm/inline/volatile to __xxx__

[Mike Frysinger]

Headers in userspace should be using the __xxx__ form of the asm, inline,
and volatile keywords.  Since people like to revert these things without
realizing what's going on, have the headers install step autoconvert these
keywords.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
v2
	- get asm/volatile to actually work ... i missed this due to other
	  local patches.  "asm volatile" should get fixed now.

 scripts/headers_install.pl |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/scripts/headers_install.pl b/scripts/headers_install.pl
index 7d2b414..c6ae405 100644
--- a/scripts/headers_install.pl
+++ b/scripts/headers_install.pl
@@ -36,6 +36,9 @@ foreach my $file (@files) {
 		$line =~ s/\s__attribute_const__\s/ /g;
 		$line =~ s/\s__attribute_const__$//g;
 		$line =~ s/^#include <linux\/compiler.h>//;
+		$line =~ s/(^|\s)(inline)\b/$1__$2__/g;
+		$line =~ s/(^|\s)(asm)\b(\s|[(]|$)/$1__$2__$3/g;
+		$line =~ s/(^|\s|[(])(volatile)\b(\s|[(]|$)/$1__$2__$3/g;
 		printf OUTFILE "%s", $line;
 	}
 	close OUTFILE;
-- 
1.6.0.6

Re: [PATCH v2] headers_install.pl: autoconvert asm/inline/volatile to __xxx__

[Sam Ravnborg]

On Sat, Dec 27, 2008 at 03:23:15AM -0500, Mike Frysinger wrote:
> Headers in userspace should be using the __xxx__ form of the asm, inline,
> and volatile keywords.  Since people like to revert these things without
> realizing what's going on, have the headers install step autoconvert these
> keywords.

Applied

	Sam

Re: [PATCH] headers_install.pl: autoconvert asm/inline/volatile to __xxx__

[H. Peter Anvin]

Mike Frysinger wrote:
> Headers in userspace should be using the __xxx__ form of the asm, inline,
> and volatile keywords.  Since people like to revert these things without
> realizing what's going on, have the headers install step autoconvert these
> keywords.
> 
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

volatile?

volatile is a keyword in C90, and anything older than that we're
guaranteed to not be compatible with anyway...

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] headers_install.pl: autoconvert asm/inline/volatile to __xxx__

[Mike Frysinger]

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

On Monday 29 December 2008 05:33:54 H. Peter Anvin wrote:
> Mike Frysinger wrote:
> > Headers in userspace should be using the __xxx__ form of the asm, inline,
> > and volatile keywords.  Since people like to revert these things without
> > realizing what's going on, have the headers install step autoconvert
> > these keywords.
> >
> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>
> volatile?
>
> volatile is a keyword in C90, and anything older than that we're
> guaranteed to not be compatible with anyway...

i dont really have any history examples left at this point.  we've been doing 
{asm,inline,volatile} conversion together forever in the Gentoo world, so i 
dont have anything to point you at.  i would highlight that the code to 
support it is trivial and "free", so just leaving it in is easy enough.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] headers_install.pl: autoconvert asm/inline/volatile to __xxx__

[Arnd Bergmann]

On Saturday 27 December 2008, Mike Frysinger wrote:
> Headers in userspace should be using the __xxx__ form of the asm, inline,
> and volatile keywords.  Since people like to revert these things without
> realizing what's going on, have the headers install step autoconvert these
> keywords.
> 
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

The patch looks right, but I would argue that most of the instances of
any of these should not be exported in the first place. I'm not
sure whether there is a legitimate use for the byteorder inlines,
but the others I could find look like they are not meant for use by the
kernel.

	Arnd <><

Re: [PATCH] headers_install.pl: autoconvert asm/inline/volatile to __xxx__

[Mike Frysinger]

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

On Wednesday 31 December 2008 12:15:07 Arnd Bergmann wrote:
> On Saturday 27 December 2008, Mike Frysinger wrote:
> > Headers in userspace should be using the __xxx__ form of the asm, inline,
> > and volatile keywords.  Since people like to revert these things without
> > realizing what's going on, have the headers install step autoconvert
> > these keywords.
> >
> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>
> The patch looks right, but I would argue that most of the instances of
> any of these should not be exported in the first place. I'm not
> sure whether there is a legitimate use for the byteorder inlines,
> but the others I could find look like they are not meant for use by the
> kernel.

maybe, but those threads are going to take quite a while to resolve and things 
will constantly leak back out.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] headers_install.pl: autoconvert asm/inline/volatile to __xxx__

[Arnd Bergmann]

On Wednesday 31 December 2008, Mike Frysinger wrote:
> On Wednesday 31 December 2008 12:15:07 Arnd Bergmann wrote:
> > On Saturday 27 December 2008, Mike Frysinger wrote:
> > > Headers in userspace should be using the __xxx__ form of the asm, inline,
> > > and volatile keywords.  Since people like to revert these things without
> > > realizing what's going on, have the headers install step autoconvert
> > > these keywords.
> > >
> > > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> >
> > The patch looks right, but I would argue that most of the instances of
> > any of these should not be exported in the first place. I'm not
> > sure whether there is a legitimate use for the byteorder inlines,
> > but the others I could find look like they are not meant for use by the
> > kernel.
> 
> maybe, but those threads are going to take quite a while to resolve and things 
> will constantly leak back out.

I guess I forgot to make my main point, I'm not arguing against your patch
at all, but would also like to see another patch to headers_check.pl that
warns about all instances of the __asm__, __inline__ and volatile keywords,
possibly with a list of known good cases.

Since you already have on the patch to warn about the extern declarations
and Sam submitted a respective patch for the CONFIG_ symbols, it would be
good to add these keywords to the same check.

Your headers_install.pl patch is required for correctness with pedantic
compilers, while the one I suggest would only help reducing the leak
of kernel internal code into user space headers.

	Arnd <><

Re: [PATCH] headers_install.pl: autoconvert asm/inline/volatile to __xxx__

[Mike Frysinger]

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

On Friday 02 January 2009 18:20:34 Arnd Bergmann wrote:
> On Wednesday 31 December 2008, Mike Frysinger wrote:
> > On Wednesday 31 December 2008 12:15:07 Arnd Bergmann wrote:
> > > On Saturday 27 December 2008, Mike Frysinger wrote:
> > > > Headers in userspace should be using the __xxx__ form of the asm,
> > > > inline, and volatile keywords.  Since people like to revert these
> > > > things without realizing what's going on, have the headers install
> > > > step autoconvert these keywords.
> > >
> > > The patch looks right, but I would argue that most of the instances of
> > > any of these should not be exported in the first place. I'm not
> > > sure whether there is a legitimate use for the byteorder inlines,
> > > but the others I could find look like they are not meant for use by the
> > > kernel.
> >
> > maybe, but those threads are going to take quite a while to resolve and
> > things will constantly leak back out.
>
> I guess I forgot to make my main point, I'm not arguing against your patch
> at all, but would also like to see another patch to headers_check.pl that
> warns about all instances of the __asm__, __inline__ and volatile keywords,
> possibly with a list of known good cases.

how would you propose maintaining said list ?  attempting to maintain external 
to the file in question will just lead to rot (since we dont want to whitelist 
entire files, and we cant track line numbers, and we cant detect where the 
keyword is being used in terms of macro/function) ... that leaves two choices 
that i can think of:
 - a new __asm_userok__ type marker
 - adding a simple /* userok */ comment we can filter in the perl regex

neither of which sounds entire pleasant either ...
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] headers_install.pl: autoconvert asm/inline/volatile to __xxx__

[Arnd Bergmann]

On Saturday 03 January 2009, Mike Frysinger wrote:
> how would you propose maintaining said list ?  attempting to maintain external 
> to the file in question will just lead to rot (since we dont want to whitelist 
> entire files, and we cant track line numbers, and we cant detect where the 
> keyword is being used in terms of macro/function) ... that leaves two choices 
> that i can think of:
>  - a new __asm_userok__ type marker
>  - adding a simple /* userok */ comment we can filter in the perl regex
> 
> neither of which sounds entire pleasant either ...

It all depends on how many exceptions there are. The number of instances
of asm and volatile keywords is really small (one file each, on x86), so
we can easily decide whether or not they should all be allowed or disallowed.

For inline, we already have a precedent in include/linux/socket.h,
which contains

/*
 *	This mess will go away with glibc
 */
#if    defined(__GNUC__)
#define __KINLINE static __inline__
#elif defined(__cplusplus) /* || __STDC_VERSION__ >= 199901L  --arnd */
#define __KINLINE static inline
#else
#define __KINLINE static
#endif

This demonstrates that there are additional problems to consider. In order
to do the right thing, we could first use your patch to fix up all instances
of inline, and then warn about any __inline__ and inline, converting the
legitimate ones to __KINLINE.

Again, there are very few legitimate ones, unless you count the byteorder
headers as correct, which I'm still undecided about.

	Arnd <><