checkpatch.pl should check UAPI headers don't #include <uapi/...>

checkpatch.pl should check UAPI headers don't #include <uapi/...>

[David Howells]

Hi Joe,

Can you make checkpatch.pl check that lines added to UAPI headers don't have
the form:

	#include <uapi/...>

or:

	#include "uapi/..."

Such as these should be regarded as errors as they will likely break userspace
which shouldn't get to see any uapi/ directories.

An example of this is in:

	commit 19bf7f8ac3f8131100027281c495dbbe00cd5ae0
	Merge: 787c57c 35fd3dc
	Author: Marcelo Tosatti <mtosatti@redhat.com>
	Date:   Mon Oct 29 19:15:32 2012 -0200

where during the conflict resolution, the following change was made:

	- #include <asm/epapr_hcalls.h>
	++#include <uapi/asm/epapr_hcalls.h>

I recognise that checkpatch.pl might not have helped in this case since it
isn't normally applied to merged as far as I know.

Thanks,
David

[PATCH] checkpatch: Warn on #include <uapi/...

[Joe Perches]

Avoid specifying include paths with uapi/...
as userspace should not use that path.

Neaten message line wrapping above.

Signed-off-by: Joe Perches <joe@perches.com>
cc: David Howells <dhowells@redhat.com>
---
 scripts/checkpatch.pl |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 054a293..d0c11ee 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2238,8 +2238,11 @@ sub process {
 			my $path = $1;
 			if ($path =~ m{//}) {
 				ERROR("MALFORMED_INCLUDE",
-				      "malformed #include filename\n" .
-					$herecurr);
+				      "malformed #include filename\n" . $herecurr);
+			}
+			if ($path =~ "^uapi/") {
+				ERROR("UAPI_INCLUDE",
+				      "#include should not start with uapi/\n" . $herecurr);
 			}
 		}
 
-- 
1.7.8.112.g3fd21

Re: [PATCH] checkpatch: Warn on #include <uapi/...

[David Howells]

Joe Perches <joe@perches.com> wrote:

> +			if ($path =~ "^uapi/") {
> +				ERROR("UAPI_INCLUDE",
> +				      "#include should not start with uapi/\n" . $herecurr);
>  			}

But does this limit the check only to headers in uapi/ directories?  You need
to be able to do this outside of a uapi/ directory.  For instance,
include/linux/fs.h shadows include/uapi/linux/fs.h and so the former has to
#include the latter directly as <uapi/linux/fs.h> or else suffer a circular
link.

David

Re: [PATCH] checkpatch: Warn on #include <uapi/...

[Joe Perches]

On Tue, 2012-12-18 at 19:17 +0000, David Howells wrote:
> Joe Perches <joe@perches.com> wrote:
> 
> > +			if ($path =~ "^uapi/") {
> > +				ERROR("UAPI_INCLUDE",
> > +				      "#include should not start with uapi/\n" . $herecurr);
> >  			}
> 
> But does this limit the check only to headers in uapi/ directories?

No.  I'm confused, I believe your example was:

arch/powerpc/include/asm/kvm_para.h
@@ -16,77 +16,11 @@
  *
  * Authors: Hollis Blanchard <hollisb@us.ibm.com>
  */
-
 #ifndef __POWERPC_KVM_PARA_H__
 #define __POWERPC_KVM_PARA_H__
 
-#include <linux/types.h>
+#include <uapi/asm/kvm_para.h>

---------------

What do you want this to check?

Re: [PATCH] checkpatch: Warn on #include <uapi/...

[David Howells]

Joe Perches <joe@perches.com> wrote:

> No.  I'm confused, I believe your example was:
> 
> arch/powerpc/include/asm/kvm_para.h
> ...
> -#include <linux/types.h>
> +#include <uapi/asm/kvm_para.h>

No, that is a correct alteration.

The example I gave was:

	- #include <asm/epapr_hcalls.h>
	++#include <uapi/asm/epapr_hcalls.h>

which is to be found in arch/powerpc/include/uapi/asm/kvm_para.h.

Sorry for the confusion.

David

Re: [PATCH] checkpatch: Warn on #include <uapi/...

[Joe Perches]

On Tue, 2012-12-18 at 20:35 +0000, David Howells wrote:
> Joe Perches <joe@perches.com> wrote:
[]
> > arch/powerpc/include/asm/kvm_para.h
> > ...
> > -#include <linux/types.h>
> > +#include <uapi/asm/kvm_para.h>
> 
> No, that is a correct alteration.
> 
> The example I gave was:
> 
> 	- #include <asm/epapr_hcalls.h>
> 	++#include <uapi/asm/epapr_hcalls.h>
> 
> which is to be found in arch/powerpc/include/uapi/asm/kvm_para.h.

Just to verify, any file in any path [.../]include/uapi/...
should not itself have a line like '#include <uapi/...' ?

Re: [PATCH] checkpatch: Warn on #include <uapi/...

[David Howells]

Joe Perches <joe@perches.com> wrote:

> Just to verify, any file in any path [.../]include/uapi/...
> should not itself have a line like '#include <uapi/...' ?

That is correct.

David

[PATCH V2] checkpatch: Warn on uapi #includes that #include <uapi/...

[Joe Perches]

Avoid specifying internal uapi #include paths with uapi/...
as userspace should not use and never see that.

Neaten message line wrapping above.

Signed-off-by: Joe Perches <joe@perches.com>
cc: David Howells <dhowells@redhat.com>
---
 scripts/checkpatch.pl |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 054a293..5eab67e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2238,8 +2238,11 @@ sub process {
 			my $path = $1;
 			if ($path =~ m{//}) {
 				ERROR("MALFORMED_INCLUDE",
-				      "malformed #include filename\n" .
-					$herecurr);
+				      "malformed #include filename\n" . $herecurr);
+			}
+			if ($path =~ "^uapi/" && $realfile =~ m@\binclude/uapi/@) {
+				ERROR("UAPI_INCLUDE",
+				      "No #include in ...include/uapi/... should use a uapi/ path prefix\n" . $herecurr);
 			}
 		}
 
-- 
1.7.8.112.g3fd21

Re: [PATCH V2] checkpatch: Warn on uapi #includes that #include <uapi/...

[David Howells]

Joe Perches <joe@perches.com> wrote:

> Avoid specifying internal uapi #include paths with uapi/...
> as userspace should not use and never see that.
> 
> Neaten message line wrapping above.
> 
> Signed-off-by: Joe Perches <joe@perches.com>

Acked-by: David Howells <dhowells@redhat.com>

Re: [PATCH V2] checkpatch: Warn on uapi #includes that #include <uapi/...

[Andy Whitcroft]

On Tue, Dec 18, 2012 at 05:30:58PM -0800, Joe Perches wrote:
> Avoid specifying internal uapi #include paths with uapi/...
> as userspace should not use and never see that.
> 
> Neaten message line wrapping above.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> cc: David Howells <dhowells@redhat.com>
> ---
>  scripts/checkpatch.pl |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 054a293..5eab67e 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2238,8 +2238,11 @@ sub process {
>  			my $path = $1;
>  			if ($path =~ m{//}) {
>  				ERROR("MALFORMED_INCLUDE",
> -				      "malformed #include filename\n" .
> -					$herecurr);
> +				      "malformed #include filename\n" . $herecurr);
> +			}
> +			if ($path =~ "^uapi/" && $realfile =~ m@\binclude/uapi/@) {
> +				ERROR("UAPI_INCLUDE",
> +				      "No #include in ...include/uapi/... should use a uapi/ path prefix\n" . $herecurr);
>  			}
>  		}
>  

Looks reasonable indeed.

Acked-by: Andy Whitcroft <apw@canonical.com>

-apw