[Qemu-devel] [PATCH 0/3] checkpatch tweaks

[Qemu-devel] [PATCH 0/3] checkpatch tweaks

[Paolo Bonzini]

My proposal after having watched patchew complain for a couple days
about patches being sent on the list.

Paolo

Paolo Bonzini (3):
  checkpatch: tweak the files in which TABs are checked
  checkpatch: bump most warnings to errors
  checkpatch: default to success if only warnings

 scripts/checkpatch.pl | 88 ++++++++++++++++++++++++---------------------------
 1 file changed, 41 insertions(+), 47 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/3] checkpatch: tweak the files in which TABs are checked

[Paolo Bonzini]

Include Python and shell scripts, and make an exception for Perl
scripts we imported from Linux or elsewhere.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/checkpatch.pl | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index b7cb4ab..7ccf6a8 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1351,8 +1351,10 @@ sub process {
 			WARN("adding a line without newline at end of file\n" . $herecurr);
 		}
 
-# check we are in a valid source file C or perl if not then ignore this hunk
-		next if ($realfile !~ /\.(h|c|cpp|pl)$/);
+# check we are in a valid source file; if not then tabs are allowed.
+# make an exception from some scripts imported from other projects.
+		next if ($realfile !~ /\.(h|c|cpp|pl|py|sh)$/);
+		next if ($realfile =~ /(checkpatch|get_maintainer|texi2pod)\.pl$/);
 
 # in QEMU, no tabs are allowed
 		if ($rawline =~ /^\+.*\t/) {
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/3] checkpatch: tweak the files in which TABs are checked

[Fam Zheng]

On Tue, 08/09 17:47, Paolo Bonzini wrote:
> Include Python and shell scripts, and make an exception for Perl
> scripts we imported from Linux or elsewhere.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  scripts/checkpatch.pl | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index b7cb4ab..7ccf6a8 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1351,8 +1351,10 @@ sub process {
>  			WARN("adding a line without newline at end of file\n" . $herecurr);
>  		}
>  
> -# check we are in a valid source file C or perl if not then ignore this hunk
> -		next if ($realfile !~ /\.(h|c|cpp|pl)$/);
> +# check we are in a valid source file; if not then tabs are allowed.
> +# make an exception from some scripts imported from other projects.

s/from some/for some/ ?

> +		next if ($realfile !~ /\.(h|c|cpp|pl|py|sh)$/);
> +		next if ($realfile =~ /(checkpatch|get_maintainer|texi2pod)\.pl$/);
>  
>  # in QEMU, no tabs are allowed
>  		if ($rawline =~ /^\+.*\t/) {
> -- 
> 2.7.4
> 
> 
> 

Re: [Qemu-devel] [PATCH 1/3] checkpatch: tweak the files in which TABs are checked

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Include Python and shell scripts, and make an exception for Perl
> scripts we imported from Linux or elsewhere.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  scripts/checkpatch.pl | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index b7cb4ab..7ccf6a8 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1351,8 +1351,10 @@ sub process {
>  			WARN("adding a line without newline at end of file\n" . $herecurr);
>  		}
>  
> -# check we are in a valid source file C or perl if not then ignore this hunk
> -		next if ($realfile !~ /\.(h|c|cpp|pl)$/);
> +# check we are in a valid source file; if not then tabs are allowed.
> +# make an exception from some scripts imported from other projects.
> +		next if ($realfile !~ /\.(h|c|cpp|pl|py|sh)$/);
> +		next if ($realfile =~ /(checkpatch|get_maintainer|texi2pod)\.pl$/);
>  
>  # in QEMU, no tabs are allowed
>  		if ($rawline =~ /^\+.*\t/) {

I'm afraid "if not then tabs are allowed" is confusing.  We're obviously
skipping more than just the tabs check: RCS/CVS revision markers, and a
whole bunch of C style checks.  Makes sense, we don't want to do these
checks for imported files.  "if not then tabs are allowed" starts to
make some sense only once you've stared at the next "next if ..." line
for a while.  Let's avoid that.  Minimal change:

   # check we are in a valid source file; if not then ignore this hunk
   # make an exception from some scripts imported from other projects.

Radim's "[PATCH] checkpatch: ignore automatically imported Linux
headers" adds a similar exception for other imported files in a
different place:

  @@ -1312,6 +1312,9 @@ sub process {
   # ignore non-hunk lines and lines being removed
                  next if (!$hunk_line || $line =~ /^-/);

  +# ignore files that are being periodically imported from Linux
  +		next if ($realfile =~ /^(linux-headers|include\/standard-headers)\//);
  +
   #trailing whitespace
                  if ($line =~ /^\+.*\015/) {
                          my $herevet = "$here\n" . cat_vet($rawline) . "\n";

Should both exceptions be in the same place?

Re: [Qemu-devel] [PATCH 1/3] checkpatch: tweak the files in which TABs are checked

[Cornelia Huck]

On Wed, 10 Aug 2016 08:46:07 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> I'm afraid "if not then tabs are allowed" is confusing.  We're obviously
> skipping more than just the tabs check: RCS/CVS revision markers, and a
> whole bunch of C style checks.  Makes sense, we don't want to do these
> checks for imported files.  "if not then tabs are allowed" starts to
> make some sense only once you've stared at the next "next if ..." line
> for a while.  Let's avoid that.  Minimal change:
> 
>    # check we are in a valid source file; if not then ignore this hunk
>    # make an exception from some scripts imported from other projects.
> 
> Radim's "[PATCH] checkpatch: ignore automatically imported Linux
> headers" adds a similar exception for other imported files in a
> different place:
> 
>   @@ -1312,6 +1312,9 @@ sub process {
>    # ignore non-hunk lines and lines being removed
>                   next if (!$hunk_line || $line =~ /^-/);
> 
>   +# ignore files that are being periodically imported from Linux
>   +		next if ($realfile =~ /^(linux-headers|include\/standard-headers)\//);
>   +
>    #trailing whitespace
>                   if ($line =~ /^\+.*\015/) {
>                           my $herevet = "$here\n" . cat_vet($rawline) . "\n";
> 
> Should both exceptions be in the same place?

There are two cases of 'imported file':
(a) Things like the headers update where we want to copy whatever we
got from the kernel: If coding style is already messed up there, we
still want to copy.
(b) Things like checkpatch.pl which we tweak ourselves: We want to keep
close to the existing coding style, but don't want to mess up things
further.

I think in the long run it would make sense to skip any checks for case
(a) but still keep a subset of checks (like trailing whitespace) for
(b). For the sake of silencing the checkpatch bot, just skipping (a)
and (b) makes sense for now.

[Qemu-devel] [PATCH 2/3] checkpatch: bump most warnings to errors

[Paolo Bonzini]

This only leaves a warning-level message for extra-long lines, which
are relatively common and cause patchew to send email that will likely
be ignored.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/checkpatch.pl | 66 +++++++++++++++++++++++++--------------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7ccf6a8..cea1997 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1289,11 +1289,11 @@ sub process {
 			# This is a signoff, if ugly, so do not double report.
 			$signoff++;
 			if (!($line =~ /^\s*Signed-off-by:/)) {
-				WARN("Signed-off-by: is the preferred form\n" .
+				ERROR("Signed-off-by: is the preferred form\n" .
 					$herecurr);
 			}
 			if ($line =~ /^\s*signed-off-by:\S/i) {
-				WARN("space required after Signed-off-by:\n" .
+				ERROR("space required after Signed-off-by:\n" .
 					$herecurr);
 			}
 		}
@@ -1343,12 +1343,12 @@ sub process {
 
 # check for spaces before a quoted newline
 		if ($rawline =~ /^.*\".*\s\\n/) {
-			WARN("unnecessary whitespace before a quoted newline\n" . $herecurr);
+			ERROR("unnecessary whitespace before a quoted newline\n" . $herecurr);
 		}
 
 # check for adding lines without a newline.
 		if ($line =~ /^\+/ && defined $lines[$linenr] && $lines[$linenr] =~ /^\\ No newline at end of file/) {
-			WARN("adding a line without newline at end of file\n" . $herecurr);
+			ERROR("adding a line without newline at end of file\n" . $herecurr);
 		}
 
 # check we are in a valid source file; if not then tabs are allowed.
@@ -1368,7 +1368,7 @@ sub process {
 
 # check for RCS/CVS revision markers
 		if ($rawline =~ /^\+.*\$(Revision|Log|Id)(?:\$|)/) {
-			WARN("CVS style keyword markers, these will _not_ be updated\n". $herecurr);
+			ERROR("CVS style keyword markers, these will _not_ be updated\n". $herecurr);
 		}
 
 # Check for potential 'bare' types
@@ -1500,7 +1500,7 @@ sub process {
 			{
 				my ($nlength, $nindent) = line_stats($lines[$ctx_ln - 1]);
 				if ($nindent > $indent) {
-					WARN("trailing semicolon indicates no statements, indent implies otherwise\n" .
+					ERROR("trailing semicolon indicates no statements, indent implies otherwise\n" .
 						"$here\n$ctx\n$rawlines[$ctx_ln - 1]\n");
 				}
 			}
@@ -1588,7 +1588,7 @@ sub process {
 
 			if ($check && (($sindent % 4) != 0 ||
 			    ($sindent <= $indent && $s ne ''))) {
-				WARN("suspect code indent for conditional statements ($indent, $sindent)\n" . $herecurr . "$stat_real\n");
+				ERROR("suspect code indent for conditional statements ($indent, $sindent)\n" . $herecurr . "$stat_real\n");
 			}
 		}
 
@@ -1766,7 +1766,7 @@ sub process {
 			} elsif ($ctx =~ /$Type$/) {
 
 			} else {
-				WARN("space prohibited between function name and open parenthesis '('\n" . $herecurr);
+				ERROR("space prohibited between function name and open parenthesis '('\n" . $herecurr);
 			}
 		}
 # Check operator spacing.
@@ -2005,7 +2005,7 @@ sub process {
 		if ($line =~ /^.\s*return\s*(E[A-Z]*)\s*;/) {
 			my $name = $1;
 			if ($name ne 'EOF' && $name ne 'ERROR') {
-				WARN("return of an errno should typically be -ve (return -$1)\n" . $herecurr);
+				ERROR("return of an errno should typically be -ve (return -$1)\n" . $herecurr);
 			}
 		}
 
@@ -2077,7 +2077,7 @@ sub process {
 				(?:\&\&|\|\||\)|\])
 			)/x)
 		{
-			WARN("boolean test with hexadecimal, perhaps just 1 \& or \|?\n" . $herecurr);
+			ERROR("boolean test with hexadecimal, perhaps just 1 \& or \|?\n" . $herecurr);
 		}
 
 # if and else should not have general statements after it
@@ -2133,7 +2133,7 @@ sub process {
 
 #no spaces allowed after \ in define
 		if ($line=~/\#\s*define.*\\\s$/) {
-			WARN("Whitepspace after \\ makes next lines useless\n" . $herecurr);
+			ERROR("Whitespace after \\ makes next lines useless\n" . $herecurr);
 		}
 
 # multi-statement macros should be enclosed in a do while loop, grab the
@@ -2285,7 +2285,7 @@ sub process {
 					}
 				}
 				if ($seen != ($#chunks + 1)) {
-					WARN("braces {} are necessary for all arms of this statement\n" . $herectx);
+					ERROR("braces {} are necessary for all arms of this statement\n" . $herectx);
 				}
 			}
 		}
@@ -2353,19 +2353,19 @@ sub process {
 					$herectx .= raw_line($linenr, $n) . "\n";;
 				}
 
-				WARN("braces {} are necessary even for single statement blocks\n" . $herectx);
+				ERROR("braces {} are necessary even for single statement blocks\n" . $herectx);
 			}
 		}
 
 # no volatiles please
 		my $asm_volatile = qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b};
 		if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) {
-			WARN("Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr);
+			ERROR("Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr);
 		}
 
 # warn about #if 0
 		if ($line =~ /^.\s*\#\s*if\s+0\b/) {
-			WARN("if this code is redundant consider removing it\n" .
+			ERROR("if this code is redundant consider removing it\n" .
 				$herecurr);
 		}
 
@@ -2373,7 +2373,7 @@ sub process {
 		if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
 			my $expr = $1;
 			if ($line =~ /\bg_free\(\Q$expr\E\);/) {
-				WARN("g_free(NULL) is safe this check is probably not required\n" . $hereprev);
+				ERROR("g_free(NULL) is safe this check is probably not required\n" . $hereprev);
 			}
 		}
 
@@ -2391,19 +2391,19 @@ sub process {
 # check for memory barriers without a comment.
 		if ($line =~ /\b(smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/) {
 			if (!ctx_has_comment($first_line, $linenr)) {
-				WARN("memory barrier without comment\n" . $herecurr);
+				ERROR("memory barrier without comment\n" . $herecurr);
 			}
 		}
 # check of hardware specific defines
 # we have e.g. CONFIG_LINUX and CONFIG_WIN32 for common cases
 # where they might be necessary.
 		if ($line =~ m@^.\s*\#\s*if.*\b__@) {
-			WARN("architecture specific defines should be avoided\n" .  $herecurr);
+			ERROR("architecture specific defines should be avoided\n" .  $herecurr);
 		}
 
 # Check that the storage class is at the beginning of a declaration
 		if ($line =~ /\b$Storage\b/ && $line !~ /^.\s*$Storage\b/) {
-			WARN("storage class should be at the beginning of the declaration\n" . $herecurr)
+			ERROR("storage class should be at the beginning of the declaration\n" . $herecurr)
 		}
 
 # check the location of the inline attribute, that it is between
@@ -2415,7 +2415,7 @@ sub process {
 
 # check for sizeof(&)
 		if ($line =~ /\bsizeof\s*\(\s*\&/) {
-			WARN("sizeof(& should be avoided\n" . $herecurr);
+			ERROR("sizeof(& should be avoided\n" . $herecurr);
 		}
 
 # check for new externs in .c files.
@@ -2432,40 +2432,40 @@ sub process {
 			if ($s =~ /^\s*;/ &&
 			    $function_name ne 'uninitialized_var')
 			{
-				WARN("externs should be avoided in .c files\n" .  $herecurr);
+				ERROR("externs should be avoided in .c files\n" .  $herecurr);
 			}
 
 			if ($paren_space =~ /\n/) {
-				WARN("arguments for function declarations should follow identifier\n" . $herecurr);
+				ERROR("arguments for function declarations should follow identifier\n" . $herecurr);
 			}
 
 		} elsif ($realfile =~ /\.c$/ && defined $stat &&
 		    $stat =~ /^.\s*extern\s+/)
 		{
-			WARN("externs should be avoided in .c files\n" .  $herecurr);
+			ERROR("externs should be avoided in .c files\n" .  $herecurr);
 		}
 
 # check for pointless casting of g_malloc return
 		if ($line =~ /\*\s*\)\s*g_(try)?(m|re)alloc(0?)(_n)?\b/) {
 			if ($2 == 'm') {
-				WARN("unnecessary cast may hide bugs, use g_$1new$3 instead\n" . $herecurr);
+				ERROR("unnecessary cast may hide bugs, use g_$1new$3 instead\n" . $herecurr);
 			} else {
-				WARN("unnecessary cast may hide bugs, use g_$1renew$3 instead\n" . $herecurr);
+				ERROR("unnecessary cast may hide bugs, use g_$1renew$3 instead\n" . $herecurr);
 			}
 		}
 
 # check for gcc specific __FUNCTION__
 		if ($line =~ /__FUNCTION__/) {
-			WARN("__func__ should be used instead of gcc specific __FUNCTION__\n"  . $herecurr);
+			ERROR("__func__ should be used instead of gcc specific __FUNCTION__\n"  . $herecurr);
 		}
 
 # recommend qemu_strto* over strto* for numeric conversions
 		if ($line =~ /\b(strto[^kd].*?)\s*\(/) {
-			WARN("consider using qemu_$1 in preference to $1\n" . $herecurr);
+			ERROR("consider using qemu_$1 in preference to $1\n" . $herecurr);
 		}
 # check for module_init(), use category-specific init macros explicitly please
 		if ($line =~ /^module_init\s*\(/) {
-			WARN("please use block_init(), type_init() etc. instead of module_init()\n" . $herecurr);
+			ERROR("please use block_init(), type_init() etc. instead of module_init()\n" . $herecurr);
 		}
 # check for various ops structs, ensure they are const.
 		my $struct_ops = qr{AIOCBInfo|
@@ -2490,7 +2490,7 @@ sub process {
 				VMStateInfo}x;
 		if ($line !~ /\bconst\b/ &&
 		    $line =~ /\b($struct_ops)\b/) {
-			WARN("struct $1 should normally be const\n" .
+			ERROR("struct $1 should normally be const\n" .
 				$herecurr);
 		}
 
@@ -2500,14 +2500,14 @@ sub process {
 			$string = substr($rawline, $-[1], $+[1] - $-[1]);
 			$string =~ s/%%/__/g;
 			if ($string =~ /(?<!%)%L[udi]/) {
-				WARN("\%Ld/%Lu are not-standard C, use %lld/%llu\n" . $herecurr);
+				ERROR("\%Ld/%Lu are not-standard C, use %lld/%llu\n" . $herecurr);
 				last;
 			}
 		}
 
 # QEMU specific tests
 		if ($rawline =~ /\b(?:Qemu|QEmu)\b/) {
-			WARN("use QEMU instead of Qemu or QEmu\n" . $herecurr);
+			ERROR("use QEMU instead of Qemu or QEmu\n" . $herecurr);
 		}
 
 # Qemu error function tests
@@ -2521,7 +2521,7 @@ sub process {
 				error_report}x;
 
 	if ($rawline =~ /\b(?:$qemu_error_funcs)\s*\(\s*\".*\\n/) {
-		WARN("Error messages should not contain newlines\n" . $herecurr);
+		ERROR("Error messages should not contain newlines\n" . $herecurr);
 	}
 
 	# Continue checking for error messages that contains newlines. This
@@ -2542,7 +2542,7 @@ sub process {
 		}
 
 		if ($rawlines[$i] =~ /\b(?:$qemu_error_funcs)\s*\(/) {
-			WARN("Error messages should not contain newlines\n" . $herecurr);
+			ERROR("Error messages should not contain newlines\n" . $herecurr);
 		}
 	}
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/3] checkpatch: bump most warnings to errors

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> This only leaves a warning-level message for extra-long lines, which
> are relatively common and cause patchew to send email that will likely
> be ignored.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Are we ready to give up on illegibly long lines?

What happened to "[PATCH 2/4] CODING_STYLE, checkpatch: update line
length rules"?

Re: [Qemu-devel] [PATCH 2/3] checkpatch: bump most warnings to errors

[Paolo Bonzini]

> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > This only leaves a warning-level message for extra-long lines, which
> > are relatively common and cause patchew to send email that will likely
> > be ignored.
> >
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Are we ready to give up on illegibly long lines?

We have other levels of code review than checkpatch.  80 chars can be
illegibly short in some circumstances where 83 or 84 are enough.

Paolo

> What happened to "[PATCH 2/4] CODING_STYLE, checkpatch: update line
> length rules"?

Re: [Qemu-devel] [PATCH 2/3] checkpatch: bump most warnings to errors

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>> > This only leaves a warning-level message for extra-long lines, which
>> > are relatively common and cause patchew to send email that will likely
>> > be ignored.
>> >
>> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> 
>> Are we ready to give up on illegibly long lines?
>
> We have other levels of code review than checkpatch.  80 chars can be
> illegibly short in some circumstances where 83 or 84 are enough.

Isn't that addressed neatly in your patch?  It has a soft and a hard
limit.  Exceeding the hard limit is an error, exceeding the soft limit
is a warning.  I rather liked that.  If I remember correctly, the only
disagreements were about the value of the soft limit.

> Paolo
>
>> What happened to "[PATCH 2/4] CODING_STYLE, checkpatch: update line
>> length rules"?

Re: [Qemu-devel] [PATCH 2/3] checkpatch: bump most warnings to errors

[Paolo Bonzini]

----- Original Message -----
> From: "Markus Armbruster" <armbru@redhat.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: thuth@redhat.com, famz@redhat.com, qemu-devel@nongnu.org
> Sent: Wednesday, August 10, 2016 9:48:24 AM
> Subject: Re: [Qemu-devel] [PATCH 2/3] checkpatch: bump most warnings to errors
> 
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> >> Paolo Bonzini <pbonzini@redhat.com> writes:
> >> 
> >> > This only leaves a warning-level message for extra-long lines, which
> >> > are relatively common and cause patchew to send email that will likely
> >> > be ignored.
> >> >
> >> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> 
> >> Are we ready to give up on illegibly long lines?
> >
> > We have other levels of code review than checkpatch.  80 chars can be
> > illegibly short in some circumstances where 83 or 84 are enough.
> 
> Isn't that addressed neatly in your patch?  It has a soft and a hard
> limit.  Exceeding the hard limit is an error, exceeding the soft limit
> is a warning.  I rather liked that.  If I remember correctly, the only
> disagreements were about the value of the soft limit.

Yes, indeed.  I can respin the patch then.

Paolo

Re: [Qemu-devel] [PATCH 2/3] checkpatch: bump most warnings to errors

[Cornelia Huck]

On Wed, 10 Aug 2016 03:08:33 -0400 (EDT)
Paolo Bonzini <pbonzini@redhat.com> wrote:

> > Paolo Bonzini <pbonzini@redhat.com> writes:
> > 
> > > This only leaves a warning-level message for extra-long lines, which
> > > are relatively common and cause patchew to send email that will likely
> > > be ignored.
> > >
> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > Are we ready to give up on illegibly long lines?
> 
> We have other levels of code review than checkpatch.  80 chars can be
> illegibly short in some circumstances where 83 or 84 are enough.

We could leave the WARN at 80 chars and add an error at 120 chars or so.

Re: [Qemu-devel] [PATCH 2/3] checkpatch: bump most warnings to errors

[Cornelia Huck]

On Tue,  9 Aug 2016 17:47:43 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> This only leaves a warning-level message for extra-long lines, which
> are relatively common and cause patchew to send email that will likely
> be ignored.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  scripts/checkpatch.pl | 66 +++++++++++++++++++++++++--------------------------
>  1 file changed, 33 insertions(+), 33 deletions(-)
> 

>  # no volatiles please
>  		my $asm_volatile = qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b};
>  		if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) {
> -			WARN("Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr);
> +			ERROR("Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr);

It's a bit weird to have this refer to a Linux file :)

Re: [Qemu-devel] [PATCH 2/3] checkpatch: bump most warnings to errors

[Paolo Bonzini]

----- Original Message -----
> From: "Cornelia Huck" <cornelia.huck@de.ibm.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: qemu-devel@nongnu.org, thuth@redhat.com, famz@redhat.com, armbru@redhat.com
> Sent: Wednesday, August 10, 2016 9:46:14 AM
> Subject: Re: [Qemu-devel] [PATCH 2/3] checkpatch: bump most warnings to errors
> 
> On Tue,  9 Aug 2016 17:47:43 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > This only leaves a warning-level message for extra-long lines, which
> > are relatively common and cause patchew to send email that will likely
> > be ignored.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  scripts/checkpatch.pl | 66
> >  +++++++++++++++++++++++++--------------------------
> >  1 file changed, 33 insertions(+), 33 deletions(-)
> > 
> 
> >  # no volatiles please
> >  		my $asm_volatile = qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b};
> >  		if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) {
> > -			WARN("Use of volatile is usually wrong: see
> > Documentation/volatile-considered-harmful.txt\n" . $herecurr);
> > +			ERROR("Use of volatile is usually wrong: see
> > Documentation/volatile-considered-harmful.txt\n" . $herecurr);
> 
> It's a bit weird to have this refer to a Linux file :)

Right.  We should include the explanation in docs/atomics.txt and point
to that.

Paolo

[Qemu-devel] [PATCH 3/3] checkpatch: default to success if only warnings

[Paolo Bonzini]

CHK-level checks have been removed from checkpatch or bumped to
errors, so there is no effect anymore for --strict/--subjective.
Furthermore, even most WARNs have been bumped to errors, with
WARN only reserved to things that patchew probably ought not
to complain about (and that maintainers probably will notice
anyway during review if they are extreme).

Default to exiting with success even if there are WARN-level
failures, and cause --strict to fail for warnings.  Maintainers
that want to have a strict 80-character limit for their subsystem
can add it to a commit hook for example.

The --subjective synonym is removed.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/checkpatch.pl | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index cea1997..3701020 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -22,7 +22,7 @@ my $tst_only;
 my $emacs = 0;
 my $terse = 0;
 my $file = 0;
-my $check = 0;
+my $no_warnings = 0;
 my $summary = 1;
 my $mailback = 0;
 my $summary_file = 0;
@@ -45,7 +45,7 @@ Options:
   --emacs                    emacs compile window format
   --terse                    one line per report
   -f, --file                 treat FILE as regular source file
-  --subjective, --strict     enable more subjective tests
+  --strict                   fail if only warnings are found
   --root=PATH                PATH to the kernel tree root
   --no-summary               suppress the per-file summary
   --mailback                 only produce a report in case of warnings/errors
@@ -71,8 +71,7 @@ GetOptions(
 	'emacs!'	=> \$emacs,
 	'terse!'	=> \$terse,
 	'f|file!'	=> \$file,
-	'subjective!'	=> \$check,
-	'strict!'	=> \$check,
+	'strict!'	=> \$no_warnings,
 	'root=s'	=> \$root,
 	'summary!'	=> \$summary,
 	'mailback!'	=> \$mailback,
@@ -1072,12 +1071,6 @@ sub WARN {
 		our $cnt_warn++;
 	}
 }
-sub CHK {
-	if ($check && report("CHECK: $_[0]\n")) {
-		our $clean = 0;
-		our $cnt_chk++;
-	}
-}
 
 sub process {
 	my $filename = shift;
@@ -2590,7 +2583,6 @@ sub process {
 	if ($summary && !($clean == 1 && $quiet == 1)) {
 		print "$filename " if ($summary_file);
 		print "total: $cnt_error errors, $cnt_warn warnings, " .
-			(($check)? "$cnt_chk checks, " : "") .
 			"$cnt_lines lines checked\n";
 		print "\n" if ($quiet == 0);
 	}
@@ -2613,5 +2605,5 @@ sub process {
 		print "CHECKPATCH in MAINTAINERS.\n";
 	}
 
-	return $clean;
+	return ($no_warnings ? $clean : $cnt_error == 0);
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/3] checkpatch tweaks

[Fam Zheng]

On Tue, 08/09 17:47, Paolo Bonzini wrote:
> My proposal after having watched patchew complain for a couple days
> about patches being sent on the list.

Looks good to me, I think patchew will just work as you want (be silent with
long line warnings) once these are merged.

Fam

Re: [Qemu-devel] [PATCH 0/3] checkpatch tweaks

[Christian Borntraeger]

On 08/10/2016 04:10 AM, Fam Zheng wrote:
> On Tue, 08/09 17:47, Paolo Bonzini wrote:
>> My proposal after having watched patchew complain for a couple days
>> about patches being sent on the list.
> 
> Looks good to me, I think patchew will just work as you want (be silent with
> long line warnings) once these are merged.
> 
> Fam

Together with the exclusion of linux-headers this looks like the right
thing to do. Thanks

Re: [Qemu-devel] [PATCH 0/3] checkpatch tweaks

[Cornelia Huck]

On Tue,  9 Aug 2016 17:47:41 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> My proposal after having watched patchew complain for a couple days
> about patches being sent on the list.
> 
> Paolo
> 
> Paolo Bonzini (3):
>   checkpatch: tweak the files in which TABs are checked
>   checkpatch: bump most warnings to errors
>   checkpatch: default to success if only warnings
> 
>  scripts/checkpatch.pl | 88 ++++++++++++++++++++++++---------------------------
>  1 file changed, 41 insertions(+), 47 deletions(-)
> 

Sounds like a good way to go about this.

See my comments for some of the individual checkpatch changes, though.