[PATCH v2 0/7] scripts/checkpatch: refactor & fix SPDX-License-Identifier detection

[PATCH v2 0/7] scripts/checkpatch: refactor & 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.

Daniel P. Berrangé (7):
  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: use new hook for MAINTAINERS update check
  scripts/checkpatch: reimplement mandate for SPDX-License-Identifier

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

-- 
2.49.0

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

[Daniel P. Berrangé]

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

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>
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

[PATCH v2 2/7] scripts/checkpatch.pl: fix various indentation mistakes

[Daniel P. Berrangé]

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

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

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

[PATCH v2 3/7] scripts/checkpatch: introduce tracking of file start/end

[Daniel P. Berrangé]

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

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.

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

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

[Daniel P. Berrangé]

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

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

[PATCH v2 5/7] scripts/checkpatch: use new hook for file permissions check

[Daniel P. Berrangé]

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

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

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

[PATCH v2 6/7] scripts/checkpatch: use new hook for MAINTAINERS update check

[Daniel P. Berrangé]

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

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.

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 4a18daa384..d416a6dcf9 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 "MAINTAINEfRS") {
+			$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

[PATCH v2 7/7] scripts/checkpatch: reimplement mandate for SPDX-License-Identifier

[Daniel P. Berrangé]

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

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.

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 d416a6dcf9..95609ca010 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 v2 7/7] scripts/checkpatch: reimplement mandate for SPDX-License-Identifier

[Cédric Le Goater]

On 5/12/25 20:24, Daniel P. Berrangé wrote:
> From: Daniel P. Berrangé <berrange@redhat.com>
> 
> 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.
> 
> 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 d416a6dcf9..95609ca010 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);
>   		}
>   

When run on patch :

   https://lore.kernel.org/qemu-devel/20250512091014.3454083-2-kane_chen@aspeedtech.com/

./scripts/checkpatch.pl outputs :
   
   ERROR: New file 'hw/misc/aspeed_otpmem.c' requires 'SPDX-License-Identifier'
   WARNING: line over 80 characters
   #288: FILE: include/hw/misc/aspeed_otpmem.h:22:
   +    void (*prog)(AspeedOTPMemState *s, uint32_t addr, uint32_t val, Error **errp);
   
   WARNING: line over 80 characters
   #289: FILE: include/hw/misc/aspeed_otpmem.h:23:
   +    void (*set_default)(AspeedOTPMemState *s, uint32_t addr, uint32_t val, Error **errp);
   
   ERROR: New file 'include/hw/misc/aspeed_otpmem.h' requires 'SPDX-License-Identifier'
   WARNING: added, moved or deleted file(s):
   
     hw/misc/aspeed_otpmem.c
     include/hw/misc/aspeed_otpmem.h
   
   Does MAINTAINERS need updating?
   
   total: 2 errors, 3 warnings, 255 lines checked
   
Looks good.


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

Thanks,

C.

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

[Cédric Le Goater]

On 5/12/25 20:24, Daniel P. Berrangé wrote:
> From: Daniel P. Berrangé <berrange@redhat.com>
> 
> 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>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>


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

Thanks,

C.


> ---
>   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);

Re: [PATCH v2 2/7] scripts/checkpatch.pl: fix various indentation mistakes

[Cédric Le Goater]

On 5/12/25 20:24, Daniel P. Berrangé wrote:
> From: Daniel P. Berrangé <berrange@redhat.com>
> 
> Various checks in the code were under-indented relative to other
> surrounding code.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>


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

Thanks,

C.


> ---
>   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\(/) {

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

[Cédric Le Goater]

On 5/12/25 20:24, Daniel P. Berrangé wrote:
> From: Daniel P. Berrangé <berrange@redhat.com>
> 
> 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.
> 
> 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 4a18daa384..d416a6dcf9 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 "MAINTAINEfRS") {

MAINTAINEfRS ? looks like a typo.


With that fixed,


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

Thanks,

C.



> +			$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);

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

[Cédric Le Goater]

On 5/12/25 20:24, Daniel P. Berrangé wrote:
> From: Daniel P. Berrangé <berrange@redhat.com>
> 
> The file permissions check is the kind of check intended to be performed
> in the new start of file hook.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>


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

Thanks,

C.


> ---
>   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$/) {

Re: [PATCH v2 3/7] scripts/checkpatch: introduce tracking of file start/end

[Cédric Le Goater]

On 5/12/25 20:24, Daniel P. Berrangé wrote:
> From: Daniel P. Berrangé <berrange@redhat.com>
> 
> 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.
> 
> 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)

It would be nice to have some support for 'quilt' patches too. This would
mean being able to match '(---|+++) /dev/null' for new and deleted files.
Anyhow, that's for another series.

> +#  '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");
>   	}

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

[Daniel P. Berrangé]

On Wed, May 14, 2025 at 01:51:52PM +0200, Cédric Le Goater wrote:
> On 5/12/25 20:24, Daniel P. Berrangé wrote:
> > From: Daniel P. Berrangé <berrange@redhat.com>
> > 
> > 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.
> > 
> > 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 4a18daa384..d416a6dcf9 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 "MAINTAINEfRS") {
> 
> MAINTAINEfRS ? looks like a typo.

Opps, an intentional typo when I was testing logic that
I accidentally committed.

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

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 v2 7/7] scripts/checkpatch: reimplement mandate for SPDX-License-Identifier

[Daniel P. Berrangé]

On Mon, May 12, 2025 at 07:24:47PM +0100, Daniel P. Berrangé wrote:
> From: Daniel P. Berrangé <berrange@redhat.com>
> 
> 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.
> 
> 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 d416a6dcf9..95609ca010 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)$/) {

Slight tweak needed here as 'Makefile' is a filename base
not an extension:

                /(\.(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
> 

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 :|