[PATCH 00/13] update checkpatch to version 0.26

[PATCH 00/13] update checkpatch to version 0.26

[Andy Whitcroft]

This update brings a couple of new checks, and a number of fixes.  Of note:

 - allows parentheses on return of comparisons
 - tracks #ifdef sections better
 - fixes miss reports of variables containing static

It also updates the maintainers entry to track my new email address and
to drop inactive maintainers.

Complete changelog below.

-apw

Andy Whitcroft (11):
  checkpatch: update MAINTAINERS entry
  checkpatch: update copyrights
  checkpatch: allow parentheses on return for comparisons
  checkpatch: loosen spacing on typedef function checks
  checkpatch: fix continuation detection when handling spacing on operators
  checkpatch: track #ifdef/#else/#endif when tracking blocks
  checkpatch: do not report nr_static as a static declaration
  checkpatch: ensure we actually detect if assignments split across lines
  checkpatch: struct file_operations should normally be const
  checkpatch: fix the perlcritic errors
  checkpatch: version: 0.26

Mike Frysinger (1):
  checkpatch: try to catch missing VMLINUX_SYMBOL() in vmlinux.lds.h

Wolfram Sang (1):
  checkpatch: Add warning for p0-patches

 scripts/checkpatch.pl |   84 +++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 67 insertions(+), 17 deletions(-)

[PATCH 01/13] checkpatch: update MAINTAINERS entry

[Andy Whitcroft]

Update my email address to my new work address.  Also, as per our recent
email conversation remove Randy and Joel from the maintainers list.
Finally add LKML so that emails are recorded somewhere.

Signed-off-by: Andy Whitcroft <apw@shadowen.org>
Cc: Randy Dunlap <rdunlap@xenotime.net>
Cc: Joel Schopp <jschopp@austin.ibm.com>
---
 MAINTAINERS |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 618c1ef..0d587ff 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1091,11 +1091,8 @@ S:	Maintained
 
 CHECKPATCH
 P:	Andy Whitcroft
-M:	apw@shadowen.org
-P:	Randy Dunlap
-M:	rdunlap@xenotime.net
-P:	Joel Schopp
-M:	jschopp@austin.ibm.com
+M:	apw@canonical.com
+L:	linux-kernel@vger.kernel.org
 S:	Supported
 
 CISCO 10G ETHERNET DRIVER
-- 
1.6.0.4.911.gc990

[PATCH 02/13] checkpatch: update copyrights

[Andy Whitcroft]

From: Andy Whitcroft <apw@canonical.com>

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 770fa41..f015698 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1,7 +1,8 @@
 #!/usr/bin/perl -w
 # (c) 2001, Dave Jones. <davej@redhat.com> (the file handling bit)
 # (c) 2005, Joel Schopp <jschopp@austin.ibm.com> (the ugly bit)
-# (c) 2007, Andy Whitcroft <apw@uk.ibm.com> (new conditions, test suite, etc)
+# (c) 2007,2008, Andy Whitcroft <apw@uk.ibm.com> (new conditions, test suite)
+# (c) 2008, Andy Whitcroft <apw@canonical.com>
 # Licensed under the terms of the GNU GPL License version 2
 
 use strict;
-- 
1.6.0.4.911.gc990

[PATCH 03/13] checkpatch: Add warning for p0-patches

[Andy Whitcroft]

From: Wolfram Sang <w.sang@pengutronix.de>

Some people work internally with -p0-patches which has the danger that
one forgets to convert them to -p1 before mainlining. Bitten myself and
seen p0-patches in mailing lists occasionally, this patch adds a warning
to checkpatch.pl in case a patch is -p0. If you really want, you can
fool this check to generate false positives, this is why it just spits a
warning. Making the check 100% proof is trickier than it looks, so let's
start with a version which catches the cases of real use.

[apw@canonical.com: update message language, handle null prefix, add tests]
Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f015698..b953c76 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1057,6 +1057,7 @@ sub process {
 	my $in_comment = 0;
 	my $comment_edge = 0;
 	my $first_line = 0;
+	my $p1_prefix = '';
 
 	my $prev_values = 'E';
 
@@ -1205,7 +1206,12 @@ sub process {
 		# extract the filename as it passes
 		if ($line=~/^\+\+\+\s+(\S+)/) {
 			$realfile = $1;
-			$realfile =~ s@^[^/]*/@@;
+			$realfile =~ s@^([^/]*)/@@;
+
+			$p1_prefix = $1;
+			if ($tree && $p1_prefix ne '' && -e "$root/$p1_prefix") {
+				WARN("patch prefix '$p1_prefix' exists, appears to be a -p0 patch\n");
+			}
 
 			if ($realfile =~ m@^include/asm/@) {
 				ERROR("do not modify files in include/asm, change architecture specific files in include/asm-<architecture>\n" . "$here$rawline\n");
-- 
1.6.0.4.911.gc990

[PATCH 04/13] checkpatch: allow parentheses on return for comparisons

[Andy Whitcroft]

From: Andy Whitcroft <apw@canonical.com>

It seems to be a common idiom to include braces on conditionals in all
contexts including return.  Allow this exception to the return is not
a function checks.  Reported by Kay Sievers.

Cc: Kay Sievers <kay.sievers@vrfy.org>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index b953c76..5c7fd1a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -127,6 +127,7 @@ our $Lval	= qr{$Ident(?:$Member)*};
 
 our $Constant	= qr{(?:[0-9]+|0x[0-9a-fA-F]+)[UL]*};
 our $Assignment	= qr{(?:\*\=|/=|%=|\+=|-=|<<=|>>=|&=|\^=|\|=|=)};
+our $Compare    = qr{<=|>=|==|!=|<|>};
 our $Operators	= qr{
 			<=|>=|==|!=|
 			=>|->|<<|>>|<|>|!|~|
@@ -1983,9 +1984,9 @@ sub process {
 			my $spacing = $1;
 			my $value = $2;
 
-			# Flatten any parentheses and braces
+			# Flatten any parentheses
 			$value =~ s/\)\(/\) \(/g;
-			while ($value =~ s/\([^\(\)]*\)/1/) {
+			while ($value !~ /(?:$Ident|-?$Constant)\s*$Compare\s*(?:$Ident|-?$Constant)/ && $value =~ s/\([^\(\)]*\)/1/) {
 			}
 
 			if ($value =~ /^(?:$Ident|-?$Constant)$/) {
-- 
1.6.0.4.911.gc990

[PATCH 05/13] checkpatch: try to catch missing VMLINUX_SYMBOL() in vmlinux.lds.h

[Andy Whitcroft]

From: Mike Frysinger <vapier@gentoo.org>

Seems like every other release we have someone who updates vmlinux.lds.h and
adds C-visible symbols without VMLINUX_SYMBOL() around them.  So start
checking the file and reject assignments which have plain symbols on either
side.

[apw@canonical.com: soften the check, add tests]
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 5c7fd1a..705a043 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2224,6 +2224,15 @@ sub process {
 			}
 		}
 
+# make sure symbols are always wrapped with VMLINUX_SYMBOL() ...
+# all assignments may have only one of the following with an assignment:
+#	.
+#	ALIGN(...)
+#	VMLINUX_SYMBOL(...)
+		if ($realfile eq 'vmlinux.lds.h' && $line =~ /(?:(?:^|\s)$Ident\s*=|=\s*$Ident(?:\s|$))/) {
+			WARN("vmlinux.lds.h needs VMLINUX_SYMBOL() around C-visible symbols\n" . $herecurr);
+		}
+
 # check for redundant bracing round if etc
 		if ($line =~ /(^.*)\bif\b/ && $1 !~ /else\s*$/) {
 			my ($level, $endln, @chunks) =
-- 
1.6.0.4.911.gc990

[PATCH 06/13] checkpatch: loosen spacing on typedef function checks

[Andy Whitcroft]

From: Andy Whitcroft <apw@canonical.com>

Loosen spacing checks to correctly detect this valid use of a typedef:

	typedef struct rcu_data *(*get_data_func)(int);

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 705a043..d80b55a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1618,7 +1618,7 @@ sub process {
 # check for new typedefs, only function parameters and sparse annotations
 # make sense.
 		if ($line =~ /\btypedef\s/ &&
-		    $line !~ /\btypedef\s+$Type\s+\(\s*\*?$Ident\s*\)\s*\(/ &&
+		    $line !~ /\btypedef\s+$Type\s*\(\s*\*?$Ident\s*\)\s*\(/ &&
 		    $line !~ /\btypedef\s+$Type\s+$Ident\s*\(/ &&
 		    $line !~ /\b$typeTypedefs\b/ &&
 		    $line !~ /\b__bitwise(?:__|)\b/) {
-- 
1.6.0.4.911.gc990

[PATCH 07/13] checkpatch: fix continuation detection when handling spacing on operators

[Andy Whitcroft]

From: Andy Whitcroft <apw@canonical.com>

We are miss categorising a continuation fragment following an operator
which may lead to us thinking that there is a space after it when there
is not.  Fix this up.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d80b55a..67b0c9f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1793,7 +1793,7 @@ sub process {
 					$c = 'C' if ($elements[$n + 2] =~ /^$;/);
 					$c = 'B' if ($elements[$n + 2] =~ /^(\)|\]|;)/);
 					$c = 'O' if ($elements[$n + 2] eq '');
-					$c = 'E' if ($elements[$n + 2] =~ /\s*\\$/);
+					$c = 'E' if ($elements[$n + 2] =~ /^\s*\\$/);
 				} else {
 					$c = 'E';
 				}
-- 
1.6.0.4.911.gc990

[PATCH 08/13] checkpatch: track #ifdef/#else/#endif when tracking blocks

[Andy Whitcroft]

From: Andy Whitcroft <apw@canonical.com>

When picking up a complete statement or block for analysis we cannot
simply track open/close/etc parenthesis we must take into account
preprocessor section boundaries.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 67b0c9f..906624c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -405,6 +405,7 @@ sub ctx_statement_block {
 
 	my $type = '';
 	my $level = 0;
+	my @stack = ([$type, $level]);
 	my $p;
 	my $c;
 	my $len = 0;
@@ -436,6 +437,16 @@ sub ctx_statement_block {
 		$remainder = substr($blk, $off);
 
 		#warn "CSB: c<$c> type<$type> level<$level> remainder<$remainder> coff_set<$coff_set>\n";
+
+		# Handle nested #if/#else.
+		if ($remainder =~ /^#\s*(?:ifndef|ifdef|if)\s/) {
+			push(@stack, [ $type, $level ]);
+		} elsif ($remainder =~ /^#\s*(?:else|elif)\b/) {
+			($type, $level) = @{$stack[$#stack - 1]};
+		} elsif ($remainder =~ /^#\s*endif\b/) {
+			($type, $level) = @{pop(@stack)};
+		}
+
 		# Statement ends at the ';' or a close '}' at the
 		# outermost level.
 		if ($level == 0 && $c eq ';') {
@@ -582,11 +593,22 @@ sub ctx_block_get {
 	my @res = ();
 
 	my $level = 0;
+	my @stack = ($level);
 	for ($line = $start; $remain > 0; $line++) {
 		next if ($rawlines[$line] =~ /^-/);
 		$remain--;
 
 		$blk .= $rawlines[$line];
+
+		# Handle nested #if/#else.
+		if ($rawlines[$line] =~ /^.\s*#\s*(?:ifndef|ifdef|if)\s/) {
+			push(@stack, $level);
+		} elsif ($rawlines[$line] =~ /^.\s*#\s*(?:else|elif)\b/) {
+			$level = $stack[$#stack - 1];
+		} elsif ($rawlines[$line] =~ /^.\s*#\s*endif\b/) {
+			$level = pop(@stack);
+		}
+
 		foreach my $c (split(//, $rawlines[$line])) {
 			##print "C<$c>L<$level><$open$close>O<$off>\n";
 			if ($off > 0) {
-- 
1.6.0.4.911.gc990

[PATCH 09/13] checkpatch: do not report nr_static as a static declaration

[Andy Whitcroft]

From: Andy Whitcroft <apw@canonical.com>

Ensure we do not report identifiers containing the word static as static
declarations.  For example this should not be reported as an unecessary
assignement of 0:

	long nr_static = 0;

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 906624c..a521d49 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1632,7 +1632,7 @@ sub process {
 				$herecurr);
 		}
 # check for static initialisers.
-		if ($line =~ /\s*static\s.*=\s*(0|NULL|false)\s*;/) {
+		if ($line =~ /\bstatic\s.*=\s*(0|NULL|false)\s*;/) {
 			ERROR("do not initialise statics to 0 or NULL\n" .
 				$herecurr);
 		}
-- 
1.6.0.4.911.gc990

[PATCH 10/13] checkpatch: ensure we actually detect if assignments split across lines

[Andy Whitcroft]

From: Andy Whitcroft <apw@canonical.com>

When checking for assignments within if conditionals we check the whole
of the condition, but the match is performed using a line constrained
regular expression.  This means we can miss split conditionals or those
on the second line.  Allow the check to span lines.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a521d49..c39ce0b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2048,7 +2048,7 @@ sub process {
 		    $line =~ /\b(?:if|while|for)\s*\(/ && $line !~ /^.\s*#/) {
 			my ($s, $c) = ($stat, $cond);
 
-			if ($c =~ /\bif\s*\(.*[^<>!=]=[^=].*/) {
+			if ($c =~ /\bif\s*\(.*[^<>!=]=[^=].*/s) {
 				ERROR("do not use assignment in if condition\n" . $herecurr);
 			}
 
-- 
1.6.0.4.911.gc990

[PATCH 11/13] checkpatch: struct file_operations should normally be const

[Andy Whitcroft]

From: Andy Whitcroft <apw@canonical.com>

In the general use case struct file_operations should be a const object.
Check for and warn where it is not.  As suggested by Steven and Ingo.

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c39ce0b..94371f6 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2509,6 +2509,11 @@ sub process {
 		if ($line =~ /^.\s*__initcall\s*\(/) {
 			WARN("please use device_initcall() instead of __initcall()\n" . $herecurr);
 		}
+# check for struct file_operations, ensure they are const.
+		if ($line =~ /\bstruct\s+file_operations\b/ &&
+		    $line !~ /\bconst\b/) {
+			WARN("struct file_operations should normally be const\n" . $herecurr);
+		}
 
 # use of NR_CPUS is usually wrong
 # ignore definitions of NR_CPUS and usage to define arrays as likely right
-- 
1.6.0.4.911.gc990

Re: [PATCH 11/13] checkpatch: struct file_operations should normally be const

[Steven Rostedt]

On Sun, Dec 07, 2008 at 06:30:48PM +0000, Andy Whitcroft wrote:
> From: Andy Whitcroft <apw@canonical.com>
> 
> In the general use case struct file_operations should be a const object.
> Check for and warn where it is not.  As suggested by Steven and Ingo.

Andy,

Thanks for doing this!

Acked-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

> 
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Ingo Molnar <mingo@elte.hu>
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> ---
>  scripts/checkpatch.pl |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index c39ce0b..94371f6 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2509,6 +2509,11 @@ sub process {
>  		if ($line =~ /^.\s*__initcall\s*\(/) {
>  			WARN("please use device_initcall() instead of __initcall()\n" . $herecurr);
>  		}
> +# check for struct file_operations, ensure they are const.
> +		if ($line =~ /\bstruct\s+file_operations\b/ &&
> +		    $line !~ /\bconst\b/) {
> +			WARN("struct file_operations should normally be const\n" . $herecurr);
> +		}
>  
>  # use of NR_CPUS is usually wrong
>  # ignore definitions of NR_CPUS and usage to define arrays as likely right
> -- 
> 1.6.0.4.911.gc990
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

[PATCH 12/13] checkpatch: fix the perlcritic errors

[Andy Whitcroft]

From: Andy Whitcroft <apw@canonical.com>

Clean up checkpatch using perlcritic.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 94371f6..bd6ac90 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -69,7 +69,9 @@ my $dbg_possible = 0;
 my $dbg_type = 0;
 my $dbg_attr = 0;
 for my $key (keys %debug) {
-	eval "\${dbg_$key} = '$debug{$key}';"
+	## no critic
+	eval "\${dbg_$key} = '$debug{$key}';";
+	die "$@" if ($@);
 }
 
 if ($terse) {
@@ -206,9 +208,9 @@ my @dep_includes = ();
 my @dep_functions = ();
 my $removal = "Documentation/feature-removal-schedule.txt";
 if ($tree && -f "$root/$removal") {
-	open(REMOVE, "<$root/$removal") ||
+	open(my $REMOVE, '<', "$root/$removal") ||
 				die "$P: $removal: open failed - $!\n";
-	while (<REMOVE>) {
+	while (<$REMOVE>) {
 		if (/^Check:\s+(.*\S)/) {
 			for my $entry (split(/[, ]+/, $1)) {
 				if ($entry =~ m@include/(.*)@) {
@@ -220,17 +222,21 @@ if ($tree && -f "$root/$removal") {
 			}
 		}
 	}
+	close($REMOVE);
 }
 
 my @rawlines = ();
 my @lines = ();
 my $vname;
 for my $filename (@ARGV) {
+	my $FILE;
 	if ($file) {
-		open(FILE, "diff -u /dev/null $filename|") ||
+		open($FILE, '-|', "diff -u /dev/null $filename") ||
 			die "$P: $filename: diff failed - $!\n";
+	} elsif ($filename eq '-') {
+		open($FILE, '<&STDIN');
 	} else {
-		open(FILE, "<$filename") ||
+		open($FILE, '<', "$filename") ||
 			die "$P: $filename: open failed - $!\n";
 	}
 	if ($filename eq '-') {
@@ -238,11 +244,11 @@ for my $filename (@ARGV) {
 	} else {
 		$vname = $filename;
 	}
-	while (<FILE>) {
+	while (<$FILE>) {
 		chomp;
 		push(@rawlines, $_);
 	}
-	close(FILE);
+	close($FILE);
 	if (!process($filename)) {
 		$exit = 1;
 	}
-- 
1.6.0.4.911.gc990

[PATCH 13/13] checkpatch: version: 0.26

[Andy Whitcroft]

From: Andy Whitcroft <apw@canonical.com>

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bd6ac90..7bed4ed 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -10,7 +10,7 @@ use strict;
 my $P = $0;
 $P =~ s@.*/@@g;
 
-my $V = '0.25';
+my $V = '0.26';
 
 use Getopt::Long qw(:config no_auto_abbrev);
 
-- 
1.6.0.4.911.gc990