Skip to contentSkip to conversation

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

PATCH Superseded David Howells 3 participants 10 messages Dec 18, 2012 – Dec 19, 2012 Checkpatch

lore
Share & tools
lore

David Howells

author
#0 lore raw

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

maintainer
#1 lore raw

PATCH 1/1 checkpatch: Warn on #include <uapi/...

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

Neaten message line wrapping above.

Signed-off-by Joe Perches <[email protected]>
cc David Howells <[email protected]>
---
 scripts/checkpatch.pl |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)
scripts/checkpatch.pl +5 -2
index 054a293..d0c11ee 100755
@@ -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);
 			}
 		}
Signature · 1 line
1.7.8.112.g3fd21
author
#2 lore raw

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

scripts/checkpatch.pl sub process { · line 2246

Quoted 4 lines · Joe Perches
scripts/checkpatch.pl +3 -0
+			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

maintainer
#3 lore raw

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

Quoted 8 lines · David Howells

scripts/checkpatch.pl sub process { · line 2246

Quoted 4 lines · Joe Perches
scripts/checkpatch.pl +3 -0
+			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 <[email protected]> */
-
#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?

author
#4 lore raw

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

Review

Quoted 6 lines · Joe Perches

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

arch/powerpc/include/asm/kvm_para.h
...

(quoted diff) +1 -1
-#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

maintainer
#5 lore raw

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

Quoted 1 line · David Howells

Joe Perches <[email protected]> wrote:

[]

Quoted 7 lines

Review

Quoted 4 lines

arch/powerpc/include/asm/kvm_para.h
...

(quoted diff) +1 -1
-#include <linux/types.h>
+#include <uapi/asm/kvm_para.h>

No, that is a correct alteration.

Quoted 6 lines

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/...' ?

author
#6 lore raw

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

Quoted 2 lines · Joe Perches

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

That is correct.

David

maintainer
#7 lore raw

PATCH v2 1/1 checkpatch: Warn on uapi #includes that #include <uapi/...

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 <[email protected]>
cc David Howells <[email protected]>
---
 scripts/checkpatch.pl |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)
scripts/checkpatch.pl +5 -2
index 054a293..5eab67e 100755
@@ -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);
 			}
 		}
Signature · 1 line
1.7.8.112.g3fd21
maintainer Acked-by on v2 1/1
#9 lore raw

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

In reply to Joe Perches
Quoted 19 lines · 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 <[email protected]>
cc David Howells <[email protected]>
---
 scripts/checkpatch.pl |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)
scripts/checkpatch.pl +0 -0
index 054a293..5eab67e 100755
@@ -2238,8 +2238,11 @@ sub process {
 			my $path = $1;
 			if ($path =~ m{//}) {
 				ERROR("MALFORMED_INCLUDE",

scripts/checkpatch.pl sub process { · line 2248

Quoted 10 lines
scripts/checkpatch.pl +5 -2
-				      "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 <[email protected]>

-apw