[PATCH] checkpatch: fix CONST_STRUCT when const_structs.checkpatch is missing

[PATCH] checkpatch: fix CONST_STRUCT when const_structs.checkpatch is missing

[Quentin Monnet]

Checkpatch reports warnings when some specific structs are not declared
as const in the code. The list of structs to consider was initially
defined in the checkpatch.pl script itself, but it was later moved to an
external file (scripts/const_structs.checkpatch). This introduced two
minor issues:

- When file scripts/const_structs.checkpatch is not present (for
  example, if checkpatch is run outside of the kernel directory with the
  "--no-tree" option), a warning is printed to stderr to tell the user
  that "No structs that should be const will be found". This is fair,
  but the warning is printed unconditionally, even if the option
  "--ignore CONST_STRUCT" is passed. In the latter case, we explicitly
  ask checkpatch to skip this check, so no warning should be printed.

- When scripts/const_structs.checkpatch is missing, or even when trying
  to silence the warning by adding an empty file, $const_structs is set
  to "", and the regex used for finding structs that should be const,
  "$line =~ /\bstruct\s+($const_structs)\b(?!\s*\{)/)", matches all
  structs found in the code, thus reporting a number of false positives.

Let's fix the first item by skipping scripts/const_structs.checkpatch
processing if "CONST_STRUCT" checks are ignored, and the second one by
skipping the test if $const_structs is an empty string.

Fixes: bf1fa1dae68e ("checkpatch: externalize the structs that should be const")
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
---
 scripts/checkpatch.pl | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index b06093777fd8..dcbf4ff5d445 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -781,8 +781,10 @@ sub read_words {
 }
 
 my $const_structs = "";
-read_words(\$const_structs, $conststructsfile)
-    or warn "No structs that should be const will be found - file '$conststructsfile': $!\n";
+if (show_type("CONST_STRUCT")) {
+	read_words(\$const_structs, $conststructsfile)
+	    or warn "No structs that should be const will be found - file '$conststructsfile': $!\n";
+}
 
 my $typeOtherTypedefs = "";
 if (length($typedefsfile)) {
@@ -6660,7 +6662,8 @@ sub process {
 
 # check for various structs that are normally const (ops, kgdb, device_tree)
 # and avoid what seem like struct definitions 'struct foo {'
-		if ($line !~ /\bconst\b/ &&
+		if ($const_structs ne "" &&
+		    $line !~ /\bconst\b/ &&
 		    $line =~ /\bstruct\s+($const_structs)\b(?!\s*\{)/) {
 			WARN("CONST_STRUCT",
 			     "struct $1 should normally be const\n" . $herecurr);
-- 
2.20.1

Re: [PATCH] checkpatch: fix CONST_STRUCT when const_structs.checkpatch is missing

[Joe Perches]

On Mon, 2020-06-22 at 21:48 +0100, Quentin Monnet wrote:
> Checkpatch reports warnings when some specific structs are not declared
> as const in the code. The list of structs to consider was initially
> defined in the checkpatch.pl script itself, but it was later moved to an
> external file (scripts/const_structs.checkpatch). This introduced two
> minor issues:
> 
> - When file scripts/const_structs.checkpatch is not present (for
>   example, if checkpatch is run outside of the kernel directory with the
>   "--no-tree" option), a warning is printed to stderr to tell the user
>   that "No structs that should be const will be found". This is fair,
>   but the warning is printed unconditionally, even if the option
>   "--ignore CONST_STRUCT" is passed. In the latter case, we explicitly
>   ask checkpatch to skip this check, so no warning should be printed.
> 
> - When scripts/const_structs.checkpatch is missing, or even when trying
>   to silence the warning by adding an empty file, $const_structs is set
>   to "", and the regex used for finding structs that should be const,
>   "$line =~ /\bstruct\s+($const_structs)\b(?!\s*\{)/)", matches all
>   structs found in the code, thus reporting a number of false positives.
> 
> Let's fix the first item by skipping scripts/const_structs.checkpatch
> processing if "CONST_STRUCT" checks are ignored, and the second one by
> skipping the test if $const_structs is an empty string.
> 
> Fixes: bf1fa1dae68e ("checkpatch: externalize the structs that should be const")

Probably not worthy of a Fixes: line, as that's
generally used for backporting, but OK by me.

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -781,8 +781,10 @@ sub read_words {
>  }
>  
>  my $const_structs = "";

This might be a tiny bit faster/less cpu using:

my $const_structs;

> -read_words(\$const_structs, $conststructsfile)
> -    or warn "No structs that should be const will be found - file '$conststructsfile': $!\n";
> +if (show_type("CONST_STRUCT")) {
> +	read_words(\$const_structs, $conststructsfile)
> +	    or warn "No structs that should be const will be found - file '$conststructsfile': $!\n";
> +}
>  
>  my $typeOtherTypedefs = "";
>  if (length($typedefsfile)) {
> @@ -6660,7 +6662,8 @@ sub process {
>  
>  # check for various structs that are normally const (ops, kgdb, device_tree)
>  # and avoid what seem like struct definitions 'struct foo {'
> -		if ($line !~ /\bconst\b/ &&
> +		if ($const_structs ne "" &&

instead testing

		if (defined($const_structs) &&

> +		    $line !~ /\bconst\b/ &&
>  		    $line =~ /\bstruct\s+($const_structs)\b(?!\s*\{)/) {
>  			WARN("CONST_STRUCT",
>  			     "struct $1 should normally be const\n" . $herecurr);

Re: [PATCH] checkpatch: fix CONST_STRUCT when const_structs.checkpatch is missing

[Quentin Monnet]

2020-06-22 14:24 UTC-0700 ~ Joe Perches <joe@perches.com>
> On Mon, 2020-06-22 at 21:48 +0100, Quentin Monnet wrote:
>> Checkpatch reports warnings when some specific structs are not declared
>> as const in the code. The list of structs to consider was initially
>> defined in the checkpatch.pl script itself, but it was later moved to an
>> external file (scripts/const_structs.checkpatch). This introduced two
>> minor issues:
>>
>> - When file scripts/const_structs.checkpatch is not present (for
>>   example, if checkpatch is run outside of the kernel directory with the
>>   "--no-tree" option), a warning is printed to stderr to tell the user
>>   that "No structs that should be const will be found". This is fair,
>>   but the warning is printed unconditionally, even if the option
>>   "--ignore CONST_STRUCT" is passed. In the latter case, we explicitly
>>   ask checkpatch to skip this check, so no warning should be printed.
>>
>> - When scripts/const_structs.checkpatch is missing, or even when trying
>>   to silence the warning by adding an empty file, $const_structs is set
>>   to "", and the regex used for finding structs that should be const,
>>   "$line =~ /\bstruct\s+($const_structs)\b(?!\s*\{)/)", matches all
>>   structs found in the code, thus reporting a number of false positives.
>>
>> Let's fix the first item by skipping scripts/const_structs.checkpatch
>> processing if "CONST_STRUCT" checks are ignored, and the second one by
>> skipping the test if $const_structs is an empty string.
>>
>> Fixes: bf1fa1dae68e ("checkpatch: externalize the structs that should be const")
> 
> Probably not worthy of a Fixes: line, as that's
> generally used for backporting, but OK by me.

Yeah I hesitated, I'll drop it for v2.

> 
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
>> @@ -781,8 +781,10 @@ sub read_words {
>>  }
>>  
>>  my $const_structs = "";
> 
> This might be a tiny bit faster/less cpu using:
> 
> my $const_structs;
> 
>> -read_words(\$const_structs, $conststructsfile)
>> -    or warn "No structs that should be const will be found - file '$conststructsfile': $!\n";
>> +if (show_type("CONST_STRUCT")) {
>> +	read_words(\$const_structs, $conststructsfile)
>> +	    or warn "No structs that should be const will be found - file '$conststructsfile': $!\n";
>> +}
>>  
>>  my $typeOtherTypedefs = "";
>>  if (length($typedefsfile)) {
>> @@ -6660,7 +6662,8 @@ sub process {
>>  
>>  # check for various structs that are normally const (ops, kgdb, device_tree)
>>  # and avoid what seem like struct definitions 'struct foo {'
>> -		if ($line !~ /\bconst\b/ &&
>> +		if ($const_structs ne "" &&
> 
> instead testing
> 
> 		if (defined($const_structs) &&

Sure, I will respin.

Thank you for the review!
Quentin