* checkpatch.pl should check UAPI headers don't #include <uapi/...>
@ 2012-12-18 14:23 David Howells
2012-12-18 17:53 ` [PATCH] checkpatch: Warn on #include <uapi/ Joe Perches
0 siblings, 1 reply; 10+ messages in thread
From: David Howells @ 2012-12-18 14:23 UTC (permalink / raw)
To: Joe Perches, Marcelo Tosatti; +Cc: dhowells, Alexander Graf, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] checkpatch: Warn on #include <uapi/...
2012-12-18 14:23 checkpatch.pl should check UAPI headers don't #include <uapi/...> David Howells
@ 2012-12-18 17:53 ` Joe Perches
2012-12-18 19:17 ` David Howells
0 siblings, 1 reply; 10+ messages in thread
From: Joe Perches @ 2012-12-18 17:53 UTC (permalink / raw)
To: Andrew Morton, Andy Whitcroft
Cc: Marcelo Tosatti, Alexander Graf, David Howells, linux-kernel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] checkpatch: Warn on #include <uapi/...
2012-12-18 17:53 ` [PATCH] checkpatch: Warn on #include <uapi/ Joe Perches
@ 2012-12-18 19:17 ` David Howells
2012-12-18 19:52 ` Joe Perches
0 siblings, 1 reply; 10+ messages in thread
From: David Howells @ 2012-12-18 19:17 UTC (permalink / raw)
To: Joe Perches
Cc: dhowells, Andrew Morton, Andy Whitcroft, Marcelo Tosatti,
Alexander Graf, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] checkpatch: Warn on #include <uapi/...
2012-12-18 19:17 ` David Howells
@ 2012-12-18 19:52 ` Joe Perches
2012-12-18 20:35 ` David Howells
0 siblings, 1 reply; 10+ messages in thread
From: Joe Perches @ 2012-12-18 19:52 UTC (permalink / raw)
To: David Howells
Cc: Andrew Morton, Andy Whitcroft, Marcelo Tosatti, Alexander Graf,
linux-kernel
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?
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] checkpatch: Warn on #include <uapi/...
2012-12-18 19:52 ` Joe Perches
@ 2012-12-18 20:35 ` David Howells
2012-12-18 22:14 ` Joe Perches
0 siblings, 1 reply; 10+ messages in thread
From: David Howells @ 2012-12-18 20:35 UTC (permalink / raw)
To: Joe Perches
Cc: dhowells, Andrew Morton, Andy Whitcroft, Marcelo Tosatti,
Alexander Graf, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] checkpatch: Warn on #include <uapi/...
2012-12-18 20:35 ` David Howells
@ 2012-12-18 22:14 ` Joe Perches
2012-12-18 22:35 ` David Howells
0 siblings, 1 reply; 10+ messages in thread
From: Joe Perches @ 2012-12-18 22:14 UTC (permalink / raw)
To: David Howells
Cc: Andrew Morton, Andy Whitcroft, Marcelo Tosatti, Alexander Graf,
linux-kernel
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/...' ?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] checkpatch: Warn on #include <uapi/...
2012-12-18 22:14 ` Joe Perches
@ 2012-12-18 22:35 ` David Howells
2012-12-19 1:30 ` [PATCH V2] checkpatch: Warn on uapi #includes that " Joe Perches
0 siblings, 1 reply; 10+ messages in thread
From: David Howells @ 2012-12-18 22:35 UTC (permalink / raw)
To: Joe Perches
Cc: dhowells, Andrew Morton, Andy Whitcroft, Marcelo Tosatti,
Alexander Graf, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH V2] checkpatch: Warn on uapi #includes that #include <uapi/...
2012-12-18 22:35 ` David Howells
@ 2012-12-19 1:30 ` Joe Perches
2012-12-19 13:18 ` David Howells
2012-12-19 13:31 ` Andy Whitcroft
0 siblings, 2 replies; 10+ messages in thread
From: Joe Perches @ 2012-12-19 1:30 UTC (permalink / raw)
To: Andrew Morton, Andy Whitcroft
Cc: Marcelo Tosatti, Alexander Graf, David Howells, linux-kernel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH V2] checkpatch: Warn on uapi #includes that #include <uapi/...
2012-12-19 1:30 ` [PATCH V2] checkpatch: Warn on uapi #includes that " Joe Perches
@ 2012-12-19 13:18 ` David Howells
2012-12-19 13:31 ` Andy Whitcroft
1 sibling, 0 replies; 10+ messages in thread
From: David Howells @ 2012-12-19 13:18 UTC (permalink / raw)
To: Joe Perches
Cc: dhowells, Andrew Morton, Andy Whitcroft, Marcelo Tosatti,
Alexander Graf, linux-kernel
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>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2] checkpatch: Warn on uapi #includes that #include <uapi/...
2012-12-19 1:30 ` [PATCH V2] checkpatch: Warn on uapi #includes that " Joe Perches
2012-12-19 13:18 ` David Howells
@ 2012-12-19 13:31 ` Andy Whitcroft
1 sibling, 0 replies; 10+ messages in thread
From: Andy Whitcroft @ 2012-12-19 13:31 UTC (permalink / raw)
To: Joe Perches
Cc: Andrew Morton, Marcelo Tosatti, Alexander Graf, David Howells,
linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-12-19 13:31 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-12-18 14:23 checkpatch.pl should check UAPI headers don't #include <uapi/...> David Howells
2012-12-18 17:53 ` [PATCH] checkpatch: Warn on #include <uapi/ Joe Perches
2012-12-18 19:17 ` David Howells
2012-12-18 19:52 ` Joe Perches
2012-12-18 20:35 ` David Howells
2012-12-18 22:14 ` Joe Perches
2012-12-18 22:35 ` David Howells
2012-12-19 1:30 ` [PATCH V2] checkpatch: Warn on uapi #includes that " Joe Perches
2012-12-19 13:18 ` David Howells
2012-12-19 13:31 ` Andy Whitcroft
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox