[PATCH 0/2] checkpatch: Do appropriate kernel->qemu renaming

[PATCH 0/2] checkpatch: Do appropriate kernel->qemu renaming

[Aleksandar Markovic]

There are several places where 'kernel' is mentioned instead of
'qemu' in checkpatch.pl.

This small series corrects this.

Aleksandar Markovic (2):
  checkpatch: Rename top_of_kernel_tree() to top_of_qemu_tree()
  checkpatch: Change occurences of 'kernel' to 'qemu' in user messages

 scripts/checkpatch.pl | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

-- 
2.20.1

[PATCH 1/2] checkpatch: Rename top_of_kernel_tree() to top_of_qemu_tree()

[Aleksandar Markovic]

This subroutine establishes top of qemu, not kernel, tree.

Do appropriate renaming.

Signed-off-by: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
---
 scripts/checkpatch.pl | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 0ba213e9f2..c760c5a4a9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -190,14 +190,14 @@ if ($terse) {
 
 if ($tree) {
 	if (defined $root) {
-		if (!top_of_kernel_tree($root)) {
+		if (!top_of_qemu_tree($root)) {
 			die "$P: $root: --root does not point at a valid tree\n";
 		}
 	} else {
-		if (top_of_kernel_tree('.')) {
+		if (top_of_qemu_tree('.')) {
 			$root = '.';
 		} elsif ($0 =~ m@(.*)/scripts/[^/]*$@ &&
-						top_of_kernel_tree($1)) {
+						top_of_qemu_tree($1)) {
 			$root = $1;
 		}
 	}
@@ -456,7 +456,7 @@ if ($chk_branch) {
 
 exit($exit);
 
-sub top_of_kernel_tree {
+sub top_of_qemu_tree {
 	my ($root) = @_;
 
 	my @tree_check = (
-- 
2.20.1

[PATCH 2/2] checkpatch: Change occurences of 'kernel' to 'qemu' in user messages

[Aleksandar Markovic]

It is odd that we inform user that, for example, his current working
directory is not kernel root, when, in face, we mean qemu root.

Replace that and few other similar odd user messages.

Signed-off-by: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
---
 scripts/checkpatch.pl | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c760c5a4a9..f46c18e413 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -49,7 +49,7 @@ Version: $V
 
 Options:
   -q, --quiet                quiet
-  --no-tree                  run without a kernel tree
+  --no-tree                  run without a qemu tree
   --no-signoff               do not check for 'Signed-off-by' line
   --patch                    treat FILE as patchfile
   --branch                   treat args as GIT revision list
@@ -57,7 +57,7 @@ Options:
   --terse                    one line per report
   -f, --file                 treat FILE as regular source file
   --strict                   fail if only warnings are found
-  --root=PATH                PATH to the kernel tree root
+  --root=PATH                PATH to the qemu tree root
   --no-summary               suppress the per-file summary
   --mailback                 only produce a report in case of warnings/errors
   --summary-file             include the filename in summary
@@ -203,7 +203,7 @@ if ($tree) {
 	}
 
 	if (!defined $root) {
-		print "Must be run from the top-level dir. of a kernel tree\n";
+		print "Must be run from the top-level dir. of a qemu tree\n";
 		exit(2);
 	}
 }
-- 
2.20.1

Re: [PATCH 2/2] checkpatch: Change occurences of 'kernel' to 'qemu' in user messages

[Peter Maydell]

On Sat, 20 Jun 2020 at 14:33, Aleksandar Markovic
<aleksandar.qemu.devel@gmail.com> wrote:
>
> It is odd that we inform user that, for example, his current working
> directory is not kernel root, when, in face, we mean qemu root.
>
> Replace that and few other similar odd user messages.
>
> Signed-off-by: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
> ---
>  scripts/checkpatch.pl | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Regardless of my comments on the cover letter, for user facing
messages this makes sense, especially since it's only 3 lines.

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

thanks
-- PMM

Re: [PATCH 2/2] checkpatch: Change occurences of 'kernel' to 'qemu' in user messages

[Paolo Bonzini]

On 20/06/20 17:04, Peter Maydell wrote:
> On Sat, 20 Jun 2020 at 14:33, Aleksandar Markovic
> <aleksandar.qemu.devel@gmail.com> wrote:
>>
>> It is odd that we inform user that, for example, his current working
>> directory is not kernel root, when, in face, we mean qemu root.
>>
>> Replace that and few other similar odd user messages.
>>
>> Signed-off-by: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
>> ---
>>  scripts/checkpatch.pl | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Regardless of my comments on the cover letter, for user facing
> messages this makes sense, especially since it's only 3 lines.
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> thanks
> -- PMM
> 

Queued, thanks.

paolo

Re: [PATCH 2/2] checkpatch: Change occurences of 'kernel' to 'qemu' in user messages

[Aleksandar Markovic]

On Wed, Jul 1, 2020 at 4:20 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 20/06/20 17:04, Peter Maydell wrote:
> > On Sat, 20 Jun 2020 at 14:33, Aleksandar Markovic
> > <aleksandar.qemu.devel@gmail.com> wrote:
> >>
> >> It is odd that we inform user that, for example, his current working
> >> directory is not kernel root, when, in face, we mean qemu root.
> >>
> >> Replace that and few other similar odd user messages.
> >>
> >> Signed-off-by: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
> >> ---
> >>  scripts/checkpatch.pl | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > Regardless of my comments on the cover letter, for user facing
> > messages this makes sense, especially since it's only 3 lines.
> >
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> >
> > thanks
> > -- PMM
> >
>
> Queued, thanks.
>

Thanks a lot, Paolo.

There is though one spelling mistake in my commit message:

s/face/fact

Truly yours,
Aleksandar

> paolo
>

Re: [PATCH 2/2] checkpatch: Change occurences of 'kernel' to 'qemu' in user messages

[Aleksandar Markovic]

On Sat, Jun 20, 2020 at 9:32 AM Aleksandar Markovic
<aleksandar.qemu.devel@gmail.com> wrote:
>
> It is odd that we inform user that, for example, his current working
> directory is not kernel root, when, in face, we mean qemu root.
>
> Replace that and few other similar odd user messages.
>
> Signed-off-by: Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
> ---

Paolo, Stefan, Michael,

Are some of you willing to select this patch for your queues?

I could include it in my mips queue, scheduled to be sent soon, but I
certainly don't want to be seen intruding into someone else's area.

Truly yours, Aleksandar

>  scripts/checkpatch.pl | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index c760c5a4a9..f46c18e413 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -49,7 +49,7 @@ Version: $V
>
>  Options:
>    -q, --quiet                quiet
> -  --no-tree                  run without a kernel tree
> +  --no-tree                  run without a qemu tree
>    --no-signoff               do not check for 'Signed-off-by' line
>    --patch                    treat FILE as patchfile
>    --branch                   treat args as GIT revision list
> @@ -57,7 +57,7 @@ Options:
>    --terse                    one line per report
>    -f, --file                 treat FILE as regular source file
>    --strict                   fail if only warnings are found
> -  --root=PATH                PATH to the kernel tree root
> +  --root=PATH                PATH to the qemu tree root
>    --no-summary               suppress the per-file summary
>    --mailback                 only produce a report in case of warnings/errors
>    --summary-file             include the filename in summary
> @@ -203,7 +203,7 @@ if ($tree) {
>         }
>
>         if (!defined $root) {
> -               print "Must be run from the top-level dir. of a kernel tree\n";
> +               print "Must be run from the top-level dir. of a qemu tree\n";
>                 exit(2);
>         }
>  }
> --
> 2.20.1
>

Re: [PATCH 0/2] checkpatch: Do appropriate kernel->qemu renaming

[Peter Maydell]

On Sat, 20 Jun 2020 at 14:33, Aleksandar Markovic
<aleksandar.qemu.devel@gmail.com> wrote:
> There are several places where 'kernel' is mentioned instead of
> 'qemu' in checkpatch.pl.
>
> This small series corrects this.

So, the thing about this kind of change is that it's implicitly saying "we have
forked checkpatch and will not try to update to newer versions of it from
the kernel tree", because this sort of no-behavioural-change tends to
get in the way of that kind of update by obscuring the delta between
the kernel version and ours. Effectively I think we *have* ended up
with our own fork,
simply because we haven't cared to do that kind of update from the kernel's
version and we've gradually added our own changes to our copy. But it seems
like it's worth discussing the general principle.

thanks
-- PMM

Re: [PATCH 0/2] checkpatch: Do appropriate kernel->qemu renaming

[Aleksandar Markovic]

суб, 20. јун 2020. у 16:25 Peter Maydell <peter.maydell@linaro.org> је
написао/ла:
>
> On Sat, 20 Jun 2020 at 14:33, Aleksandar Markovic
> <aleksandar.qemu.devel@gmail.com> wrote:
> > There are several places where 'kernel' is mentioned instead of
> > 'qemu' in checkpatch.pl.
> >
> > This small series corrects this.
>
> So, the thing about this kind of change is that it's implicitly saying "we have
> forked checkpatch and will not try to update to newer versions of it from
> the kernel tree", because this sort of no-behavioural-change tends to
> get in the way of that kind of update by obscuring the delta between
> the kernel version and ours. Effectively I think we *have* ended up
> with our own fork,
> simply because we haven't cared to do that kind of update from the kernel's
> version and we've gradually added our own changes to our copy. But it seems
> like it's worth discussing the general principle.
>

I do not say (explicitly or implicitly) that we should or should not
follow and attempt to mirror changes in checkpatch.pl from kernel. (In
fact, I think we should.)

However, I don't think that several differences (in this series, I
thinks altogether 7 lines) that would originate from difference of
names QEMU vs. kernel would not be any significant obstacle for a
potential future attempts to do comparison QEMU checkpatch vs kernel
chekpatch.

Take a look at two versions of top_of_kernel_tree below - they already
differ in their body. Left this way, QEMU's checkpatch version simply
violates basic naming principles in software development. And, it
looks we want to sacrifice the principle - just for the sake of
convenience of a potential engineer having 7 less line in his diff
(out of much more).

I don't insist on these two patches. I, of course, leave the decision
to Peter, Paolo, Stefan, Michael, or others tracking kernel's
checkpatch script.

Thanks,
Aleksandar

QEMU version:

sub top_of_kernel_tree {
my ($root) = @_;

my @tree_check = (
"COPYING", "MAINTAINERS", "Makefile",
"README.rst", "docs", "VERSION",
"linux-user", "softmmu"
);

foreach my $check (@tree_check) {
if (! -e $root . '/' . $check) {
return 0;
}
}
return 1;
}

Kernel version:

sub top_of_kernel_tree {
my ($root) = @_;

my @tree_check = (
"COPYING", "CREDITS", "Kbuild", "MAINTAINERS", "Makefile",
"README", "Documentation", "arch", "include", "drivers",
"fs", "init", "ipc", "kernel", "lib", "scripts",
);

foreach my $check (@tree_check) {
if (! -e $root . '/' . $check) {
return 0;
}
}
return 1;
}

> thanks
> -- PMM

Re: [PATCH 0/2] checkpatch: Do appropriate kernel->qemu renaming

[Aleksandar Markovic]

суб, 20. јун 2020. у 17:09 Aleksandar Markovic
<aleksandar.qemu.devel@gmail.com> је написао/ла:
>
> суб, 20. јун 2020. у 16:25 Peter Maydell <peter.maydell@linaro.org> је
> написао/ла:
> >
> > On Sat, 20 Jun 2020 at 14:33, Aleksandar Markovic
> > <aleksandar.qemu.devel@gmail.com> wrote:
> > > There are several places where 'kernel' is mentioned instead of
> > > 'qemu' in checkpatch.pl.
> > >
> > > This small series corrects this.
> >
> > So, the thing about this kind of change is that it's implicitly saying "we have
> > forked checkpatch and will not try to update to newer versions of it from
> > the kernel tree", because this sort of no-behavioural-change tends to
> > get in the way of that kind of update by obscuring the delta between
> > the kernel version and ours. Effectively I think we *have* ended up
> > with our own fork,
> > simply because we haven't cared to do that kind of update from the kernel's
> > version and we've gradually added our own changes to our copy. But it seems
> > like it's worth discussing the general principle.
> >
>
> I do not say (explicitly or implicitly) that we should or should not
> follow and attempt to mirror changes in checkpatch.pl from kernel. (In
> fact, I think we should.)
>
> However, I don't think that several differences (in this series, I
> thinks altogether 7 lines) that would originate from difference of
> names QEMU vs. kernel would not be any significant obstacle for a
> potential future attempts to do comparison QEMU checkpatch vs kernel
> chekpatch.
>

Sorry, I meant to say:

However, I don't think that several differences (in this series, I
think, altogether 7 lines) that would originate from difference of
names (QEMU vs. kernel) would be any significant obstacle for a
potential future attempts to do comparison QEMU checkpatch vs kernel
checkpatch.



> Take a look at two versions of top_of_kernel_tree below - they already
> differ in their body. Left this way, QEMU's checkpatch version simply
> violates basic naming principles in software development. And, it
> looks we want to sacrifice the principle - just for the sake of
> convenience of a potential engineer having 7 less line in his diff
> (out of much more).
>
> I don't insist on these two patches. I, of course, leave the decision
> to Peter, Paolo, Stefan, Michael, or others tracking kernel's
> checkpatch script.
>
> Thanks,
> Aleksandar
>
> QEMU version:
>
> sub top_of_kernel_tree {
> my ($root) = @_;
>
> my @tree_check = (
> "COPYING", "MAINTAINERS", "Makefile",
> "README.rst", "docs", "VERSION",
> "linux-user", "softmmu"
> );
>
> foreach my $check (@tree_check) {
> if (! -e $root . '/' . $check) {
> return 0;
> }
> }
> return 1;
> }
>
> Kernel version:
>
> sub top_of_kernel_tree {
> my ($root) = @_;
>
> my @tree_check = (
> "COPYING", "CREDITS", "Kbuild", "MAINTAINERS", "Makefile",
> "README", "Documentation", "arch", "include", "drivers",
> "fs", "init", "ipc", "kernel", "lib", "scripts",
> );
>
> foreach my $check (@tree_check) {
> if (! -e $root . '/' . $check) {
> return 0;
> }
> }
> return 1;
> }
>
> > thanks
> > -- PMM

Re: [PATCH 0/2] checkpatch: Do appropriate kernel->qemu renaming

[Stefan Hajnoczi]

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

On Sat, Jun 20, 2020 at 05:20:41PM +0200, Aleksandar Markovic wrote:
> суб, 20. јун 2020. у 17:09 Aleksandar Markovic
> <aleksandar.qemu.devel@gmail.com> је написао/ла:
> >
> > суб, 20. јун 2020. у 16:25 Peter Maydell <peter.maydell@linaro.org> је
> > написао/ла:
> > >
> > > On Sat, 20 Jun 2020 at 14:33, Aleksandar Markovic
> > > <aleksandar.qemu.devel@gmail.com> wrote:
> > > > There are several places where 'kernel' is mentioned instead of
> > > > 'qemu' in checkpatch.pl.
> > > >
> > > > This small series corrects this.
> > >
> > > So, the thing about this kind of change is that it's implicitly saying "we have
> > > forked checkpatch and will not try to update to newer versions of it from
> > > the kernel tree", because this sort of no-behavioural-change tends to
> > > get in the way of that kind of update by obscuring the delta between
> > > the kernel version and ours. Effectively I think we *have* ended up
> > > with our own fork,
> > > simply because we haven't cared to do that kind of update from the kernel's
> > > version and we've gradually added our own changes to our copy. But it seems
> > > like it's worth discussing the general principle.
> > >
> >
> > I do not say (explicitly or implicitly) that we should or should not
> > follow and attempt to mirror changes in checkpatch.pl from kernel. (In
> > fact, I think we should.)
> >
> > However, I don't think that several differences (in this series, I
> > thinks altogether 7 lines) that would originate from difference of
> > names QEMU vs. kernel would not be any significant obstacle for a
> > potential future attempts to do comparison QEMU checkpatch vs kernel
> > chekpatch.
> >
> 
> Sorry, I meant to say:
> 
> However, I don't think that several differences (in this series, I
> think, altogether 7 lines) that would originate from difference of
> names (QEMU vs. kernel) would be any significant obstacle for a
> potential future attempts to do comparison QEMU checkpatch vs kernel
> checkpatch.

I looked through the 2020, 2019, and 2018 git log for checkpatch.pl and
found few commits that were directly applied from Linux. Some were
rewritten from scratch for QEMU and inspired by Linux commits. In
practice QEMU maintains a fork rather than a mirror of checkpatch.pl.

Let's encourage improvements to checkpatch.pl, even if that means
diverging further from Linux, because it's a valuable tool for improving
coding style, quality, etc. If there is a feeling that changes to
checkpatch.pl are discouraged there will be more coding style
inconsistencies, reviewers will have to point out trivial issues
manually, etc because we're afraid to touch the script.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] checkpatch: Do appropriate kernel->qemu renaming

[Michael S. Tsirkin]

On Sat, Jun 20, 2020 at 03:32:05PM +0200, Aleksandar Markovic wrote:
> There are several places where 'kernel' is mentioned instead of
> 'qemu' in checkpatch.pl.
> 
> This small series corrects this.


fine by me

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> Aleksandar Markovic (2):
>   checkpatch: Rename top_of_kernel_tree() to top_of_qemu_tree()
>   checkpatch: Change occurences of 'kernel' to 'qemu' in user messages
> 
>  scripts/checkpatch.pl | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> -- 
> 2.20.1