[PATCH v3 0/9] scripts/checkpatch: fix SPDX-License-Identifier detection

[PATCH v3 0/9] scripts/checkpatch: fix SPDX-License-Identifier detection

[Daniel P. Berrangé]

This is hugely expanded an update of

  https://lists.nongnu.org/archive/html/qemu-devel/2025-05/msg02040.html

In that series, Peter suggested creating standalone methods to act
as hooks to call when detecting the start/end of a file in a diff.

This implements that idea and adapts a number of existing checks
to use the new hooks.

Changed in v3:

 - Add check for redundent license boilerplate text
 - Fix check for Makefile names

Daniel P. Berrangé (9):
  Revert "scripts: mandate that new files have SPDX-License-Identifier"
  scripts/checkpatch.pl: fix various indentation mistakes
  scripts/checkpatch: introduce tracking of file start/end
  scripts/checkpatch: use new hook for ACPI test data check
  scripts/checkpatch: use new hook for file permissions check
  scripts/checkpatch: expand pattern for matching makefiles
  scripts/checkpatch: use new hook for MAINTAINERS update check
  scripts/checkpatch: reimplement mandate for SPDX-License-Identifier
  scripts/checkpatch: reject license boilerplate on new files

 scripts/checkpatch.pl | 381 +++++++++++++++++++++++++++---------------
 1 file changed, 243 insertions(+), 138 deletions(-)

-- 
2.49.0

[PATCH v3 1/9] Revert "scripts: mandate that new files have SPDX-License-Identifier"

[Daniel P. Berrangé]

This reverts commit fa4d79c64dae03ffa269e42e21822453856618b7.

The logic in this commit was flawed in two critical ways

 * It always failed to report SPDX validation on the last newly
   added file. IOW, it only worked if at least 2 new files were
   added in a commit

 * If an existing file change, followed a new file change, in
   the commit and the existing file context/changed lines
   included SPDX-License-Identifier, it would incorrectly
   associate this with the previous newly added file.

Simply reverting this commit will make it significantly easier to
understand the improved logic in the following commit.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/checkpatch.pl | 30 ------------------------------
 1 file changed, 30 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 365892de04..d355c0dad5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1442,8 +1442,6 @@ sub process {
 	my $in_imported_file = 0;
 	my $in_no_imported_file = 0;
 	my $non_utf8_charset = 0;
-	my $expect_spdx = 0;
-	my $expect_spdx_file;
 
 	our @report = ();
 	our $cnt_lines = 0;
@@ -1681,34 +1679,6 @@ sub process {
 			WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
 		}
 
-# All new files should have a SPDX-License-Identifier tag
-		if ($line =~ /^new file mode\s*\d+\s*$/) {
-		    if ($expect_spdx) {
-			if ($expect_spdx_file =~
-			    /\.(c|h|py|pl|sh|json|inc|Makefile)$/) {
-			    # source code files MUST have SPDX license declared
-			    ERROR("New file '$expect_spdx_file' requires " .
-				  "'SPDX-License-Identifier'");
-			} else {
-			    # Other files MAY have SPDX license if appropriate
-			    WARN("Does new file '$expect_spdx_file' need " .
-				 "'SPDX-License-Identifier'?");
-			}
-		    }
-		    $expect_spdx = 1;
-		    $expect_spdx_file = undef;
-		} elsif ($expect_spdx) {
-		    $expect_spdx_file = $realfile unless
-			defined $expect_spdx_file;
-
-		    # SPDX tags may occurr in comments which were
-		    # stripped from '$line', so use '$rawline'
-		    if ($rawline =~ /SPDX-License-Identifier/) {
-			$expect_spdx = 0;
-			$expect_spdx_file = undef;
-		    }
-		}
-
 # Check SPDX-License-Identifier references a permitted license
 		if ($rawline =~ m,SPDX-License-Identifier: (.*?)(\*/)?\s*$,) {
 		    &checkspdx($realfile, $1);
-- 
2.49.0

Re: [PATCH v3 1/9] Revert "scripts: mandate that new files have SPDX-License-Identifier"

[Peter Maydell]

On Thu, 15 May 2025 at 14:59, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> This reverts commit fa4d79c64dae03ffa269e42e21822453856618b7.
>
> The logic in this commit was flawed in two critical ways
>
>  * It always failed to report SPDX validation on the last newly
>    added file. IOW, it only worked if at least 2 new files were
>    added in a commit
>
>  * If an existing file change, followed a new file change, in
>    the commit and the existing file context/changed lines
>    included SPDX-License-Identifier, it would incorrectly
>    associate this with the previous newly added file.
>
> Simply reverting this commit will make it significantly easier to
> understand the improved logic in the following commit.
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v3 2/9] scripts/checkpatch.pl: fix various indentation mistakes

[Daniel P. Berrangé]

Various checks in the code were under-indented relative to other
surrounding code.

Reviewed-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/checkpatch.pl | 98 +++++++++++++++++++++----------------------
 1 file changed, 49 insertions(+), 49 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d355c0dad5..7675418b0b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1681,19 +1681,19 @@ sub process {
 
 # Check SPDX-License-Identifier references a permitted license
 		if ($rawline =~ m,SPDX-License-Identifier: (.*?)(\*/)?\s*$,) {
-		    &checkspdx($realfile, $1);
+			&checkspdx($realfile, $1);
 		}
 
 		if ($rawline =~ m,(SPDX-[a-zA-Z0-9-_]+):,) {
-		    my $tag = $1;
-		    my @permitted = qw(
-			SPDX-License-Identifier
-		    );
-
-		    unless (grep { /^$tag$/ } @permitted) {
-			ERROR("Tag $tag not permitted in QEMU code, valid " .
-			      "choices are: " . join(", ", @permitted));
-		    }
+			my $tag = $1;
+			my @permitted = qw(
+				SPDX-License-Identifier
+			);
+
+			unless (grep { /^$tag$/ } @permitted) {
+				ERROR("Tag $tag not permitted in QEMU code, valid " .
+				      "choices are: " . join(", ", @permitted));
+			}
 		}
 
 # Check for wrappage within a valid hunk of the file
@@ -2274,7 +2274,7 @@ sub process {
 
 # missing space after union, struct or enum definition
 		if ($line =~ /^.\s*(?:typedef\s+)?(enum|union|struct)(?:\s+$Ident)?(?:\s+$Ident)?[=\{]/) {
-		    ERROR("missing space after $1 definition\n" . $herecurr);
+			ERROR("missing space after $1 definition\n" . $herecurr);
 		}
 
 # check for spacing round square brackets; allowed:
@@ -2569,7 +2569,7 @@ sub process {
 
 		if ($line =~ /^.\s*(Q(?:S?LIST|SIMPLEQ|TAILQ)_HEAD)\s*\(\s*[^,]/ &&
 		    $line !~ /^.typedef/) {
-		    ERROR("named $1 should be typedefed separately\n" . $herecurr);
+			ERROR("named $1 should be typedefed separately\n" . $herecurr);
 		}
 
 # Need a space before open parenthesis after if, while etc
@@ -3118,48 +3118,48 @@ sub process {
 
 # Qemu error function tests
 
-	# Find newlines in error messages
-	my $qemu_error_funcs = qr{error_setg|
-				error_setg_errno|
-				error_setg_win32|
-				error_setg_file_open|
-				error_set|
-				error_prepend|
-				warn_reportf_err|
-				error_reportf_err|
-				error_vreport|
-				warn_vreport|
-				info_vreport|
-				error_report|
-				warn_report|
-				info_report|
-				g_test_message}x;
-
-	if ($rawline =~ /\b(?:$qemu_error_funcs)\s*\(.*\".*\\n/) {
-		ERROR("Error messages should not contain newlines\n" . $herecurr);
-	}
+		# Find newlines in error messages
+		my $qemu_error_funcs = qr{error_setg|
+					 error_setg_errno|
+					 error_setg_win32|
+					 error_setg_file_open|
+					 error_set|
+					 error_prepend|
+					 warn_reportf_err|
+					 error_reportf_err|
+					 error_vreport|
+					 warn_vreport|
+					 info_vreport|
+					 error_report|
+					 warn_report|
+					 info_report|
+					 g_test_message}x;
+
+		if ($rawline =~ /\b(?:$qemu_error_funcs)\s*\(.*\".*\\n/) {
+			ERROR("Error messages should not contain newlines\n" . $herecurr);
+		}
 
-	# Continue checking for error messages that contains newlines. This
-	# check handles cases where string literals are spread over multiple lines.
-	# Example:
-	# error_report("Error msg line #1"
-	#              "Error msg line #2\n");
-	my $quoted_newline_regex = qr{\+\s*\".*\\n.*\"};
-	my $continued_str_literal = qr{\+\s*\".*\"};
+		# Continue checking for error messages that contains newlines. This
+		# check handles cases where string literals are spread over multiple lines.
+		# Example:
+		# error_report("Error msg line #1"
+		#              "Error msg line #2\n");
+		my $quoted_newline_regex = qr{\+\s*\".*\\n.*\"};
+		my $continued_str_literal = qr{\+\s*\".*\"};
 
-	if ($rawline =~ /$quoted_newline_regex/) {
-		# Backtrack to first line that does not contain only a quoted literal
-		# and assume that it is the start of the statement.
-		my $i = $linenr - 2;
+		if ($rawline =~ /$quoted_newline_regex/) {
+			# Backtrack to first line that does not contain only a quoted literal
+			# and assume that it is the start of the statement.
+			my $i = $linenr - 2;
 
-		while (($i >= 0) & $rawlines[$i] =~ /$continued_str_literal/) {
-			$i--;
-		}
+			while (($i >= 0) & $rawlines[$i] =~ /$continued_str_literal/) {
+				$i--;
+			}
 
-		if ($rawlines[$i] =~ /\b(?:$qemu_error_funcs)\s*\(/) {
-			ERROR("Error messages should not contain newlines\n" . $herecurr);
+			if ($rawlines[$i] =~ /\b(?:$qemu_error_funcs)\s*\(/) {
+				ERROR("Error messages should not contain newlines\n" . $herecurr);
+			}
 		}
-	}
 
 # check for non-portable libc calls that have portable alternatives in QEMU
 		if ($line =~ /\bffs\(/) {
-- 
2.49.0

Re: [PATCH v3 2/9] scripts/checkpatch.pl: fix various indentation mistakes

[Peter Maydell]

On Thu, 15 May 2025 at 14:59, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> Various checks in the code were under-indented relative to other
> surrounding code.

Isn't the problem here not that they're under-indented,
but that they don't follow the "indent with hard coded
tab characters" style that the rest of the script does?

Looking at the patch on patchew it looks like these changes do
make the code use hardcoded tabs, but I think it would be worth
mentioning that in the commit message. (I assumed from the wording
-- and also because my mail client was being misleading -- that
these changes were adding 8-space indent.)

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v3 2/9] scripts/checkpatch.pl: fix various indentation mistakes

[Daniel P. Berrangé]

On Mon, May 19, 2025 at 01:12:09PM +0100, Peter Maydell wrote:
> On Thu, 15 May 2025 at 14:59, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > Various checks in the code were under-indented relative to other
> > surrounding code.
> 
> Isn't the problem here not that they're under-indented,
> but that they don't follow the "indent with hard coded
> tab characters" style that the rest of the script does?
> 
> Looking at the patch on patchew it looks like these changes do
> make the code use hardcoded tabs, but I think it would be worth
> mentioning that in the commit message. (I assumed from the wording
> -- and also because my mail client was being misleading -- that
> these changes were adding 8-space indent.)

It was a mixture of problems. Some places where using 4 spaces instead
of 1 tab, other places used a mixture of tabs and 4 spaces and one place
used only tabs, but too few of them.

> 
> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> thanks
> -- PMM
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 2/9] scripts/checkpatch.pl: fix various indentation mistakes

[Alex Bennée]

Daniel P. Berrangé <berrange@redhat.com> writes:

> Various checks in the code were under-indented relative to other
> surrounding code.

Are we accepting that we have hard forked from the Linux checkpatch now?

<snip>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v3 2/9] scripts/checkpatch.pl: fix various indentation mistakes

[Daniel P. Berrangé]

On Mon, May 19, 2025 at 05:27:21PM +0100, Alex Bennée wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > Various checks in the code were under-indented relative to other
> > surrounding code.
> 
> Are we accepting that we have hard forked from the Linux checkpatch now?

The changed lines were all in QEMU specific checks, so don't make that
situation any worse than it already is.

AFAICT, we've got so many add-ons that we're a quite significant fork
already. It may be possible to merge back changes from Linux still,
but I wouldn't want to be the one figuring out that resolution :-)


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 2/9] scripts/checkpatch.pl: fix various indentation mistakes

[Peter Maydell]

On Mon, 19 May 2025 at 17:27, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > Various checks in the code were under-indented relative to other
> > surrounding code.
>
> Are we accepting that we have hard forked from the Linux checkpatch now?

Yes, I think that ship sailed some years ago: we've never
done more than try to port some specific kernel commits
across to our version; we've not tried to do a sync
to the kernel's version of the script ever since we
first imported the script in 2011.

thanks
-- PMM

[PATCH v3 3/9] scripts/checkpatch: introduce tracking of file start/end

[Daniel P. Berrangé]

Some checks want to be performed either at the start of a new file
within a patch, or at the end. This is complicated by the fact that
the information relevant to the check may be spread across multiple
lines. It is further complicated by a need to support both git and
non-git diffs, and special handling for renames where there might
not be any patch hunks.

To handle this more sanely, introduce explicit tracking of file
start/end, taking account of git metadata, and calling a hook
function at each transition.

Reviewed-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/checkpatch.pl | 109 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 106 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7675418b0b..b74391e63a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1417,6 +1417,38 @@ sub checkspdx {
     }
 }
 
+# All three of the methods below take a 'file info' record
+# which is a hash ref containing
+#
+#  'isgit': is this from an enhanced git diff or plain diff
+#  'linestart': line number of start of file diff
+#  'lineend': line number of end of file diff
+#  'filenew': the new filename
+#  'fileold': the old filename (same as 'new filename' except
+#             for renames in git diffs)
+#  'action': one of 'modified' (always) or 'new' or 'deleted' or
+#            'renamed' (git diffs only)
+#  'mode': file mode for new/deleted files (git diffs only)
+#  'similarity': file similarity when renamed (git diffs only)
+#  'facts': hash ref for storing any metadata related to checks
+#
+
+# Called at the end of each patch, with the list of
+# real filenames that were seen in the patch
+sub process_file_list {
+	my @fileinfos = @_;
+}
+
+# Called at the start of processing a diff hunk for a file
+sub process_start_of_file {
+	my $fileinfo = shift;
+}
+
+# Called at the end of processing a diff hunk for a file
+sub process_end_of_file {
+	my $fileinfo = shift;
+}
+
 sub process {
 	my $filename = shift;
 
@@ -1453,7 +1485,10 @@ sub process {
 	my $realfile = '';
 	my $realline = 0;
 	my $realcnt = 0;
+	my $fileinfo;
+	my @fileinfolist;
 	my $here = '';
+	my $oldhere = '';
 	my $in_comment = 0;
 	my $comment_edge = 0;
 	my $first_line = 0;
@@ -1591,17 +1626,56 @@ sub process {
 		$prefix = "$filename:$realline: " if ($emacs && $file);
 		$prefix = "$filename:$linenr: " if ($emacs && !$file);
 
+		$oldhere = $here;
 		$here = "#$linenr: " if (!$file);
 		$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+)\s+(\S+)$/) {
+			my $fileold = $1;
+			my $filenew = $2;
+
+			if (defined $fileinfo) {
+				$fileinfo->{lineend} = $oldhere;
+				process_end_of_file($fileinfo)
+		        }
+			$fileold =~ s@^([^/]*)/@@ if (!$file);
+			$filenew =~ s@^([^/]*)/@@ if (!$file);
+			$realfile = $filenew;
 	                checkfilename($realfile, \$acpi_testexpected, \$acpi_nontestexpected);
+
+			$fileinfo = {
+				"isgit" => 1,
+				"githeader" => 1,
+				"linestart" => $here,
+				"lineend" => 0,
+				"fileold" => $fileold,
+				"filenew" => $filenew,
+				"action" => "modified",
+				"mode" => 0,
+				"similarity" => 0,
+				"facts" => {},
+		        };
+			push @fileinfolist, $fileinfo;
+		} elsif (defined $fileinfo && $fileinfo->{githeader} &&
+			 $line =~ /^(new|deleted) (?:file )?mode\s+([0-7]+)$/) {
+			$fileinfo->{action} = $1;
+			$fileinfo->{mode} = oct($2);
+		} elsif (defined $fileinfo && $fileinfo->{githeader} &&
+			 $line =~ /^similarity index (\d+)%/) {
+			$fileinfo->{similarity} = int($1);
+		} elsif (defined $fileinfo && $fileinfo->{githeader} &&
+			 $line =~ /^rename (from|to) [\w\/\.\-]+\s*$/) {
+			$fileinfo->{action} = "renamed";
+			# For a no-change rename, we'll never have any "+++..."
+			# lines, so trigger actions now
+			if ($1 eq "to" && $fileinfo->{similarity} == 100)  {
+				process_start_of_file($fileinfo);
+			}
 		} elsif ($line =~ /^\+\+\+\s+(\S+)/) {
 			$realfile = $1;
 			$realfile =~ s@^([^/]*)/@@ if (!$file);
+
 	                checkfilename($realfile, \$acpi_testexpected, \$acpi_nontestexpected);
 
 			$p1_prefix = $1;
@@ -1610,6 +1684,30 @@ sub process {
 				WARN("patch prefix '$p1_prefix' exists, appears to be a -p0 patch\n");
 			}
 
+			if (defined $fileinfo && !$fileinfo->{isgit}) {
+				$fileinfo->{lineend} = $oldhere;
+				process_end_of_file($fileinfo);
+			}
+
+			if (!defined $fileinfo || !$fileinfo->{isgit}) {
+				$fileinfo = {
+					"isgit" => 0,
+					"githeader" => 0,
+					"linestart" => $here,
+					"lineend" => 0,
+					"fileold" => $realfile,
+					"filenew" => $realfile,
+					"action" => "modified",
+					"mode" => 0,
+					"similarity" => 0,
+					"facts" => {},
+			        };
+				push @fileinfolist, $fileinfo;
+			} else {
+				$fileinfo->{githeader} = 0;
+			}
+			process_start_of_file($fileinfo);
+
 			next;
 		}
 
@@ -3213,6 +3311,11 @@ sub process {
 		}
 	}
 
+	if (defined $fileinfo) {
+		process_end_of_file($fileinfo);
+	}
+	process_file_list(@fileinfolist);
+
 	if ($is_patch && $chk_signoff && $signoff == 0) {
 		ERROR("Missing Signed-off-by: line(s)\n");
 	}
-- 
2.49.0

Re: [PATCH v3 3/9] scripts/checkpatch: introduce tracking of file start/end

[Peter Maydell]

On Thu, 15 May 2025 at 14:59, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> Some checks want to be performed either at the start of a new file
> within a patch, or at the end. This is complicated by the fact that
> the information relevant to the check may be spread across multiple
> lines. It is further complicated by a need to support both git and
> non-git diffs, and special handling for renames where there might
> not be any patch hunks.
>
> To handle this more sanely, introduce explicit tracking of file
> start/end, taking account of git metadata, and calling a hook
> function at each transition.
>
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  scripts/checkpatch.pl | 109 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 106 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 7675418b0b..b74391e63a 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1417,6 +1417,38 @@ sub checkspdx {
>      }
>  }
>
> +# All three of the methods below take a 'file info' record
> +# which is a hash ref containing
> +#
> +#  'isgit': is this from an enhanced git diff or plain diff
> +#  'linestart': line number of start of file diff
> +#  'lineend': line number of end of file diff
> +#  'filenew': the new filename
> +#  'fileold': the old filename (same as 'new filename' except
> +#             for renames in git diffs)
> +#  'action': one of 'modified' (always) or 'new' or 'deleted' or
> +#            'renamed' (git diffs only)
> +#  'mode': file mode for new/deleted files (git diffs only)
> +#  'similarity': file similarity when renamed (git diffs only)
> +#  'facts': hash ref for storing any metadata related to checks


> +                       $fileinfo = {
> +                               "isgit" => 1,
> +                               "githeader" => 1,
> +                               "linestart" => $here,
> +                               "lineend" => 0,
> +                               "fileold" => $fileold,
> +                               "filenew" => $filenew,
> +                               "action" => "modified",
> +                               "mode" => 0,
> +                               "similarity" => 0,
> +                               "facts" => {},
> +                       };

The 'githeader' field here should be documented in the
comment block above.

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v3 4/9] scripts/checkpatch: use new hook for ACPI test data check

[Daniel P. Berrangé]

The ACPI test data check needs to analyse a list of all files in a
commit, so can use the new hook for processing the file list.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/checkpatch.pl | 61 ++++++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 32 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index b74391e63a..6a7b543ddf 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1330,29 +1330,6 @@ sub WARN {
 	}
 }
 
-# According to tests/qtest/bios-tables-test.c: do not
-# change expected file in the same commit with adding test
-sub checkfilename {
-	my ($name, $acpi_testexpected, $acpi_nontestexpected) = @_;
-
-        # Note: shell script that rebuilds the expected files is in the same
-        # directory as files themselves.
-        # Note: allowed diff list can be changed both when changing expected
-        # files and when changing tests.
-	if ($name =~ m#^tests/data/acpi/# and not $name =~ m#^\.sh$#) {
-		$$acpi_testexpected = $name;
-	} elsif ($name !~ m#^tests/qtest/bios-tables-test-allowed-diff.h$#) {
-		$$acpi_nontestexpected = $name;
-	}
-	if (defined $$acpi_testexpected and defined $$acpi_nontestexpected) {
-		ERROR("Do not add expected files together with tests, " .
-		      "follow instructions in " .
-		      "tests/qtest/bios-tables-test.c: both " .
-		      $$acpi_testexpected . " and " .
-		      $$acpi_nontestexpected . " found\n");
-	}
-}
-
 sub checkspdx {
     my ($file, $expr) = @_;
 
@@ -1437,6 +1414,34 @@ sub checkspdx {
 # real filenames that were seen in the patch
 sub process_file_list {
 	my @fileinfos = @_;
+
+	# According to tests/qtest/bios-tables-test.c: do not
+	# change expected file in the same commit with adding test
+	my @acpi_testexpected;
+	my @acpi_nontestexpected;
+
+	foreach my $fileinfo (@fileinfos) {
+		# Note: shell script that rebuilds the expected files is in
+		# the same directory as files themselves.
+		# Note: allowed diff list can be changed both when changing
+		#  expected files and when changing tests.
+		if ($fileinfo->{filenew} =~ m#^tests/data/acpi/# &&
+		    $fileinfo->{filenew} !~ m#^\.sh$#) {
+			push @acpi_testexpected, $fileinfo->{filenew};
+		} elsif ($fileinfo->{filenew} !~
+			 m#^tests/qtest/bios-tables-test-allowed-diff.h$#) {
+			push @acpi_nontestexpected, $fileinfo->{filenew};
+		}
+	}
+	if (int(@acpi_testexpected) > 0 and int(@acpi_nontestexpected) > 0) {
+		ERROR("Do not add expected files together with tests, " .
+		      "follow instructions in " .
+		      "tests/qtest/bios-tables-test.c. Files\n\n  " .
+		      join("\n  ", @acpi_testexpected) .
+		      "\n\nand\n\n  " .
+		      join("\n  ", @acpi_nontestexpected) .
+		      "\n\nfound in the same patch\n");
+	}
 }
 
 # Called at the start of processing a diff hunk for a file
@@ -1501,9 +1506,6 @@ sub process {
 	my %suppress_whiletrailers;
 	my %suppress_export;
 
-        my $acpi_testexpected;
-        my $acpi_nontestexpected;
-
 	# Pre-scan the patch sanitizing the lines.
 
 	sanitise_line_reset();
@@ -1642,7 +1644,6 @@ sub process {
 			$fileold =~ s@^([^/]*)/@@ if (!$file);
 			$filenew =~ s@^([^/]*)/@@ if (!$file);
 			$realfile = $filenew;
-	                checkfilename($realfile, \$acpi_testexpected, \$acpi_nontestexpected);
 
 			$fileinfo = {
 				"isgit" => 1,
@@ -1676,8 +1677,6 @@ sub process {
 			$realfile = $1;
 			$realfile =~ s@^([^/]*)/@@ if (!$file);
 
-	                checkfilename($realfile, \$acpi_testexpected, \$acpi_nontestexpected);
-
 			$p1_prefix = $1;
 			if (!$file && $tree && $p1_prefix ne '' &&
 			    -e "$root/$p1_prefix") {
@@ -1770,9 +1769,7 @@ sub process {
 		     $line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ ||
 		     ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ &&
 		      (defined($1) || defined($2)))) &&
-                      !(($realfile ne '') &&
-                        defined($acpi_testexpected) &&
-                        ($realfile eq $acpi_testexpected))) {
+		    $realfile !~ m#^tests/data/acpi/#) {
 			$reported_maintainer_file = 1;
 			WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
 		}
-- 
2.49.0

Re: [PATCH v3 4/9] scripts/checkpatch: use new hook for ACPI test data check

[Cédric Le Goater]

On 5/15/25 15:59, Daniel P. Berrangé wrote:
> The ACPI test data check needs to analyse a list of all files in a
> commit, so can use the new hook for processing the file list.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>> ---
>   scripts/checkpatch.pl | 61 ++++++++++++++++++++-----------------------
>   1 file changed, 29 insertions(+), 32 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index b74391e63a..6a7b543ddf 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1330,29 +1330,6 @@ sub WARN {
>   	}
>   }
>   
> -# According to tests/qtest/bios-tables-test.c: do not
> -# change expected file in the same commit with adding test
> -sub checkfilename {
> -	my ($name, $acpi_testexpected, $acpi_nontestexpected) = @_;
> -
> -        # Note: shell script that rebuilds the expected files is in the same
> -        # directory as files themselves.
> -        # Note: allowed diff list can be changed both when changing expected
> -        # files and when changing tests.
> -	if ($name =~ m#^tests/data/acpi/# and not $name =~ m#^\.sh$#) {
> -		$$acpi_testexpected = $name;
> -	} elsif ($name !~ m#^tests/qtest/bios-tables-test-allowed-diff.h$#) {
> -		$$acpi_nontestexpected = $name;
> -	}
> -	if (defined $$acpi_testexpected and defined $$acpi_nontestexpected) {
> -		ERROR("Do not add expected files together with tests, " .
> -		      "follow instructions in " .
> -		      "tests/qtest/bios-tables-test.c: both " .
> -		      $$acpi_testexpected . " and " .
> -		      $$acpi_nontestexpected . " found\n");
> -	}
> -}
> -
>   sub checkspdx {
>       my ($file, $expr) = @_;
>   
> @@ -1437,6 +1414,34 @@ sub checkspdx {
>   # real filenames that were seen in the patch
>   sub process_file_list {
>   	my @fileinfos = @_;
> +
> +	# According to tests/qtest/bios-tables-test.c: do not
> +	# change expected file in the same commit with adding test
> +	my @acpi_testexpected;
> +	my @acpi_nontestexpected;
> +
> +	foreach my $fileinfo (@fileinfos) {
> +		# Note: shell script that rebuilds the expected files is in
> +		# the same directory as files themselves.
> +		# Note: allowed diff list can be changed both when changing
> +		#  expected files and when changing tests.
> +		if ($fileinfo->{filenew} =~ m#^tests/data/acpi/# &&
> +		    $fileinfo->{filenew} !~ m#^\.sh$#) {
> +			push @acpi_testexpected, $fileinfo->{filenew};
> +		} elsif ($fileinfo->{filenew} !~
> +			 m#^tests/qtest/bios-tables-test-allowed-diff.h$#) {
> +			push @acpi_nontestexpected, $fileinfo->{filenew};
> +		}
> +	}
> +	if (int(@acpi_testexpected) > 0 and int(@acpi_nontestexpected) > 0) {
> +		ERROR("Do not add expected files together with tests, " .
> +		      "follow instructions in " .
> +		      "tests/qtest/bios-tables-test.c. Files\n\n  " .
> +		      join("\n  ", @acpi_testexpected) .
> +		      "\n\nand\n\n  " .
> +		      join("\n  ", @acpi_nontestexpected) .
> +		      "\n\nfound in the same patch\n");
> +	}
>   }
>   
>   # Called at the start of processing a diff hunk for a file
> @@ -1501,9 +1506,6 @@ sub process {
>   	my %suppress_whiletrailers;
>   	my %suppress_export;
>   
> -        my $acpi_testexpected;
> -        my $acpi_nontestexpected;
> -
>   	# Pre-scan the patch sanitizing the lines.
>   
>   	sanitise_line_reset();
> @@ -1642,7 +1644,6 @@ sub process {
>   			$fileold =~ s@^([^/]*)/@@ if (!$file);
>   			$filenew =~ s@^([^/]*)/@@ if (!$file);
>   			$realfile = $filenew;
> -	                checkfilename($realfile, \$acpi_testexpected, \$acpi_nontestexpected);
>   
>   			$fileinfo = {
>   				"isgit" => 1,
> @@ -1676,8 +1677,6 @@ sub process {
>   			$realfile = $1;
>   			$realfile =~ s@^([^/]*)/@@ if (!$file);
>   
> -	                checkfilename($realfile, \$acpi_testexpected, \$acpi_nontestexpected);
> -
>   			$p1_prefix = $1;
>   			if (!$file && $tree && $p1_prefix ne '' &&
>   			    -e "$root/$p1_prefix") {
> @@ -1770,9 +1769,7 @@ sub process {
>   		     $line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ ||
>   		     ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ &&
>   		      (defined($1) || defined($2)))) &&
> -                      !(($realfile ne '') &&
> -                        defined($acpi_testexpected) &&
> -                        ($realfile eq $acpi_testexpected))) {
> +		    $realfile !~ m#^tests/data/acpi/#) {
>   			$reported_maintainer_file = 1;
>   			WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
>   		}


* https://lore.kernel.org/qemu-devel/20250403151829.44858-12-philmd@linaro.org/
   WARNING: Does new file 'tests/data/acpi/aarch64/virt/APIC.its_off' need 'SPDX-License-Identifier'?
   WARNING: Does new file 'tests/data/acpi/aarch64/virt/FACP.its_off' need 'SPDX-License-Identifier'?
   WARNING: Does new file 'tests/data/acpi/aarch64/virt/IORT.its_off' need 'SPDX-License-Identifier'?
   ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c. Files
   
     tests/data/acpi/aarch64/virt/APIC.its_off
     tests/data/acpi/aarch64/virt/FACP.its_off
     tests/data/acpi/aarch64/virt/IORT.its_off
   
   and
   
     tests/qtest/bios-tables-test.c
   
   found in the same patch
   
   total: 1 errors, 3 warnings, 11 lines checked
   

Tested-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

Re: [PATCH v3 4/9] scripts/checkpatch: use new hook for ACPI test data check

[Peter Maydell]

On Thu, 15 May 2025 at 14:59, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The ACPI test data check needs to analyse a list of all files in a
> commit, so can use the new hook for processing the file list.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  scripts/checkpatch.pl | 61 ++++++++++++++++++++-----------------------
>  1 file changed, 29 insertions(+), 32 deletions(-)



> @@ -1770,9 +1769,7 @@ sub process {
>                      $line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ ||
>                      ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ &&
>                       (defined($1) || defined($2)))) &&
> -                      !(($realfile ne '') &&
> -                        defined($acpi_testexpected) &&
> -                        ($realfile eq $acpi_testexpected))) {
> +                   $realfile !~ m#^tests/data/acpi/#) {

Is the indentation off on this line?

>                         $reported_maintainer_file = 1;
>                         WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
>                 }
> --

Otherwise

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v3 4/9] scripts/checkpatch: use new hook for ACPI test data check

[Daniel P. Berrangé]

On Mon, May 19, 2025 at 01:29:14PM +0100, Peter Maydell wrote:
> On Thu, 15 May 2025 at 14:59, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > The ACPI test data check needs to analyse a list of all files in a
> > commit, so can use the new hook for processing the file list.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  scripts/checkpatch.pl | 61 ++++++++++++++++++++-----------------------
> >  1 file changed, 29 insertions(+), 32 deletions(-)
> 
> 
> 
> > @@ -1770,9 +1769,7 @@ sub process {
> >                      $line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ ||
> >                      ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ &&
> >                       (defined($1) || defined($2)))) &&
> > -                      !(($realfile ne '') &&
> > -                        defined($acpi_testexpected) &&
> > -                        ($realfile eq $acpi_testexpected))) {
> > +                   $realfile !~ m#^tests/data/acpi/#) {
> 
> Is the indentation off on this line?

It looks like it from this diff, but it is actually correct, as it was
moved outside the inner two sets of brackets.

> 
> >                         $reported_maintainer_file = 1;
> >                         WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
> >                 }
> > --
> 
> Otherwise
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> thanks
> -- PMM
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v3 5/9] scripts/checkpatch: use new hook for file permissions check

[Daniel P. Berrangé]

The file permissions check is the kind of check intended to be performed
in the new start of file hook.

Reviewed-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/checkpatch.pl | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 6a7b543ddf..4a18daa384 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1447,6 +1447,17 @@ sub process_file_list {
 # Called at the start of processing a diff hunk for a file
 sub process_start_of_file {
 	my $fileinfo = shift;
+
+	# Check for incorrect file permissions
+	if ($fileinfo->{action} eq "new" && ($fileinfo->{mode} & 0111)) {
+		my $permhere = $fileinfo->{linestart} . "FILE: " .
+			$fileinfo->{filenew} . "\n";
+		if ($fileinfo->{filenew} =~
+		    /(\bMakefile(?:\.objs)?|\.(c|cc|cpp|h|mak|s|S))$/) {
+			ERROR("do not set execute permissions for source " .
+			      "files\n" . $permhere);
+		}
+	}
 }
 
 # Called at the end of processing a diff hunk for a file
@@ -1718,14 +1729,6 @@ sub process {
 
 		$cnt_lines++ if ($realcnt != 0);
 
-# Check for incorrect file permissions
-		if ($line =~ /^new (file )?mode.*[7531]\d{0,2}$/) {
-			my $permhere = $here . "FILE: $realfile\n";
-			if ($realfile =~ /(\bMakefile(?:\.objs)?|\.c|\.cc|\.cpp|\.h|\.mak|\.[sS])$/) {
-				ERROR("do not set execute permissions for source files\n" . $permhere);
-			}
-		}
-
 # Only allow Python 3 interpreter
 		if ($realline == 1 &&
 			$line =~ /^\+#!\ *\/usr\/bin\/(?:env )?python$/) {
-- 
2.49.0

Re: [PATCH v3 5/9] scripts/checkpatch: use new hook for file permissions check

[Peter Maydell]

On Thu, 15 May 2025 at 14:59, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The file permissions check is the kind of check intended to be performed
> in the new start of file hook.
>
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  scripts/checkpatch.pl | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v3 6/9] scripts/checkpatch: expand pattern for matching makefiles

[Daniel P. Berrangé]

The current regex matches Makefile & Makefile.objs, but the latter is
no longer used, and we're missing coverage of Makefile.include and
Makefile.target. Expand the pattern to match any suffix.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4a18daa384..00d7d72e53 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1453,7 +1453,7 @@ sub process_start_of_file {
 		my $permhere = $fileinfo->{linestart} . "FILE: " .
 			$fileinfo->{filenew} . "\n";
 		if ($fileinfo->{filenew} =~
-		    /(\bMakefile(?:\.objs)?|\.(c|cc|cpp|h|mak|s|S))$/) {
+		    /(\bMakefile.*|\.(c|cc|cpp|h|mak|s|S))$/) {
 			ERROR("do not set execute permissions for source " .
 			      "files\n" . $permhere);
 		}
-- 
2.49.0

Re: [PATCH v3 6/9] scripts/checkpatch: expand pattern for matching makefiles

[Cédric Le Goater]

On 5/15/25 15:59, Daniel P. Berrangé wrote:
> The current regex matches Makefile & Makefile.objs, but the latter is
> no longer used, and we're missing coverage of Makefile.include and
> Makefile.target. Expand the pattern to match any suffix.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   scripts/checkpatch.pl | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 4a18daa384..00d7d72e53 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1453,7 +1453,7 @@ sub process_start_of_file {
>   		my $permhere = $fileinfo->{linestart} . "FILE: " .
>   			$fileinfo->{filenew} . "\n";
>   		if ($fileinfo->{filenew} =~
> -		    /(\bMakefile(?:\.objs)?|\.(c|cc|cpp|h|mak|s|S))$/) {
> +		    /(\bMakefile.*|\.(c|cc|cpp|h|mak|s|S))$/) {
>   			ERROR("do not set execute permissions for source " .
>   			      "files\n" . $permhere);
>   		}



Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

Re: [PATCH v3 6/9] scripts/checkpatch: expand pattern for matching makefiles

[Peter Maydell]

On Thu, 15 May 2025 at 15:00, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The current regex matches Makefile & Makefile.objs, but the latter is
> no longer used, and we're missing coverage of Makefile.include and
> Makefile.target. Expand the pattern to match any suffix.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  scripts/checkpatch.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v3 7/9] scripts/checkpatch: use new hook for MAINTAINERS update check

[Daniel P. Berrangé]

When seeing a new/deleted/renamed file we check to see if MAINTAINERS
is updated, but we don't give the user a list of files affected, as
we don't want to repeat the same warning many times over.

Using the new file list hook, we can give a single warning at the
end with a list of filenames included.

Reviewed-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/checkpatch.pl | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 00d7d72e53..6adef12871 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1442,6 +1442,25 @@ sub process_file_list {
 		      join("\n  ", @acpi_nontestexpected) .
 		      "\n\nfound in the same patch\n");
 	}
+
+	my $sawmaintainers = 0;
+	my @maybemaintainers;
+	foreach my $fileinfo (@fileinfos) {
+		if ($fileinfo->{action} ne "modified" &&
+		    $fileinfo->{filenew} !~ m#^tests/data/acpi/#) {
+			push @maybemaintainers, $fileinfo->{filenew};
+		}
+		if ($fileinfo->{filenew} eq "MAINTAINERS") {
+			$sawmaintainers = 1;
+		}
+	}
+
+	# If we don't see a MAINTAINERS update, prod the user to check
+	if (int(@maybemaintainers) > 0 && !$sawmaintainers) {
+		WARN("added, moved or deleted file(s):\n\n  " .
+		     join("\n  ", @maybemaintainers) .
+		     "\n\nDoes MAINTAINERS need updating?\n");
+	}
 }
 
 # Called at the start of processing a diff hunk for a file
@@ -1485,7 +1504,6 @@ 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_mixing_imported_file = 0;
 	my $in_imported_file = 0;
 	my $in_no_imported_file = 0;
@@ -1760,23 +1778,6 @@ sub process {
 			}
 		}
 
-# Check if MAINTAINERS is being updated.  If so, there's probably no need to
-# emit the "does MAINTAINERS need updating?" message on file add/move/delete
-		if ($line =~ /^\s*MAINTAINERS\s*\|/) {
-			$reported_maintainer_file = 1;
-		}
-
-# Check for added, moved or deleted files
-		if (!$reported_maintainer_file && !$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)))) &&
-		    $realfile !~ m#^tests/data/acpi/#) {
-			$reported_maintainer_file = 1;
-			WARN("added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
-		}
-
 # Check SPDX-License-Identifier references a permitted license
 		if ($rawline =~ m,SPDX-License-Identifier: (.*?)(\*/)?\s*$,) {
 			&checkspdx($realfile, $1);
-- 
2.49.0

Re: [PATCH v3 7/9] scripts/checkpatch: use new hook for MAINTAINERS update check

[Peter Maydell]

On Thu, 15 May 2025 at 15:00, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> When seeing a new/deleted/renamed file we check to see if MAINTAINERS
> is updated, but we don't give the user a list of files affected, as
> we don't want to repeat the same warning many times over.
>
> Using the new file list hook, we can give a single warning at the
> end with a list of filenames included.
>
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  scripts/checkpatch.pl | 37 +++++++++++++++++++------------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v3 8/9] scripts/checkpatch: reimplement mandate for SPDX-License-Identifier

[Daniel P. Berrangé]

Going forward we want all newly created source files to have an
SPDX-License-Identifier tag present.

Initially mandate this for C, Python, Perl, Shell source files,
as well as JSON (QAPI) and Makefiles, while encouraging users
to consider it for other file types.

The new attempt at detecting missing SPDX-License-Identifier relies
on the hooks for relying triggering logic at the end of scanning a
new file in the diff.

Tested-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/checkpatch.pl | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 6adef12871..87050e6677 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1482,6 +1482,20 @@ sub process_start_of_file {
 # Called at the end of processing a diff hunk for a file
 sub process_end_of_file {
 	my $fileinfo = shift;
+
+	if ($fileinfo->{action} eq "new" &&
+	    !exists $fileinfo->{facts}->{sawspdx}) {
+		if ($fileinfo->{filenew} =~
+		    /(\.(c|h|py|pl|sh|json|inc)|Makefile.*)$/) {
+			# source code files MUST have SPDX license declared
+			ERROR("New file '" . $fileinfo->{filenew} .
+			      "' requires 'SPDX-License-Identifier'");
+		} else {
+			# Other files MAY have SPDX license if appropriate
+			WARN("Does new file '" . $fileinfo->{filenew} .
+			     "' need 'SPDX-License-Identifier'?");
+		}
+	}
 }
 
 sub process {
@@ -1780,6 +1794,7 @@ sub process {
 
 # Check SPDX-License-Identifier references a permitted license
 		if ($rawline =~ m,SPDX-License-Identifier: (.*?)(\*/)?\s*$,) {
+			$fileinfo->{facts}->{sawspdx} = 1;
 			&checkspdx($realfile, $1);
 		}
 
-- 
2.49.0

Re: [PATCH v3 8/9] scripts/checkpatch: reimplement mandate for SPDX-License-Identifier

[Peter Maydell]

On Thu, 15 May 2025 at 15:00, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> Going forward we want all newly created source files to have an
> SPDX-License-Identifier tag present.
>
> Initially mandate this for C, Python, Perl, Shell source files,
> as well as JSON (QAPI) and Makefiles, while encouraging users
> to consider it for other file types.
>
> The new attempt at detecting missing SPDX-License-Identifier relies
> on the hooks for relying triggering logic at the end of scanning a
> new file in the diff.
>
> Tested-by: Cédric Le Goater <clg@redhat.com>
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v3 9/9] scripts/checkpatch: reject license boilerplate on new files

[Daniel P. Berrangé]

The previous commit mandates use of SPDX-License-Identifier on common
source files, and encourages it on all other files.

Some contributors are none the less still also including the license
boilerplate text. This is redundant and will potentially cause
trouble if inconsistent with the SPDX declaration.

Match common boilerplate text blurbs and report them as invalid,
for newly added files.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/checkpatch.pl | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 87050e6677..cb1942c021 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1496,6 +1496,13 @@ sub process_end_of_file {
 			     "' need 'SPDX-License-Identifier'?");
 		}
 	}
+	if ($fileinfo->{action} eq "new" &&
+	    !exists $fileinfo->{facts}->{sawboilerplate}) {
+		ERROR("New file '" . $fileinfo->{filenew} . "' must " .
+		      "not have license boilerplate header text unless " .
+		      "this file is copied from existing code with such " .
+		      "text already present.");
+	}
 }
 
 sub process {
@@ -1798,6 +1805,15 @@ sub process {
 			&checkspdx($realfile, $1);
 		}
 
+		if ($rawline =~ /licensed under the terms of the GNU GPL/ ||
+		    $rawline =~ /under the terms of the GNU General Public License/ ||
+		    $rawline =~ /under the terms of the GNU Lesser General Public/ ||
+		    $rawline =~ /Permission is hereby granted, free of charge/ ||
+		    $rawline =~ /GNU GPL, version 2 or later/ ||
+		    $rawline =~ /See the COPYING file/) {
+			$fileinfo->{facts}->{sawboilerplate} = 1;
+		}
+
 		if ($rawline =~ m,(SPDX-[a-zA-Z0-9-_]+):,) {
 			my $tag = $1;
 			my @permitted = qw(
-- 
2.49.0

Re: [PATCH v3 9/9] scripts/checkpatch: reject license boilerplate on new files

[Cédric Le Goater]

On 5/15/25 15:59, Daniel P. Berrangé wrote:
> The previous commit mandates use of SPDX-License-Identifier on common
> source files, and encourages it on all other files.
> 
> Some contributors are none the less still also including the license
> boilerplate text. This is redundant and will potentially cause
> trouble if inconsistent with the SPDX declaration.
> 
> Match common boilerplate text blurbs and report them as invalid,
> for newly added files.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   scripts/checkpatch.pl | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 87050e6677..cb1942c021 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1496,6 +1496,13 @@ sub process_end_of_file {
>   			     "' need 'SPDX-License-Identifier'?");
>   		}
>   	}
> +	if ($fileinfo->{action} eq "new" &&
> +	    !exists $fileinfo->{facts}->{sawboilerplate}) {
> +		ERROR("New file '" . $fileinfo->{filenew} . "' must " .
> +		      "not have license boilerplate header text unless " .
> +		      "this file is copied from existing code with such " .
> +		      "text already present.");
> +	}
>   }
>   
>   sub process {
> @@ -1798,6 +1805,15 @@ sub process {
>   			&checkspdx($realfile, $1);
>   		}
>   
> +		if ($rawline =~ /licensed under the terms of the GNU GPL/ ||
> +		    $rawline =~ /under the terms of the GNU General Public License/ ||
> +		    $rawline =~ /under the terms of the GNU Lesser General Public/ ||
> +		    $rawline =~ /Permission is hereby granted, free of charge/ ||
> +		    $rawline =~ /GNU GPL, version 2 or later/ ||
> +		    $rawline =~ /See the COPYING file/) {
> +			$fileinfo->{facts}->{sawboilerplate} = 1;
> +		}
> +
>   		if ($rawline =~ m,(SPDX-[a-zA-Z0-9-_]+):,) {
>   			my $tag = $1;
>   			my @permitted = qw(

This patch reported this error :

     ERROR: New file 'include/hw/vfio/vfio-cpr.h' must not have license boilerplate header text unless this file is copied from existing code with such text already present.
     total: 1 errors, 0 warnings, 58 lines checked

See patch https://lore.kernel.org/qemu-devel/1747063973-124548-6-git-send-email-steven.sistare@oracle.com/

But this looks wrong. Right ? I don't understand how rawline matched though.


Thanks,

C.

Re: [PATCH v3 9/9] scripts/checkpatch: reject license boilerplate on new files

[Daniel P. Berrangé]

On Thu, May 15, 2025 at 02:59:36PM +0100, Daniel P. Berrangé wrote:
> The previous commit mandates use of SPDX-License-Identifier on common
> source files, and encourages it on all other files.
> 
> Some contributors are none the less still also including the license
> boilerplate text. This is redundant and will potentially cause
> trouble if inconsistent with the SPDX declaration.
> 
> Match common boilerplate text blurbs and report them as invalid,
> for newly added files.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  scripts/checkpatch.pl | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 87050e6677..cb1942c021 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1496,6 +1496,13 @@ sub process_end_of_file {
>  			     "' need 'SPDX-License-Identifier'?");
>  		}
>  	}
> +	if ($fileinfo->{action} eq "new" &&
> +	    !exists $fileinfo->{facts}->{sawboilerplate}) {

/face-palm  - I forgot to remove the '!' here so the check is
inverted and my test patch had two very similarly named files
so didn't notice it :-(

> +		ERROR("New file '" . $fileinfo->{filenew} . "' must " .
> +		      "not have license boilerplate header text unless " .
> +		      "this file is copied from existing code with such " .
> +		      "text already present.");
> +	}
>  }
>  
>  sub process {
> @@ -1798,6 +1805,15 @@ sub process {
>  			&checkspdx($realfile, $1);
>  		}
>  
> +		if ($rawline =~ /licensed under the terms of the GNU GPL/ ||
> +		    $rawline =~ /under the terms of the GNU General Public License/ ||
> +		    $rawline =~ /under the terms of the GNU Lesser General Public/ ||
> +		    $rawline =~ /Permission is hereby granted, free of charge/ ||
> +		    $rawline =~ /GNU GPL, version 2 or later/ ||
> +		    $rawline =~ /See the COPYING file/) {
> +			$fileinfo->{facts}->{sawboilerplate} = 1;
> +		}
> +
>  		if ($rawline =~ m,(SPDX-[a-zA-Z0-9-_]+):,) {
>  			my $tag = $1;
>  			my @permitted = qw(
> -- 
> 2.49.0
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 9/9] scripts/checkpatch: reject license boilerplate on new files

[Cédric Le Goater]

On 5/15/25 18:05, Daniel P. Berrangé wrote:
> On Thu, May 15, 2025 at 02:59:36PM +0100, Daniel P. Berrangé wrote:
>> The previous commit mandates use of SPDX-License-Identifier on common
>> source files, and encourages it on all other files.
>>
>> Some contributors are none the less still also including the license
>> boilerplate text. This is redundant and will potentially cause
>> trouble if inconsistent with the SPDX declaration.
>>
>> Match common boilerplate text blurbs and report them as invalid,
>> for newly added files.
>>
>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> ---
>>   scripts/checkpatch.pl | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index 87050e6677..cb1942c021 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -1496,6 +1496,13 @@ sub process_end_of_file {
>>   			     "' need 'SPDX-License-Identifier'?");
>>   		}
>>   	}
>> +	if ($fileinfo->{action} eq "new" &&
>> +	    !exists $fileinfo->{facts}->{sawboilerplate}) {
> 
> /face-palm  - I forgot to remove the '!' here so the check is
> inverted and my test patch had two very similarly named files
> so didn't notice it :-(

Oh. I didn't see. No need to resend. I will fix in my tree.


Thanks,

C.




> 
>> +		ERROR("New file '" . $fileinfo->{filenew} . "' must " .
>> +		      "not have license boilerplate header text unless " .
>> +		      "this file is copied from existing code with such " .
>> +		      "text already present.");
>> +	}
>>   }
>>   
>>   sub process {
>> @@ -1798,6 +1805,15 @@ sub process {
>>   			&checkspdx($realfile, $1);
>>   		}
>>   
>> +		if ($rawline =~ /licensed under the terms of the GNU GPL/ ||
>> +		    $rawline =~ /under the terms of the GNU General Public License/ ||
>> +		    $rawline =~ /under the terms of the GNU Lesser General Public/ ||
>> +		    $rawline =~ /Permission is hereby granted, free of charge/ ||
>> +		    $rawline =~ /GNU GPL, version 2 or later/ ||
>> +		    $rawline =~ /See the COPYING file/) {
>> +			$fileinfo->{facts}->{sawboilerplate} = 1;
>> +		}
>> +
>>   		if ($rawline =~ m,(SPDX-[a-zA-Z0-9-_]+):,) {
>>   			my $tag = $1;
>>   			my @permitted = qw(
>> -- 
>> 2.49.0
>>
> 
> With regards,
> Daniel

Re: [PATCH v3 9/9] scripts/checkpatch: reject license boilerplate on new files

[Cédric Le Goater]

On 5/15/25 18:06, Cédric Le Goater wrote:
> On 5/15/25 18:05, Daniel P. Berrangé wrote:
>> On Thu, May 15, 2025 at 02:59:36PM +0100, Daniel P. Berrangé wrote:
>>> The previous commit mandates use of SPDX-License-Identifier on common
>>> source files, and encourages it on all other files.
>>>
>>> Some contributors are none the less still also including the license
>>> boilerplate text. This is redundant and will potentially cause
>>> trouble if inconsistent with the SPDX declaration.
>>>
>>> Match common boilerplate text blurbs and report them as invalid,
>>> for newly added files.
>>>
>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>>>   scripts/checkpatch.pl | 16 ++++++++++++++++
>>>   1 file changed, 16 insertions(+)
>>>
>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>> index 87050e6677..cb1942c021 100755
>>> --- a/scripts/checkpatch.pl
>>> +++ b/scripts/checkpatch.pl
>>> @@ -1496,6 +1496,13 @@ sub process_end_of_file {
>>>                    "' need 'SPDX-License-Identifier'?");
>>>           }
>>>       }
>>> +    if ($fileinfo->{action} eq "new" &&
>>> +        !exists $fileinfo->{facts}->{sawboilerplate}) {
>>
>> /face-palm  - I forgot to remove the '!' here so the check is
>> inverted and my test patch had two very similarly named files
>> so didn't notice it :-(
> 
> Oh. I didn't see. No need to resend. I will fix in my tree.

It is now catching valid errors on :


* https://lore.kernel.org/qemu-devel/20250512180230.50129-5-rreyes@linux.ibm.com/
   ERROR: New file 'hw/s390x/ap-stub.c' must not have license boilerplate header text unless this file is copied from existing code with such text already present.
   total: 1 errors, 0 warnings, 67 lines checked

* https://lore.kernel.org/qemu-devel/1747063973-124548-7-git-send-email-steven.sistare@oracle.com/
   ERROR: New file 'hw/vfio/cpr-legacy.c' requires 'SPDX-License-Identifier'
   ERROR: New file 'hw/vfio/cpr-legacy.c' must not have license boilerplate header text unless this file is copied from existing code with such text already present.
   WARNING: added, moved or deleted file(s):

* https://lore.kernel.org/qemu-devel/1747063973-124548-36-git-send-email-steven.sistare@oracle.com/
   ERROR: New file 'hw/vfio/cpr-iommufd.c' requires 'SPDX-License-Identifier'
   ERROR: New file 'hw/vfio/cpr-iommufd.c' must not have license boilerplate header text unless this file is copied from existing code with such text already present.
   WARNING: added, moved or deleted file(s):
   
     hw/vfio/cpr-iommufd.c
   
   Does MAINTAINERS need updating?
   
   total: 2 errors, 1 warnings, 161 lines checked

* https://lore.kernel.org/qemu-devel/20250515154413.210315-1-john.levon@nutanix.com
   ERROR: New file 'hw/vfio-user/container.h' must not have license boilerplate header text unless this file is copied from existing code with such text already present.
   ERROR: New file 'hw/vfio-user/container.c' must not have license boilerplate header text unless this file is copied from existing code with such text already present.
   ERROR: New file 'hw/vfio-user/pci.c' must not have license boilerplate header text unless this file is copied from existing code with such text already present.
   WARNING: Does new file 'hw/vfio-user/meson.build' need 'SPDX-License-Identifier'?
   total: 3 errors, 1 warnings, 490 lines checked


and more.


Tested-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

Re: [PATCH v3 9/9] scripts/checkpatch: reject license boilerplate on new files

[Daniel P. Berrangé]

On Thu, May 15, 2025 at 06:19:45PM +0200, Cédric Le Goater wrote:
> On 5/15/25 18:06, Cédric Le Goater wrote:
> > On 5/15/25 18:05, Daniel P. Berrangé wrote:
> > > On Thu, May 15, 2025 at 02:59:36PM +0100, Daniel P. Berrangé wrote:
> > > > The previous commit mandates use of SPDX-License-Identifier on common
> > > > source files, and encourages it on all other files.
> > > > 
> > > > Some contributors are none the less still also including the license
> > > > boilerplate text. This is redundant and will potentially cause
> > > > trouble if inconsistent with the SPDX declaration.
> > > > 
> > > > Match common boilerplate text blurbs and report them as invalid,
> > > > for newly added files.
> > > > 
> > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > > ---
> > > >   scripts/checkpatch.pl | 16 ++++++++++++++++
> > > >   1 file changed, 16 insertions(+)


> It is now catching valid errors on :
> 
> 
> * https://lore.kernel.org/qemu-devel/20250512180230.50129-5-rreyes@linux.ibm.com/
>   ERROR: New file 'hw/s390x/ap-stub.c' must not have license boilerplate header text unless this file is copied from existing code with such text already present.
>   total: 1 errors, 0 warnings, 67 lines checked
> 
> * https://lore.kernel.org/qemu-devel/1747063973-124548-7-git-send-email-steven.sistare@oracle.com/
>   ERROR: New file 'hw/vfio/cpr-legacy.c' requires 'SPDX-License-Identifier'
>   ERROR: New file 'hw/vfio/cpr-legacy.c' must not have license boilerplate header text unless this file is copied from existing code with such text already present.
>   WARNING: added, moved or deleted file(s):
> 
> * https://lore.kernel.org/qemu-devel/1747063973-124548-36-git-send-email-steven.sistare@oracle.com/
>   ERROR: New file 'hw/vfio/cpr-iommufd.c' requires 'SPDX-License-Identifier'
>   ERROR: New file 'hw/vfio/cpr-iommufd.c' must not have license boilerplate header text unless this file is copied from existing code with such text already present.
>   WARNING: added, moved or deleted file(s):
>     hw/vfio/cpr-iommufd.c
>   Does MAINTAINERS need updating?
>   total: 2 errors, 1 warnings, 161 lines checked
> 
> * https://lore.kernel.org/qemu-devel/20250515154413.210315-1-john.levon@nutanix.com
>   ERROR: New file 'hw/vfio-user/container.h' must not have license boilerplate header text unless this file is copied from existing code with such text already present.
>   ERROR: New file 'hw/vfio-user/container.c' must not have license boilerplate header text unless this file is copied from existing code with such text already present.
>   ERROR: New file 'hw/vfio-user/pci.c' must not have license boilerplate header text unless this file is copied from existing code with such text already present.
>   WARNING: Does new file 'hw/vfio-user/meson.build' need 'SPDX-License-Identifier'?
>   total: 3 errors, 1 warnings, 490 lines checked
> 
> 
> and more.

That nicely demonstrates the value of this extra check :-)

> 
> 
> Tested-by: Cédric Le Goater <clg@redhat.com>
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> 
> Thanks,
> 
> C.
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 9/9] scripts/checkpatch: reject license boilerplate on new files

[Peter Maydell]

On Thu, 15 May 2025 at 15:00, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The previous commit mandates use of SPDX-License-Identifier on common
> source files, and encourages it on all other files.
>
> Some contributors are none the less still also including the license
> boilerplate text. This is redundant and will potentially cause
> trouble if inconsistent with the SPDX declaration.
>
> Match common boilerplate text blurbs and report them as invalid,
> for newly added files.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  scripts/checkpatch.pl | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 87050e6677..cb1942c021 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1496,6 +1496,13 @@ sub process_end_of_file {
>                              "' need 'SPDX-License-Identifier'?");
>                 }
>         }
> +       if ($fileinfo->{action} eq "new" &&
> +           !exists $fileinfo->{facts}->{sawboilerplate}) {
> +               ERROR("New file '" . $fileinfo->{filenew} . "' must " .
> +                     "not have license boilerplate header text unless " .
> +                     "this file is copied from existing code with such " .
> +                     "text already present.");

Should we also say here
  The SPDX-License-Identifier line is sufficient.

(just to make it clear why we're not allowing the boilerplate text) ?

> +       }
>  }
>
>  sub process {
> @@ -1798,6 +1805,15 @@ sub process {
>                         &checkspdx($realfile, $1);
>                 }
>
> +               if ($rawline =~ /licensed under the terms of the GNU GPL/ ||
> +                   $rawline =~ /under the terms of the GNU General Public License/ ||
> +                   $rawline =~ /under the terms of the GNU Lesser General Public/ ||
> +                   $rawline =~ /Permission is hereby granted, free of charge/ ||
> +                   $rawline =~ /GNU GPL, version 2 or later/ ||
> +                   $rawline =~ /See the COPYING file/) {
> +                       $fileinfo->{facts}->{sawboilerplate} = 1;
> +               }
> +

We could perhaps pull this out into a top level variable, similar
to how the script pre-defines some other long regexes it wants to
match against (untested code):

# Match text found in common license boilerplate comments:
# for new files the SPDX-License-Identifier line is sufficient.
our $LICENSE_BOILERPLATE = qr{
    licensed under the terms of the GNU GPL|
    under the terms of the GNU General Public License|
    under the terms of the GNU Lesser General Public|
    Permission is hereby granted, free of charge|
    GNU GPL, version 2 or later|
    See the COPYING file
}x;

and then
    if ($rawline =~ /$LICENSE_BOILERPLATE/) {

This seems to me a little better than having it all inline
in the already rather large process() function.

But either way
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM