[PATCH v2] scripts/checkpatch: more checks on files imported from Linux

[PATCH v2] scripts/checkpatch: more checks on files imported from Linux

[Stefano Garzarella]

If a file imported from Linux is touched, emit a warning and suggest
using scripts/update-linux-headers.sh.

Also check that updating imported files from Linux are not mixed with
other changes, in which case emit an error.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
v2:
- added an error when mixing imported files with other changes [Daniel,
  Cornelia]

v1: https://patchew.org/QEMU/20240717093752.50595-1-sgarzare@redhat.com/
---
 scripts/checkpatch.pl | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index ff373a7083..65b6f46f90 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1374,6 +1374,9 @@ 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;
 	my $non_utf8_charset = 0;
 
 	our @report = ();
@@ -1673,6 +1676,27 @@ sub process {
 # ignore non-hunk lines and lines being removed
 		next if (!$hunk_line || $line =~ /^-/);
 
+# Check that updating imported files from Linux are not mixed with other changes
+		if ($realfile =~ /^(linux-headers|include\/standard-headers)\//) {
+			if (!$in_imported_file) {
+				WARN("added, moved or deleted file(s) " .
+				     "imported from Linux, are you using " .
+				     "scripts/update-linux-headers.sh?\n" .
+				     $herecurr);
+			}
+			$in_imported_file = 1;
+		} else {
+			$in_no_imported_file = 1;
+		}
+
+		if (!$reported_mixing_imported_file &&
+		    $in_imported_file && $in_no_imported_file) {
+			ERROR("headers imported from Linux should be self-" .
+			      "contained in a patch with no other changes\n" .
+			      $herecurr);
+			$reported_mixing_imported_file = 1;
+		}
+
 # ignore files that are being periodically imported from Linux
 		next if ($realfile =~ /^(linux-headers|include\/standard-headers)\//);
 
-- 
2.45.2

Re: [PATCH v2] scripts/checkpatch: more checks on files imported from Linux

[Daniel P. Berrangé]

On Thu, Jul 18, 2024 at 09:20:50AM +0200, Stefano Garzarella wrote:
> If a file imported from Linux is touched, emit a warning and suggest
> using scripts/update-linux-headers.sh.
> 
> Also check that updating imported files from Linux are not mixed with
> other changes, in which case emit an error.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> v2:
> - added an error when mixing imported files with other changes [Daniel,
>   Cornelia]
> 
> v1: https://patchew.org/QEMU/20240717093752.50595-1-sgarzare@redhat.com/
> ---
>  scripts/checkpatch.pl | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@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] scripts/checkpatch: more checks on files imported from Linux

[Cornelia Huck]

On Thu, Jul 18 2024, Stefano Garzarella <sgarzare@redhat.com> wrote:

> If a file imported from Linux is touched, emit a warning and suggest
> using scripts/update-linux-headers.sh.
>
> Also check that updating imported files from Linux are not mixed with
> other changes, in which case emit an error.
>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> v2:
> - added an error when mixing imported files with other changes [Daniel,
>   Cornelia]
>
> v1: https://patchew.org/QEMU/20240717093752.50595-1-sgarzare@redhat.com/
> ---
>  scripts/checkpatch.pl | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v2] scripts/checkpatch: more checks on files imported from Linux

[Alex Bennée]

Stefano Garzarella <sgarzare@redhat.com> writes:

> If a file imported from Linux is touched, emit a warning and suggest
> using scripts/update-linux-headers.sh.
>
> Also check that updating imported files from Linux are not mixed with
> other changes, in which case emit an error.
>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Queued to for-9.1/misc-fixes, thanks.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2] scripts/checkpatch: more checks on files imported from Linux

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1469 bytes --]

On Thu, 2024-07-18 at 09:20 +0200, Stefano Garzarella wrote:
> If a file imported from Linux is touched, emit a warning and suggest
> using scripts/update-linux-headers.sh.
> 
> Also check that updating imported files from Linux are not mixed with
> other changes, in which case emit an error.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

This now seems to complain about a commit which simply adds a file to
update-linux-headers.sh and simultaneously adds the newly-imported
file. Such as commit aa274c33c39e7de981dc195abe60e1a246c9d248.

Was that the intent? Do we really need to have *three* separate
commits, one for update-linux-headers.sh, one to import the new header,
and then the third to add the QEMU code which actually uses the new
header?

$ scripts/checkpatch.pl 0001-update-linux-headers-import-linux-kvm_para.h-header.patch
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#36: 
new file mode 100644

WARNING: added, moved or deleted file(s) imported from Linux, are you using scripts/update-linux-headers.sh?
#41: FILE: include/standard-headers/linux/kvm_para.h:1:
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */

ERROR: headers imported from Linux should be self-contained in a patch with no other changes
#99: FILE: scripts/update-linux-headers.sh:63:
                                      -e 'linux/kernel' \

total: 1 errors, 2 warnings, 100 lines checked


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]

Re: [PATCH v2] scripts/checkpatch: more checks on files imported from Linux

[Peter Maydell]

On Thu, 16 Jan 2025 at 09:26, David Woodhouse <dwmw2@infradead.org> wrote:
>
> On Thu, 2024-07-18 at 09:20 +0200, Stefano Garzarella wrote:
> > If a file imported from Linux is touched, emit a warning and suggest
> > using scripts/update-linux-headers.sh.
> >
> > Also check that updating imported files from Linux are not mixed with
> > other changes, in which case emit an error.
> >
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>
> This now seems to complain about a commit which simply adds a file to
> update-linux-headers.sh and simultaneously adds the newly-imported
> file. Such as commit aa274c33c39e7de981dc195abe60e1a246c9d248.
>
> Was that the intent? Do we really need to have *three* separate
> commits, one for update-linux-headers.sh, one to import the new header,
> and then the third to add the QEMU code which actually uses the new
> header?
>
> $ scripts/checkpatch.pl 0001-update-linux-headers-import-linux-kvm_para.h-header.patch
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #36:
> new file mode 100644
>
> WARNING: added, moved or deleted file(s) imported from Linux, are you using scripts/update-linux-headers.sh?
> #41: FILE: include/standard-headers/linux/kvm_para.h:1:
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>
> ERROR: headers imported from Linux should be self-contained in a patch with no other changes
> #99: FILE: scripts/update-linux-headers.sh:63:
>                                       -e 'linux/kernel' \

Just to state for the record the conclusion of our conversation
on IRC; yes, the error is correct. Header-imports from
Linux should be in a patch of their own which has no
changes except the results of running update-linux-headers.sh
(and ideally we would not have let aa274c33c39e7 through
code review). So, yes, you have a series of three patches:
 * change update-linux-headers.sh
 * automated-change-only commit resulting from running the script
 * the change you want to make that requires the new header

thanks
-- PMM