[v2] checkpatch: detect missing changes to trace-events

[RFC v2] checkpatch: detect missing changes to trace-events

Posted by Claudio Fontana 3 years, 8 months ago

Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
 scripts/checkpatch.pl | 37 ++++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

v1 -> v2 :

* track the "from" file in addition to the "to" file,
  and grep into both (if they exist), looking for trace.h, trace-root.h

  If files are reachable and readable, emit a warning if there is no
  update to trace-events.


diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bd3faa154c..37db212fc6 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1300,6 +1300,7 @@ sub process {
 	my $in_header_lines = $file ? 0 : 1;
 	my $in_commit_log = 0;		#Scanning lines before patch
 	my $reported_maintainer_file = 0;
+	my $reported_trace_events_file = 0;
 	my $non_utf8_charset = 0;
 
 	our @report = ();
@@ -1309,6 +1310,7 @@ sub process {
 	our $cnt_chk = 0;
 
 	# Trace the real file/line as we go.
+	my $fromfile = '';
 	my $realfile = '';
 	my $realline = 0;
 	my $realcnt = 0;
@@ -1454,10 +1456,15 @@ sub process {
 		$here = "#$realline: " if ($file);
 
 		# extract the filename as it passes
-		if ($line =~ /^diff --git.*?(\S+)$/) {
-			$realfile = $1;
-			$realfile =~ s@^([^/]*)/@@ if (!$file);
+		if ($line =~ /^diff --git.*?(\S+).*?(\S+)$/) {
+			$fromfile = $1;
+			$realfile = $2;
+			if (!$file) {
+				$fromfile =~ s@^([^/]*)/@@ ;
+				$realfile =~ s@^([^/]*)/@@ ;
+			}
 	                checkfilename($realfile, \$acpi_testexpected, \$acpi_nontestexpected);
+
 		} elsif ($line =~ /^\+\+\+\s+(\S+)/) {
 			$realfile = $1;
 			$realfile =~ s@^([^/]*)/@@ if (!$file);
@@ -1470,6 +1477,11 @@ sub process {
 			}
 
 			next;
+
+		} elsif ($line =~ /^---\s+(\S+)/) {
+			$fromfile = $1;
+			$fromfile =~ s@^([^/]*)/@@ if (!$file);
+			next;
 		}
 
 		$here .= "FILE: $realfile:$realline:" if ($realcnt != 0);
@@ -1524,15 +1536,26 @@ sub process {
 		if ($line =~ /^\s*MAINTAINERS\s*\|/) {
 			$reported_maintainer_file = 1;
 		}
-
+# similar check for trace-events
+		if ($line =~ /^\s*trace-events\s*\|/) {
+			$reported_trace_events_file = 1;
+		}
 # Check for added, moved or deleted files
-		if (!$reported_maintainer_file && !$in_commit_log &&
+		if (!$in_commit_log &&
 		    ($line =~ /^(?:new|deleted) file mode\s*\d+\s*$/ ||
 		     $line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ ||
 		     ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ &&
 		      (defined($1) || defined($2))))) {
-			$reported_maintainer_file = 1;
-			WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
+			if (!$reported_maintainer_file) {
+				$reported_maintainer_file = 1;
+				WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
+			}
+			if (!$reported_trace_events_file) {
+				if (`grep -F -s -e trace.h -e trace-root.h ${fromfile} ${realfile}` ne '') {
+					$reported_trace_events_file = 1;
+					WARN("added, moved or deleted file(s), does trace-events need updating?\n" . $herecurr);
+				}
+			}
 		}
 
 # Check for wrappage within a valid hunk of the file
-- 
2.16.4

Re: [RFC v2] checkpatch: detect missing changes to trace-events

Posted by no-reply@patchew.org 3 years, 8 months ago

Patchew URL: https://patchew.org/QEMU/20200807111447.15599-1-cfontana@suse.de/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===




The full log is available at
http://patchew.org/logs/20200807111447.15599-1-cfontana@suse.de/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [RFC v2] checkpatch: detect missing changes to trace-events

Posted by Stefan Hajnoczi 3 years, 8 months ago

On Fri, Aug 07, 2020 at 01:14:47PM +0200, Claudio Fontana wrote:
>  # Check for added, moved or deleted files
> -		if (!$reported_maintainer_file && !$in_commit_log &&
> +		if (!$in_commit_log &&
>  		    ($line =~ /^(?:new|deleted) file mode\s*\d+\s*$/ ||
>  		     $line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ ||
>  		     ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ &&
>  		      (defined($1) || defined($2))))) {
> -			$reported_maintainer_file = 1;
> -			WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
> +			if (!$reported_maintainer_file) {
> +				$reported_maintainer_file = 1;
> +				WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
> +			}
> +			if (!$reported_trace_events_file) {
> +				if (`grep -F -s -e trace.h -e trace-root.h ${fromfile} ${realfile}` ne '') {

Are there false positives on non-C files (e.g. Makefiles)?

The search expressions can be tightened to avoid false positives (at the
cost of possible false negatives): -e '#include "trace.h"' -e '#include
"trace-root.h"'. This way a C file containing "strace.handler" will not
cause a false positive.

I wonder if there is a native Perl way to do this search instead of
forking grep :). Nevermind though.

Re: [RFC v2] checkpatch: detect missing changes to trace-events

Posted by Claudio Fontana 3 years, 8 months ago

On 8/12/20 5:51 PM, Stefan Hajnoczi wrote:
> On Fri, Aug 07, 2020 at 01:14:47PM +0200, Claudio Fontana wrote:
>>  # Check for added, moved or deleted files
>> -		if (!$reported_maintainer_file && !$in_commit_log &&
>> +		if (!$in_commit_log &&
>>  		    ($line =~ /^(?:new|deleted) file mode\s*\d+\s*$/ ||
>>  		     $line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ ||
>>  		     ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ &&
>>  		      (defined($1) || defined($2))))) {
>> -			$reported_maintainer_file = 1;
>> -			WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
>> +			if (!$reported_maintainer_file) {
>> +				$reported_maintainer_file = 1;
>> +				WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
>> +			}
>> +			if (!$reported_trace_events_file) {
>> +				if (`grep -F -s -e trace.h -e trace-root.h ${fromfile} ${realfile}` ne '') {
> 
> Are there false positives on non-C files (e.g. Makefiles)?
> 
> The search expressions can be tightened to avoid false positives (at the
> cost of possible false negatives): -e '#include "trace.h"' -e '#include
> "trace-root.h"'. This way a C file containing "strace.handler" will not
> cause a false positive.
> 

Yep good point.

> I wonder if there is a native Perl way to do this search instead of
> forking grep :). Nevermind though.
> 

If only all the speedups from GNU grep would be available as a libgrep..

I had to post an RFC v3 of this one, because there is an issue in the order in_commit_log is set and checked (in my understanding).

https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg01953.html

This is actually potentially an issue in upstream (kernel) checkpatch.pl as well, but it does not bite until you try to use
realfile variable (or in this case fromfile also).

Ciao,

Claudio

Re: [RFC v2] checkpatch: detect missing changes to trace-events

Posted by Markus Armbruster 3 years, 8 months ago

Claudio Fontana <cfontana@suse.de> writes:

> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  scripts/checkpatch.pl | 37 ++++++++++++++++++++++++++++++-------
>  1 file changed, 30 insertions(+), 7 deletions(-)
>
> v1 -> v2 :
>
> * track the "from" file in addition to the "to" file,
>   and grep into both (if they exist), looking for trace.h, trace-root.h
>
>   If files are reachable and readable, emit a warning if there is no
>   update to trace-events.
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index bd3faa154c..37db212fc6 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1300,6 +1300,7 @@ sub process {
>  	my $in_header_lines = $file ? 0 : 1;
>  	my $in_commit_log = 0;		#Scanning lines before patch
>  	my $reported_maintainer_file = 0;
> +	my $reported_trace_events_file = 0;
>  	my $non_utf8_charset = 0;
>  
>  	our @report = ();
> @@ -1309,6 +1310,7 @@ sub process {
>  	our $cnt_chk = 0;
>  
>  	# Trace the real file/line as we go.
> +	my $fromfile = '';
>  	my $realfile = '';
>  	my $realline = 0;
>  	my $realcnt = 0;
> @@ -1454,10 +1456,15 @@ sub process {
>  		$here = "#$realline: " if ($file);
>  
>  		# extract the filename as it passes
> -		if ($line =~ /^diff --git.*?(\S+)$/) {
> -			$realfile = $1;
> -			$realfile =~ s@^([^/]*)/@@ if (!$file);
> +		if ($line =~ /^diff --git.*?(\S+).*?(\S+)$/) {
> +			$fromfile = $1;
> +			$realfile = $2;
> +			if (!$file) {
> +				$fromfile =~ s@^([^/]*)/@@ ;
> +				$realfile =~ s@^([^/]*)/@@ ;
> +			}
>  	                checkfilename($realfile, \$acpi_testexpected, \$acpi_nontestexpected);
> +

Drop this blank line.

>  		} elsif ($line =~ /^\+\+\+\s+(\S+)/) {
>  			$realfile = $1;
>  			$realfile =~ s@^([^/]*)/@@ if (!$file);
> @@ -1470,6 +1477,11 @@ sub process {
>  			}
>  
>  			next;
> +

And this one.

> +		} elsif ($line =~ /^---\s+(\S+)/) {
> +			$fromfile = $1;
> +			$fromfile =~ s@^([^/]*)/@@ if (!$file);
> +			next;
>  		}
>  
>  		$here .= "FILE: $realfile:$realline:" if ($realcnt != 0);

Aside: I don't understand why we need to match both the diff line and
the --- line (and now the +++ line, too).

> @@ -1524,15 +1536,26 @@ sub process {
>  		if ($line =~ /^\s*MAINTAINERS\s*\|/) {
>  			$reported_maintainer_file = 1;
>  		}
> -
> +# similar check for trace-events
> +		if ($line =~ /^\s*trace-events\s*\|/) {
> +			$reported_trace_events_file = 1;
> +		}

These are meant to match in the diffstat (took me a stare to figure that
out).

The existing one matches MAINTAINERS in the source root.  Good; that's
where it is.

The new one matches trace-events in the source root.  Not so good; We
use one trace-events per directory.

If I update trace-events in the source root, I won't be warned about
other trace-events in need of updating (false negative), will I?

If I don't update trace-events in the source root, I will be warned
regardless of what other trace-events I update (false positive), won't
I?

>  # Check for added, moved or deleted files
> -		if (!$reported_maintainer_file && !$in_commit_log &&
> +		if (!$in_commit_log &&
>  		    ($line =~ /^(?:new|deleted) file mode\s*\d+\s*$/ ||
>  		     $line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ ||
>  		     ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ &&
>  		      (defined($1) || defined($2))))) {
> -			$reported_maintainer_file = 1;
> -			WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
> +			if (!$reported_maintainer_file) {
> +				$reported_maintainer_file = 1;
> +				WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
> +			}
> +			if (!$reported_trace_events_file) {
> +				if (`grep -F -s -e trace.h -e trace-root.h ${fromfile} ${realfile}` ne '') {
> +					$reported_trace_events_file = 1;
> +					WARN("added, moved or deleted file(s), does trace-events need updating?\n" . $herecurr);
> +				}
> +			}
>  		}
>  
>  # Check for wrappage within a valid hunk of the file

Regarding Stefan's observations:

* Yes, the grep patterns need tightening.

* Yes, forking grep could be replaced by a simple function that slurps
  in the file and searches it.  Could doesn't imply should, let alome
  must.

As discussed in review of v1, I'm not sure checkpatch.pl is the best
home for this kind of check.  I'm not going to reject a working patch
because of that.

Re: [RFC v2] checkpatch: detect missing changes to trace-events

Posted by Claudio Fontana 3 years, 8 months ago

On 9/2/20 5:14 PM, Markus Armbruster wrote:
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>>  scripts/checkpatch.pl | 37 ++++++++++++++++++++++++++++++-------
>>  1 file changed, 30 insertions(+), 7 deletions(-)
>>
>> v1 -> v2 :
>>
>> * track the "from" file in addition to the "to" file,
>>   and grep into both (if they exist), looking for trace.h, trace-root.h
>>
>>   If files are reachable and readable, emit a warning if there is no
>>   update to trace-events.
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index bd3faa154c..37db212fc6 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -1300,6 +1300,7 @@ sub process {
>>  	my $in_header_lines = $file ? 0 : 1;
>>  	my $in_commit_log = 0;		#Scanning lines before patch
>>  	my $reported_maintainer_file = 0;
>> +	my $reported_trace_events_file = 0;
>>  	my $non_utf8_charset = 0;
>>  
>>  	our @report = ();
>> @@ -1309,6 +1310,7 @@ sub process {
>>  	our $cnt_chk = 0;
>>  
>>  	# Trace the real file/line as we go.
>> +	my $fromfile = '';
>>  	my $realfile = '';
>>  	my $realline = 0;
>>  	my $realcnt = 0;
>> @@ -1454,10 +1456,15 @@ sub process {
>>  		$here = "#$realline: " if ($file);
>>  
>>  		# extract the filename as it passes
>> -		if ($line =~ /^diff --git.*?(\S+)$/) {
>> -			$realfile = $1;
>> -			$realfile =~ s@^([^/]*)/@@ if (!$file);
>> +		if ($line =~ /^diff --git.*?(\S+).*?(\S+)$/) {
>> +			$fromfile = $1;
>> +			$realfile = $2;
>> +			if (!$file) {
>> +				$fromfile =~ s@^([^/]*)/@@ ;
>> +				$realfile =~ s@^([^/]*)/@@ ;
>> +			}
>>  	                checkfilename($realfile, \$acpi_testexpected, \$acpi_nontestexpected);
>> +
> 
> Drop this blank line.
> 
>>  		} elsif ($line =~ /^\+\+\+\s+(\S+)/) {
>>  			$realfile = $1;
>>  			$realfile =~ s@^([^/]*)/@@ if (!$file);
>> @@ -1470,6 +1477,11 @@ sub process {
>>  			}
>>  
>>  			next;
>> +
> 
> And this one.
> 
>> +		} elsif ($line =~ /^---\s+(\S+)/) {
>> +			$fromfile = $1;
>> +			$fromfile =~ s@^([^/]*)/@@ if (!$file);
>> +			next;
>>  		}
>>  
>>  		$here .= "FILE: $realfile:$realline:" if ($realcnt != 0);
> 
> Aside: I don't understand why we need to match both the diff line and
> the --- line (and now the +++ line, too).
> 
>> @@ -1524,15 +1536,26 @@ sub process {
>>  		if ($line =~ /^\s*MAINTAINERS\s*\|/) {
>>  			$reported_maintainer_file = 1;
>>  		}
>> -
>> +# similar check for trace-events
>> +		if ($line =~ /^\s*trace-events\s*\|/) {
>> +			$reported_trace_events_file = 1;
>> +		}
> 
> These are meant to match in the diffstat (took me a stare to figure that
> out).
> 
> The existing one matches MAINTAINERS in the source root.  Good; that's
> where it is.
> 
> The new one matches trace-events in the source root.  Not so good; We
> use one trace-events per directory.
> 
> If I update trace-events in the source root, I won't be warned about
> other trace-events in need of updating (false negative), will I?
> 
> If I don't update trace-events in the source root, I will be warned
> regardless of what other trace-events I update (false positive), won't
> I?
> 
>>  # Check for added, moved or deleted files
>> -		if (!$reported_maintainer_file && !$in_commit_log &&
>> +		if (!$in_commit_log &&
>>  		    ($line =~ /^(?:new|deleted) file mode\s*\d+\s*$/ ||
>>  		     $line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ ||
>>  		     ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ &&
>>  		      (defined($1) || defined($2))))) {
>> -			$reported_maintainer_file = 1;
>> -			WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
>> +			if (!$reported_maintainer_file) {
>> +				$reported_maintainer_file = 1;
>> +				WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
>> +			}
>> +			if (!$reported_trace_events_file) {
>> +				if (`grep -F -s -e trace.h -e trace-root.h ${fromfile} ${realfile}` ne '') {
>> +					$reported_trace_events_file = 1;
>> +					WARN("added, moved or deleted file(s), does trace-events need updating?\n" . $herecurr);
>> +				}
>> +			}
>>  		}
>>  
>>  # Check for wrappage within a valid hunk of the file
> 
> Regarding Stefan's observations:
> 
> * Yes, the grep patterns need tightening.
> 
> * Yes, forking grep could be replaced by a simple function that slurps
>   in the file and searches it.  Could doesn't imply should, let alome
>   must.
> 
> As discussed in review of v1, I'm not sure checkpatch.pl is the best
> home for this kind of check.  I'm not going to reject a working patch
> because of that.
> 

Hi Marcus,

I will rethink this in the future, thanks for the useful feedback,
but I think this needs to be rethought, reworked and tested more.

Thanks!

Claudio