* [Linux-kernel-mentees] [PATCH] checkpatch: add fix option for BAD_SIGN_OFF
@ 2020-12-02 18:36 Aditya Srivastava
2020-12-08 16:44 ` Aditya
0 siblings, 1 reply; 9+ messages in thread
From: Aditya Srivastava @ 2020-12-02 18:36 UTC (permalink / raw)
To: lukas.bulwahn; +Cc: linux-kernel-mentees
Currently checkpatch warns us if the Co-developed-by line for an author
isn't immediately followed by the Signed-off-by line.
Generally, this warning occurs because of:
1) Absence of Signed-off-by line for the author from the patch
2) Misplaced Signed-off-by line for the author in the patch
Provide a simple fix by:
1) Inserting Signed-off-by line for the author immediately after the
Co-developed-by line
2) Removing Signed-off-by line for the author (if it pre-existed on
some other line)
Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
---
scripts/checkpatch.pl | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 5c9f13a97c12..d8f9d4d8b13d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2509,6 +2509,9 @@ sub process {
my @setup_docs = ();
my $setup_docs = 0;
+ my %signed_off_by_lines;
+ my %co_developed_by_lines;
+
my $camelcase_file_seeded = 0;
my $checklicenseline = 1;
@@ -2794,6 +2797,7 @@ sub process {
my $ctx = $1;
my ($email_name, $email_comment, $email_address, $comment1) = parse_email($ctx);
my ($author_name, $author_comment, $author_address, $comment2) = parse_email($author);
+ $signed_off_by_lines{$ctx} = $fixlinenr;
if ($email_address eq $author_address && $email_name eq $author_name) {
$author_sob = $ctx;
@@ -2989,11 +2993,17 @@ sub process {
}
}
if (!defined $lines[$linenr]) {
- WARN("BAD_SIGN_OFF",
- "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline);
+ if (WARN("BAD_SIGN_OFF",
+ "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline) &&
+ $fix) {
+ $co_developed_by_lines{$email} = $fixlinenr;
+ }
} elsif ($rawlines[$linenr] !~ /^\s*signed-off-by:\s*(.*)/i) {
- WARN("BAD_SIGN_OFF",
- "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
+ if (WARN("BAD_SIGN_OFF",
+ "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]) &&
+ $fix) {
+ $co_developed_by_lines{$email} = $fixlinenr;
+ }
} elsif ($1 ne $email) {
WARN("BAD_SIGN_OFF",
"Co-developed-by and Signed-off-by: name/email do not match \n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
@@ -7203,6 +7213,16 @@ sub process {
ERROR("NOT_UNIFIED_DIFF",
"Does not appear to be a unified-diff format patch\n");
}
+
+ if (%co_developed_by_lines) {
+ foreach my $co_developer (keys %co_developed_by_lines) {
+ if (exists($signed_off_by_lines{$co_developer})) {
+ fix_delete_line($signed_off_by_lines{$co_developer}, "Signed-off-by: $co_developer");
+ }
+ fix_insert_line($co_developed_by_lines{$co_developer} + 1, "Signed-off-by: $co_developer");
+ }
+ }
+
if ($is_patch && $has_commit_log && $chk_signoff) {
if ($signoff == 0) {
ERROR("MISSING_SIGN_OFF",
--
2.17.1
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix option for BAD_SIGN_OFF
2020-12-02 18:36 [Linux-kernel-mentees] [PATCH] checkpatch: add fix option for BAD_SIGN_OFF Aditya Srivastava
@ 2020-12-08 16:44 ` Aditya
2020-12-08 16:54 ` Lukas Bulwahn
0 siblings, 1 reply; 9+ messages in thread
From: Aditya @ 2020-12-08 16:44 UTC (permalink / raw)
To: lukas.bulwahn; +Cc: linux-kernel-mentees
On 3/12/20 12:06 am, Aditya Srivastava wrote:
> Currently checkpatch warns us if the Co-developed-by line for an author
> isn't immediately followed by the Signed-off-by line.
>
> Generally, this warning occurs because of:
> 1) Absence of Signed-off-by line for the author from the patch
> 2) Misplaced Signed-off-by line for the author in the patch
>
> Provide a simple fix by:
> 1) Inserting Signed-off-by line for the author immediately after the
> Co-developed-by line
>
> 2) Removing Signed-off-by line for the author (if it pre-existed on
> some other line)
>
> Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> ---
> scripts/checkpatch.pl | 28 ++++++++++++++++++++++++----
> 1 file changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 5c9f13a97c12..d8f9d4d8b13d 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2509,6 +2509,9 @@ sub process {
> my @setup_docs = ();
> my $setup_docs = 0;
>
> + my %signed_off_by_lines;
> + my %co_developed_by_lines;
> +
> my $camelcase_file_seeded = 0;
>
> my $checklicenseline = 1;
> @@ -2794,6 +2797,7 @@ sub process {
> my $ctx = $1;
> my ($email_name, $email_comment, $email_address, $comment1) = parse_email($ctx);
> my ($author_name, $author_comment, $author_address, $comment2) = parse_email($author);
> + $signed_off_by_lines{$ctx} = $fixlinenr;
>
> if ($email_address eq $author_address && $email_name eq $author_name) {
> $author_sob = $ctx;
> @@ -2989,11 +2993,17 @@ sub process {
> }
> }
> if (!defined $lines[$linenr]) {
> - WARN("BAD_SIGN_OFF",
> - "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline);
> + if (WARN("BAD_SIGN_OFF",
> + "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline) &&
> + $fix) {
> + $co_developed_by_lines{$email} = $fixlinenr;
> + }
> } elsif ($rawlines[$linenr] !~ /^\s*signed-off-by:\s*(.*)/i) {
> - WARN("BAD_SIGN_OFF",
> - "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
> + if (WARN("BAD_SIGN_OFF",
> + "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]) &&
> + $fix) {
> + $co_developed_by_lines{$email} = $fixlinenr;
> + }
> } elsif ($1 ne $email) {
> WARN("BAD_SIGN_OFF",
> "Co-developed-by and Signed-off-by: name/email do not match \n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
> @@ -7203,6 +7213,16 @@ sub process {
> ERROR("NOT_UNIFIED_DIFF",
> "Does not appear to be a unified-diff format patch\n");
> }
> +
> + if (%co_developed_by_lines) {
> + foreach my $co_developer (keys %co_developed_by_lines) {
> + if (exists($signed_off_by_lines{$co_developer})) {
> + fix_delete_line($signed_off_by_lines{$co_developer}, "Signed-off-by: $co_developer");
> + }
> + fix_insert_line($co_developed_by_lines{$co_developer} + 1, "Signed-off-by: $co_developer");
> + }
> + }
> +
> if ($is_patch && $has_commit_log && $chk_signoff) {
> if ($signoff == 0) {
> ERROR("MISSING_SIGN_OFF",
>
Hi Lukas
You probably missed this patch. Please review :)
Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix option for BAD_SIGN_OFF
2020-12-08 16:44 ` Aditya
@ 2020-12-08 16:54 ` Lukas Bulwahn
2020-12-08 17:44 ` Aditya
0 siblings, 1 reply; 9+ messages in thread
From: Lukas Bulwahn @ 2020-12-08 16:54 UTC (permalink / raw)
To: Aditya; +Cc: linux-kernel-mentees
On Tue, Dec 8, 2020 at 5:44 PM Aditya <yashsri421@gmail.com> wrote:
>
> On 3/12/20 12:06 am, Aditya Srivastava wrote:
> > Currently checkpatch warns us if the Co-developed-by line for an author
> > isn't immediately followed by the Signed-off-by line.
> >
> > Generally, this warning occurs because of:
> > 1) Absence of Signed-off-by line for the author from the patch
> > 2) Misplaced Signed-off-by line for the author in the patch
> >
> > Provide a simple fix by:
> > 1) Inserting Signed-off-by line for the author immediately after the
> > Co-developed-by line
> >
> > 2) Removing Signed-off-by line for the author (if it pre-existed on
> > some other line)
> >
> > Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> > ---
> > scripts/checkpatch.pl | 28 ++++++++++++++++++++++++----
> > 1 file changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 5c9f13a97c12..d8f9d4d8b13d 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -2509,6 +2509,9 @@ sub process {
> > my @setup_docs = ();
> > my $setup_docs = 0;
> >
> > + my %signed_off_by_lines;
> > + my %co_developed_by_lines;
> > +
> > my $camelcase_file_seeded = 0;
> >
> > my $checklicenseline = 1;
> > @@ -2794,6 +2797,7 @@ sub process {
> > my $ctx = $1;
> > my ($email_name, $email_comment, $email_address, $comment1) = parse_email($ctx);
> > my ($author_name, $author_comment, $author_address, $comment2) = parse_email($author);
> > + $signed_off_by_lines{$ctx} = $fixlinenr;
> >
> > if ($email_address eq $author_address && $email_name eq $author_name) {
> > $author_sob = $ctx;
> > @@ -2989,11 +2993,17 @@ sub process {
> > }
> > }
> > if (!defined $lines[$linenr]) {
> > - WARN("BAD_SIGN_OFF",
> > - "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline);
> > + if (WARN("BAD_SIGN_OFF",
> > + "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline) &&
> > + $fix) {
> > + $co_developed_by_lines{$email} = $fixlinenr;
> > + }
> > } elsif ($rawlines[$linenr] !~ /^\s*signed-off-by:\s*(.*)/i) {
> > - WARN("BAD_SIGN_OFF",
> > - "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
> > + if (WARN("BAD_SIGN_OFF",
> > + "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]) &&
> > + $fix) {
> > + $co_developed_by_lines{$email} = $fixlinenr;
> > + }
> > } elsif ($1 ne $email) {
> > WARN("BAD_SIGN_OFF",
> > "Co-developed-by and Signed-off-by: name/email do not match \n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
> > @@ -7203,6 +7213,16 @@ sub process {
> > ERROR("NOT_UNIFIED_DIFF",
> > "Does not appear to be a unified-diff format patch\n");
> > }
> > +
> > + if (%co_developed_by_lines) {
> > + foreach my $co_developer (keys %co_developed_by_lines) {
> > + if (exists($signed_off_by_lines{$co_developer})) {
> > + fix_delete_line($signed_off_by_lines{$co_developer}, "Signed-off-by: $co_developer");
> > + }
> > + fix_insert_line($co_developed_by_lines{$co_developer} + 1, "Signed-off-by: $co_developer");
> > + }
> > + }
> > +
> > if ($is_patch && $has_commit_log && $chk_signoff) {
> > if ($signoff == 0) {
> > ERROR("MISSING_SIGN_OFF",
> >
>
Not a big fan of that. Especially because adding and removing
Signed-off-by: from a trail has a specific meaning.
So that fix can only be applied under very specific conditions. You
cannot just sign a document in the name of another person, that is
clearly fraud and identity theft.
As a patch author, you would need to really carefully think if this
fix is valid or not. Do we have examples of patches where this is
wrong and where your fix can actually be run by a specific person?
Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix option for BAD_SIGN_OFF
2020-12-08 16:54 ` Lukas Bulwahn
@ 2020-12-08 17:44 ` Aditya
2020-12-08 17:52 ` Aditya Srivastava
2020-12-08 21:07 ` Lukas Bulwahn
0 siblings, 2 replies; 9+ messages in thread
From: Aditya @ 2020-12-08 17:44 UTC (permalink / raw)
To: Lukas Bulwahn; +Cc: linux-kernel-mentees
On 8/12/20 10:24 pm, Lukas Bulwahn wrote:
> On Tue, Dec 8, 2020 at 5:44 PM Aditya <yashsri421@gmail.com> wrote:
>>
>> On 3/12/20 12:06 am, Aditya Srivastava wrote:
>>> Currently checkpatch warns us if the Co-developed-by line for an author
>>> isn't immediately followed by the Signed-off-by line.
>>>
>>> Generally, this warning occurs because of:
>>> 1) Absence of Signed-off-by line for the author from the patch
>>> 2) Misplaced Signed-off-by line for the author in the patch
>>>
>>> Provide a simple fix by:
>>> 1) Inserting Signed-off-by line for the author immediately after the
>>> Co-developed-by line
>>>
>>> 2) Removing Signed-off-by line for the author (if it pre-existed on
>>> some other line)
>>>
>>> Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
>>> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
>>> ---
>>> scripts/checkpatch.pl | 28 ++++++++++++++++++++++++----
>>> 1 file changed, 24 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>> index 5c9f13a97c12..d8f9d4d8b13d 100755
>>> --- a/scripts/checkpatch.pl
>>> +++ b/scripts/checkpatch.pl
>>> @@ -2509,6 +2509,9 @@ sub process {
>>> my @setup_docs = ();
>>> my $setup_docs = 0;
>>>
>>> + my %signed_off_by_lines;
>>> + my %co_developed_by_lines;
>>> +
>>> my $camelcase_file_seeded = 0;
>>>
>>> my $checklicenseline = 1;
>>> @@ -2794,6 +2797,7 @@ sub process {
>>> my $ctx = $1;
>>> my ($email_name, $email_comment, $email_address, $comment1) = parse_email($ctx);
>>> my ($author_name, $author_comment, $author_address, $comment2) = parse_email($author);
>>> + $signed_off_by_lines{$ctx} = $fixlinenr;
>>>
>>> if ($email_address eq $author_address && $email_name eq $author_name) {
>>> $author_sob = $ctx;
>>> @@ -2989,11 +2993,17 @@ sub process {
>>> }
>>> }
>>> if (!defined $lines[$linenr]) {
>>> - WARN("BAD_SIGN_OFF",
>>> - "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline);
>>> + if (WARN("BAD_SIGN_OFF",
>>> + "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline) &&
>>> + $fix) {
>>> + $co_developed_by_lines{$email} = $fixlinenr;
>>> + }
>>> } elsif ($rawlines[$linenr] !~ /^\s*signed-off-by:\s*(.*)/i) {
>>> - WARN("BAD_SIGN_OFF",
>>> - "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
>>> + if (WARN("BAD_SIGN_OFF",
>>> + "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]) &&
>>> + $fix) {
>>> + $co_developed_by_lines{$email} = $fixlinenr;
>>> + }
>>> } elsif ($1 ne $email) {
>>> WARN("BAD_SIGN_OFF",
>>> "Co-developed-by and Signed-off-by: name/email do not match \n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
>>> @@ -7203,6 +7213,16 @@ sub process {
>>> ERROR("NOT_UNIFIED_DIFF",
>>> "Does not appear to be a unified-diff format patch\n");
>>> }
>>> +
>>> + if (%co_developed_by_lines) {
>>> + foreach my $co_developer (keys %co_developed_by_lines) {
>>> + if (exists($signed_off_by_lines{$co_developer})) {
>>> + fix_delete_line($signed_off_by_lines{$co_developer}, "Signed-off-by: $co_developer");
>>> + }
>>> + fix_insert_line($co_developed_by_lines{$co_developer} + 1, "Signed-off-by: $co_developer");
>>> + }
>>> + }
>>> +
>>> if ($is_patch && $has_commit_log && $chk_signoff) {
>>> if ($signoff == 0) {
>>> ERROR("MISSING_SIGN_OFF",
>>>
>>
>
> Not a big fan of that. Especially because adding and removing
> Signed-off-by: from a trail has a specific meaning.
>
> So that fix can only be applied under very specific conditions. You
> cannot just sign a document in the name of another person, that is
> clearly fraud and identity theft.
>
Hi Lukas
It is actually mentioned in the kernel docs that if a user is signing
off as a co-developer, they should also use Signed-off-by line
followed by their co-developed-by tag.
Link:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html
Please note this line: "Since Co-developed-by: denotes authorship,
every Co-developed-by: must be immediately followed by a
Signed-off-by: of the associated co-author."
(Please note the usage of "must" in the above quoted sentence.)
So, what we are doing here is just the same, ie if a user is signing
off as a Co-developer, he should also include his Signed-off-by line. ie,
1) If he has missed to add it, we will add it.
2) However, if he has added it, but in some other line than after his
Co-developed-by line, we are placing it in the preferred line, so we
aren't actually deleting his sign-off.
> As a patch author, you would need to really carefully think if this
> fix is valid or not. Do we have examples of patches where this is
> wrong and where your fix can actually be run by a specific person?
>
Yes, there are 129 cases of this warning over v4.13..v5.8.
I have tried this fix over few patches where it gave desired result.
For eg,
E.g. 1) for Commit 951531691c4b ("mm/usercopy: use memory range to be
accessed for wraparound check"), the signed-off-by line is misplaced,
and is fixed using this fix option.
ie.
Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
Co-developed-by: Prasad Sodagudi <psodagud@codeaurora.org>
Reviewed-by: William Kucharski <william.kucharski@oracle.com>
becomes:
Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
Co-developed-by: Prasad Sodagudi <psodagud@codeaurora.org>
Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
Reviewed-by: William Kucharski <william.kucharski@oracle.com>
Please note that "Signed-off-by: Prasad Sodagudi
<psodagud@codeaurora.org>" line just gets moved after their
corresponding Co-developed-by line.
E.g. 2) for Commit 31d851407f90 ("cpuidle: haltpoll: Take 'idle='
override into account"), the Signed-off-by line is missing, ie does
not exist for the author, so it is added.
ie,
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Co-developed-by: Joao Martins <joao.m.martins@oracle.com>
[ rjw: Subject ]
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
becomes:
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Co-developed-by: Joao Martins <joao.m.martins@oracle.com>
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
[ rjw: Subject ]
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
which I feel is the legitimate correction, as the doc states that
Co-developed-by tag denotes authorship, so the co-developer should
also sign-off.
It is maybe similar to any patch, where user has to take
responsibility of their changes by adding their Signed-off-by line,
otherwise the patches aren't accepted.
What do you think?
This patch has some conflicts with the recent next branch. I'll send
modified rebased patch.
Thanks
Aditya
> Lukas
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Linux-kernel-mentees] [PATCH] checkpatch: add fix option for BAD_SIGN_OFF
2020-12-08 17:44 ` Aditya
@ 2020-12-08 17:52 ` Aditya Srivastava
2020-12-08 21:07 ` Lukas Bulwahn
1 sibling, 0 replies; 9+ messages in thread
From: Aditya Srivastava @ 2020-12-08 17:52 UTC (permalink / raw)
To: lukas.bulwahn; +Cc: linux-kernel-mentees, yashsri421
Currently checkpatch warns us if the Co-developed-by line for an author
isn't immediately followed by the Signed-off-by line.
Generally, this warning occurs because of:
1) Absence of Signed-off-by line for the author from the patch
2) Misplaced Signed-off-by line for the author in the patch
Provide a simple fix by:
1) Inserting Signed-off-by line for the author immediately after the
Co-developed-by line
2) Removing Signed-off-by line for the author (if it pre-existed on
some other line)
Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
---
scripts/checkpatch.pl | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index ca5bb5a3f8f6..8bd8e4518f7f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2496,6 +2496,9 @@ sub process {
my @setup_docs = ();
my $setup_docs = 0;
+ my %signed_off_by_lines;
+ my %co_developed_by_lines;
+
my $camelcase_file_seeded = 0;
my $checklicenseline = 1;
@@ -2781,6 +2784,7 @@ sub process {
my $ctx = $1;
my ($email_name, $email_comment, $email_address, $comment1) = parse_email($ctx);
my ($author_name, $author_comment, $author_address, $comment2) = parse_email($author);
+ $signed_off_by_lines{$ctx} = $fixlinenr;
if ($email_address eq $author_address && $email_name eq $author_name) {
$author_sob = $ctx;
@@ -2972,11 +2976,17 @@ sub process {
"Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . "$here\n" . $rawline);
}
if (!defined $lines[$linenr]) {
- WARN("BAD_SIGN_OFF",
- "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline);
+ if (WARN("BAD_SIGN_OFF",
+ "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline) &&
+ $fix) {
+ $co_developed_by_lines{$email} = $fixlinenr;
+ }
} elsif ($rawlines[$linenr] !~ /^\s*signed-off-by:\s*(.*)/i) {
- WARN("BAD_SIGN_OFF",
- "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
+ if (WARN("BAD_SIGN_OFF",
+ "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]) &&
+ $fix) {
+ $co_developed_by_lines{$email} = $fixlinenr;
+ }
} elsif ($1 ne $email) {
WARN("BAD_SIGN_OFF",
"Co-developed-by and Signed-off-by: name/email do not match \n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
@@ -7238,6 +7248,16 @@ sub process {
ERROR("NOT_UNIFIED_DIFF",
"Does not appear to be a unified-diff format patch\n");
}
+
+ if (%co_developed_by_lines) {
+ foreach my $co_developer (keys %co_developed_by_lines) {
+ if (exists($signed_off_by_lines{$co_developer})) {
+ fix_delete_line($signed_off_by_lines{$co_developer}, "Signed-off-by: $co_developer");
+ }
+ fix_insert_line($co_developed_by_lines{$co_developer} + 1, "Signed-off-by: $co_developer");
+ }
+ }
+
if ($is_patch && $has_commit_log && $chk_signoff) {
if ($signoff == 0) {
ERROR("MISSING_SIGN_OFF",
--
2.17.1
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix option for BAD_SIGN_OFF
2020-12-08 17:44 ` Aditya
2020-12-08 17:52 ` Aditya Srivastava
@ 2020-12-08 21:07 ` Lukas Bulwahn
2020-12-09 11:46 ` Aditya
1 sibling, 1 reply; 9+ messages in thread
From: Lukas Bulwahn @ 2020-12-08 21:07 UTC (permalink / raw)
To: Aditya; +Cc: linux-kernel-mentees
On Tue, Dec 8, 2020 at 6:44 PM Aditya <yashsri421@gmail.com> wrote:
>
> On 8/12/20 10:24 pm, Lukas Bulwahn wrote:
> > On Tue, Dec 8, 2020 at 5:44 PM Aditya <yashsri421@gmail.com> wrote:
> >>
> >> On 3/12/20 12:06 am, Aditya Srivastava wrote:
> >>> Currently checkpatch warns us if the Co-developed-by line for an author
> >>> isn't immediately followed by the Signed-off-by line.
> >>>
> >>> Generally, this warning occurs because of:
> >>> 1) Absence of Signed-off-by line for the author from the patch
> >>> 2) Misplaced Signed-off-by line for the author in the patch
> >>>
> >>> Provide a simple fix by:
> >>> 1) Inserting Signed-off-by line for the author immediately after the
> >>> Co-developed-by line
> >>>
> >>> 2) Removing Signed-off-by line for the author (if it pre-existed on
> >>> some other line)
> >>>
> >>> Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> >>> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> >>> ---
> >>> scripts/checkpatch.pl | 28 ++++++++++++++++++++++++----
> >>> 1 file changed, 24 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> >>> index 5c9f13a97c12..d8f9d4d8b13d 100755
> >>> --- a/scripts/checkpatch.pl
> >>> +++ b/scripts/checkpatch.pl
> >>> @@ -2509,6 +2509,9 @@ sub process {
> >>> my @setup_docs = ();
> >>> my $setup_docs = 0;
> >>>
> >>> + my %signed_off_by_lines;
> >>> + my %co_developed_by_lines;
> >>> +
> >>> my $camelcase_file_seeded = 0;
> >>>
> >>> my $checklicenseline = 1;
> >>> @@ -2794,6 +2797,7 @@ sub process {
> >>> my $ctx = $1;
> >>> my ($email_name, $email_comment, $email_address, $comment1) = parse_email($ctx);
> >>> my ($author_name, $author_comment, $author_address, $comment2) = parse_email($author);
> >>> + $signed_off_by_lines{$ctx} = $fixlinenr;
> >>>
> >>> if ($email_address eq $author_address && $email_name eq $author_name) {
> >>> $author_sob = $ctx;
> >>> @@ -2989,11 +2993,17 @@ sub process {
> >>> }
> >>> }
> >>> if (!defined $lines[$linenr]) {
> >>> - WARN("BAD_SIGN_OFF",
> >>> - "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline);
> >>> + if (WARN("BAD_SIGN_OFF",
> >>> + "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline) &&
> >>> + $fix) {
> >>> + $co_developed_by_lines{$email} = $fixlinenr;
> >>> + }
> >>> } elsif ($rawlines[$linenr] !~ /^\s*signed-off-by:\s*(.*)/i) {
> >>> - WARN("BAD_SIGN_OFF",
> >>> - "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
> >>> + if (WARN("BAD_SIGN_OFF",
> >>> + "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]) &&
> >>> + $fix) {
> >>> + $co_developed_by_lines{$email} = $fixlinenr;
> >>> + }
> >>> } elsif ($1 ne $email) {
> >>> WARN("BAD_SIGN_OFF",
> >>> "Co-developed-by and Signed-off-by: name/email do not match \n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
> >>> @@ -7203,6 +7213,16 @@ sub process {
> >>> ERROR("NOT_UNIFIED_DIFF",
> >>> "Does not appear to be a unified-diff format patch\n");
> >>> }
> >>> +
> >>> + if (%co_developed_by_lines) {
> >>> + foreach my $co_developer (keys %co_developed_by_lines) {
> >>> + if (exists($signed_off_by_lines{$co_developer})) {
> >>> + fix_delete_line($signed_off_by_lines{$co_developer}, "Signed-off-by: $co_developer");
> >>> + }
> >>> + fix_insert_line($co_developed_by_lines{$co_developer} + 1, "Signed-off-by: $co_developer");
> >>> + }
> >>> + }
> >>> +
> >>> if ($is_patch && $has_commit_log && $chk_signoff) {
> >>> if ($signoff == 0) {
> >>> ERROR("MISSING_SIGN_OFF",
> >>>
> >>
> >
> > Not a big fan of that. Especially because adding and removing
> > Signed-off-by: from a trail has a specific meaning.
> >
> > So that fix can only be applied under very specific conditions. You
> > cannot just sign a document in the name of another person, that is
> > clearly fraud and identity theft.
> >
>
> Hi Lukas
> It is actually mentioned in the kernel docs that if a user is signing
> off as a co-developer, they should also use Signed-off-by line
> followed by their co-developed-by tag.
> Link:
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
>
> Please note this line: "Since Co-developed-by: denotes authorship,
> every Co-developed-by: must be immediately followed by a
> Signed-off-by: of the associated co-author."
>
> (Please note the usage of "must" in the above quoted sentence.)
>
> So, what we are doing here is just the same, ie if a user is signing
> off as a Co-developer, he should also include his Signed-off-by line. ie,
>
> 1) If he has missed to add it, we will add it.
Only the person with that identity is allowed to do that. Nobody else.
How do you know who is running this script?
Nobody is allowed to add a Sign-off-by: for another person.
The documentation should clearly state that as well.
> 2) However, if he has added it, but in some other line than after his
> Co-developed-by line, we are placing it in the preferred line, so we
> aren't actually deleting his sign-off.
>
That might be legitimate, but the order of Signed-off-by: has a
meaning itself, so maybe you are breaking that then as well.
>
> > As a patch author, you would need to really carefully think if this
> > fix is valid or not. Do we have examples of patches where this is
> > wrong and where your fix can actually be run by a specific person?
> >
>
> Yes, there are 129 cases of this warning over v4.13..v5.8.
>
> I have tried this fix over few patches where it gave desired result.
> For eg,
>
> E.g. 1) for Commit 951531691c4b ("mm/usercopy: use memory range to be
> accessed for wraparound check"), the signed-off-by line is misplaced,
> and is fixed using this fix option.
> ie.
> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> Co-developed-by: Prasad Sodagudi <psodagud@codeaurora.org>
> Reviewed-by: William Kucharski <william.kucharski@oracle.com>
>
> becomes:
> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> Co-developed-by: Prasad Sodagudi <psodagud@codeaurora.org>
> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
> Reviewed-by: William Kucharski <william.kucharski@oracle.com>
>
> Please note that "Signed-off-by: Prasad Sodagudi
> <psodagud@codeaurora.org>" line just gets moved after their
> corresponding Co-developed-by line.
>
Reordering might make sense.
>
> E.g. 2) for Commit 31d851407f90 ("cpuidle: haltpoll: Take 'idle='
> override into account"), the Signed-off-by line is missing, ie does
> not exist for the author, so it is added.
>
> ie,
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> Co-developed-by: Joao Martins <joao.m.martins@oracle.com>
> [ rjw: Subject ]
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> becomes:
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> Co-developed-by: Joao Martins <joao.m.martins@oracle.com>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> [ rjw: Subject ]
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> which I feel is the legitimate correction, as the doc states that
> Co-developed-by tag denotes authorship, so the co-developer should
> also sign-off.
> It is maybe similar to any patch, where user has to take
> responsibility of their changes by adding their Signed-off-by line,
> otherwise the patches aren't accepted.
>
Yes, but how do you check that only Joao Martins runs this script and
nobody else?
That is why this feature is ultimately broken, and I would be very
careful if someone really just said I ran checkpatch --fix for that.
> What do you think?
>
> This patch has some conflicts with the recent next branch. I'll send
> modified rebased patch.
>
> Thanks
> Aditya
>
>
>
> > Lukas
> >
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix option for BAD_SIGN_OFF
2020-12-08 21:07 ` Lukas Bulwahn
@ 2020-12-09 11:46 ` Aditya
2020-12-09 11:51 ` Lukas Bulwahn
0 siblings, 1 reply; 9+ messages in thread
From: Aditya @ 2020-12-09 11:46 UTC (permalink / raw)
To: Lukas Bulwahn; +Cc: linux-kernel-mentees
On 9/12/20 2:37 am, Lukas Bulwahn wrote:
> On Tue, Dec 8, 2020 at 6:44 PM Aditya <yashsri421@gmail.com> wrote:
>>
>> On 8/12/20 10:24 pm, Lukas Bulwahn wrote:
>>> On Tue, Dec 8, 2020 at 5:44 PM Aditya <yashsri421@gmail.com> wrote:
>>>>
>>>> On 3/12/20 12:06 am, Aditya Srivastava wrote:
>>>>> Currently checkpatch warns us if the Co-developed-by line for an author
>>>>> isn't immediately followed by the Signed-off-by line.
>>>>>
>>>>> Generally, this warning occurs because of:
>>>>> 1) Absence of Signed-off-by line for the author from the patch
>>>>> 2) Misplaced Signed-off-by line for the author in the patch
>>>>>
>>>>> Provide a simple fix by:
>>>>> 1) Inserting Signed-off-by line for the author immediately after the
>>>>> Co-developed-by line
>>>>>
>>>>> 2) Removing Signed-off-by line for the author (if it pre-existed on
>>>>> some other line)
>>>>>
>>>>> Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
>>>>> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
>>>>> ---
>>>>> scripts/checkpatch.pl | 28 ++++++++++++++++++++++++----
>>>>> 1 file changed, 24 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>>>> index 5c9f13a97c12..d8f9d4d8b13d 100755
>>>>> --- a/scripts/checkpatch.pl
>>>>> +++ b/scripts/checkpatch.pl
>>>>> @@ -2509,6 +2509,9 @@ sub process {
>>>>> my @setup_docs = ();
>>>>> my $setup_docs = 0;
>>>>>
>>>>> + my %signed_off_by_lines;
>>>>> + my %co_developed_by_lines;
>>>>> +
>>>>> my $camelcase_file_seeded = 0;
>>>>>
>>>>> my $checklicenseline = 1;
>>>>> @@ -2794,6 +2797,7 @@ sub process {
>>>>> my $ctx = $1;
>>>>> my ($email_name, $email_comment, $email_address, $comment1) = parse_email($ctx);
>>>>> my ($author_name, $author_comment, $author_address, $comment2) = parse_email($author);
>>>>> + $signed_off_by_lines{$ctx} = $fixlinenr;
>>>>>
>>>>> if ($email_address eq $author_address && $email_name eq $author_name) {
>>>>> $author_sob = $ctx;
>>>>> @@ -2989,11 +2993,17 @@ sub process {
>>>>> }
>>>>> }
>>>>> if (!defined $lines[$linenr]) {
>>>>> - WARN("BAD_SIGN_OFF",
>>>>> - "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline);
>>>>> + if (WARN("BAD_SIGN_OFF",
>>>>> + "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline) &&
>>>>> + $fix) {
>>>>> + $co_developed_by_lines{$email} = $fixlinenr;
>>>>> + }
>>>>> } elsif ($rawlines[$linenr] !~ /^\s*signed-off-by:\s*(.*)/i) {
>>>>> - WARN("BAD_SIGN_OFF",
>>>>> - "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
>>>>> + if (WARN("BAD_SIGN_OFF",
>>>>> + "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]) &&
>>>>> + $fix) {
>>>>> + $co_developed_by_lines{$email} = $fixlinenr;
>>>>> + }
>>>>> } elsif ($1 ne $email) {
>>>>> WARN("BAD_SIGN_OFF",
>>>>> "Co-developed-by and Signed-off-by: name/email do not match \n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
>>>>> @@ -7203,6 +7213,16 @@ sub process {
>>>>> ERROR("NOT_UNIFIED_DIFF",
>>>>> "Does not appear to be a unified-diff format patch\n");
>>>>> }
>>>>> +
>>>>> + if (%co_developed_by_lines) {
>>>>> + foreach my $co_developer (keys %co_developed_by_lines) {
>>>>> + if (exists($signed_off_by_lines{$co_developer})) {
>>>>> + fix_delete_line($signed_off_by_lines{$co_developer}, "Signed-off-by: $co_developer");
>>>>> + }
>>>>> + fix_insert_line($co_developed_by_lines{$co_developer} + 1, "Signed-off-by: $co_developer");
>>>>> + }
>>>>> + }
>>>>> +
>>>>> if ($is_patch && $has_commit_log && $chk_signoff) {
>>>>> if ($signoff == 0) {
>>>>> ERROR("MISSING_SIGN_OFF",
>>>>>
>>>>
>>>
>>> Not a big fan of that. Especially because adding and removing
>>> Signed-off-by: from a trail has a specific meaning.
>>>
>>> So that fix can only be applied under very specific conditions. You
>>> cannot just sign a document in the name of another person, that is
>>> clearly fraud and identity theft.
>>>
>>
>> Hi Lukas
>> It is actually mentioned in the kernel docs that if a user is signing
>> off as a co-developer, they should also use Signed-off-by line
>> followed by their co-developed-by tag.
>> Link:
>> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
>>
>> Please note this line: "Since Co-developed-by: denotes authorship,
>> every Co-developed-by: must be immediately followed by a
>> Signed-off-by: of the associated co-author."
>>
>> (Please note the usage of "must" in the above quoted sentence.)
>>
>> So, what we are doing here is just the same, ie if a user is signing
>> off as a Co-developer, he should also include his Signed-off-by line. ie,
>>
>> 1) If he has missed to add it, we will add it.
>
> Only the person with that identity is allowed to do that. Nobody else.
>
Okay.
> How do you know who is running this script?
>
We can probably check it using git config to get user's signature and
match it against the signature we are going to add. Only if they
match, we add it.
But again the user may not be aware that his sign-off got added by
running the script.
I suggest, we can probably give them a MISSING_SIGN_OFF warning for
it, and if they choose to fix that, we can probably add the line. What
do you think?
> Nobody is allowed to add a Sign-off-by: for another person.
> The documentation should clearly state that as well.
>
>> 2) However, if he has added it, but in some other line than after his
>> Co-developed-by line, we are placing it in the preferred line, so we
>> aren't actually deleting his sign-off.
>>
>
> That might be legitimate, but the order of Signed-off-by: has a
> meaning itself, so maybe you are breaking that then as well.
>
>>
>>> As a patch author, you would need to really carefully think if this
>>> fix is valid or not. Do we have examples of patches where this is
>>> wrong and where your fix can actually be run by a specific person?
>>>
>>
>> Yes, there are 129 cases of this warning over v4.13..v5.8.
>>
>> I have tried this fix over few patches where it gave desired result.
>> For eg,
>>
>> E.g. 1) for Commit 951531691c4b ("mm/usercopy: use memory range to be
>> accessed for wraparound check"), the signed-off-by line is misplaced,
>> and is fixed using this fix option.
>> ie.
>> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
>> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
>> Co-developed-by: Prasad Sodagudi <psodagud@codeaurora.org>
>> Reviewed-by: William Kucharski <william.kucharski@oracle.com>
>>
>> becomes:
>> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
>> Co-developed-by: Prasad Sodagudi <psodagud@codeaurora.org>
>> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
>> Reviewed-by: William Kucharski <william.kucharski@oracle.com>
>>
>> Please note that "Signed-off-by: Prasad Sodagudi
>> <psodagud@codeaurora.org>" line just gets moved after their
>> corresponding Co-developed-by line.
>>
>
> Reordering might make sense.
>
>>
>> E.g. 2) for Commit 31d851407f90 ("cpuidle: haltpoll: Take 'idle='
>> override into account"), the Signed-off-by line is missing, ie does
>> not exist for the author, so it is added.
>>
>> ie,
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
>> Co-developed-by: Joao Martins <joao.m.martins@oracle.com>
>> [ rjw: Subject ]
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> becomes:
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
>> Co-developed-by: Joao Martins <joao.m.martins@oracle.com>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> [ rjw: Subject ]
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> which I feel is the legitimate correction, as the doc states that
>> Co-developed-by tag denotes authorship, so the co-developer should
>> also sign-off.
>> It is maybe similar to any patch, where user has to take
>> responsibility of their changes by adding their Signed-off-by line,
>> otherwise the patches aren't accepted.
>>
>
> Yes, but how do you check that only Joao Martins runs this script and
> nobody else?
>
> That is why this feature is ultimately broken, and I would be very
> careful if someone really just said I ran checkpatch --fix for that.
>
I think for this fix, we should limit to reordering sign-off after
their corresponding Co-developed-by: tag and not adding new sign-off.
Although we can check if the same user is running checkpatch, by using
git-config, but he may still be unaware that his sign-off gets added
by running the script.
What do you think?
Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix option for BAD_SIGN_OFF
2020-12-09 11:46 ` Aditya
@ 2020-12-09 11:51 ` Lukas Bulwahn
2020-12-12 10:55 ` Aditya
0 siblings, 1 reply; 9+ messages in thread
From: Lukas Bulwahn @ 2020-12-09 11:51 UTC (permalink / raw)
To: Aditya; +Cc: linux-kernel-mentees
On Wed, Dec 9, 2020 at 12:46 PM Aditya <yashsri421@gmail.com> wrote:
>
> On 9/12/20 2:37 am, Lukas Bulwahn wrote:
> > On Tue, Dec 8, 2020 at 6:44 PM Aditya <yashsri421@gmail.com> wrote:
> >>
> >> On 8/12/20 10:24 pm, Lukas Bulwahn wrote:
> >>> On Tue, Dec 8, 2020 at 5:44 PM Aditya <yashsri421@gmail.com> wrote:
> >>>>
> >>>> On 3/12/20 12:06 am, Aditya Srivastava wrote:
> >>>>> Currently checkpatch warns us if the Co-developed-by line for an author
> >>>>> isn't immediately followed by the Signed-off-by line.
> >>>>>
> >>>>> Generally, this warning occurs because of:
> >>>>> 1) Absence of Signed-off-by line for the author from the patch
> >>>>> 2) Misplaced Signed-off-by line for the author in the patch
> >>>>>
> >>>>> Provide a simple fix by:
> >>>>> 1) Inserting Signed-off-by line for the author immediately after the
> >>>>> Co-developed-by line
> >>>>>
> >>>>> 2) Removing Signed-off-by line for the author (if it pre-existed on
> >>>>> some other line)
> >>>>>
> >>>>> Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> >>>>> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> >>>>> ---
> >>>>> scripts/checkpatch.pl | 28 ++++++++++++++++++++++++----
> >>>>> 1 file changed, 24 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> >>>>> index 5c9f13a97c12..d8f9d4d8b13d 100755
> >>>>> --- a/scripts/checkpatch.pl
> >>>>> +++ b/scripts/checkpatch.pl
> >>>>> @@ -2509,6 +2509,9 @@ sub process {
> >>>>> my @setup_docs = ();
> >>>>> my $setup_docs = 0;
> >>>>>
> >>>>> + my %signed_off_by_lines;
> >>>>> + my %co_developed_by_lines;
> >>>>> +
> >>>>> my $camelcase_file_seeded = 0;
> >>>>>
> >>>>> my $checklicenseline = 1;
> >>>>> @@ -2794,6 +2797,7 @@ sub process {
> >>>>> my $ctx = $1;
> >>>>> my ($email_name, $email_comment, $email_address, $comment1) = parse_email($ctx);
> >>>>> my ($author_name, $author_comment, $author_address, $comment2) = parse_email($author);
> >>>>> + $signed_off_by_lines{$ctx} = $fixlinenr;
> >>>>>
> >>>>> if ($email_address eq $author_address && $email_name eq $author_name) {
> >>>>> $author_sob = $ctx;
> >>>>> @@ -2989,11 +2993,17 @@ sub process {
> >>>>> }
> >>>>> }
> >>>>> if (!defined $lines[$linenr]) {
> >>>>> - WARN("BAD_SIGN_OFF",
> >>>>> - "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline);
> >>>>> + if (WARN("BAD_SIGN_OFF",
> >>>>> + "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline) &&
> >>>>> + $fix) {
> >>>>> + $co_developed_by_lines{$email} = $fixlinenr;
> >>>>> + }
> >>>>> } elsif ($rawlines[$linenr] !~ /^\s*signed-off-by:\s*(.*)/i) {
> >>>>> - WARN("BAD_SIGN_OFF",
> >>>>> - "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
> >>>>> + if (WARN("BAD_SIGN_OFF",
> >>>>> + "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]) &&
> >>>>> + $fix) {
> >>>>> + $co_developed_by_lines{$email} = $fixlinenr;
> >>>>> + }
> >>>>> } elsif ($1 ne $email) {
> >>>>> WARN("BAD_SIGN_OFF",
> >>>>> "Co-developed-by and Signed-off-by: name/email do not match \n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
> >>>>> @@ -7203,6 +7213,16 @@ sub process {
> >>>>> ERROR("NOT_UNIFIED_DIFF",
> >>>>> "Does not appear to be a unified-diff format patch\n");
> >>>>> }
> >>>>> +
> >>>>> + if (%co_developed_by_lines) {
> >>>>> + foreach my $co_developer (keys %co_developed_by_lines) {
> >>>>> + if (exists($signed_off_by_lines{$co_developer})) {
> >>>>> + fix_delete_line($signed_off_by_lines{$co_developer}, "Signed-off-by: $co_developer");
> >>>>> + }
> >>>>> + fix_insert_line($co_developed_by_lines{$co_developer} + 1, "Signed-off-by: $co_developer");
> >>>>> + }
> >>>>> + }
> >>>>> +
> >>>>> if ($is_patch && $has_commit_log && $chk_signoff) {
> >>>>> if ($signoff == 0) {
> >>>>> ERROR("MISSING_SIGN_OFF",
> >>>>>
> >>>>
> >>>
> >>> Not a big fan of that. Especially because adding and removing
> >>> Signed-off-by: from a trail has a specific meaning.
> >>>
> >>> So that fix can only be applied under very specific conditions. You
> >>> cannot just sign a document in the name of another person, that is
> >>> clearly fraud and identity theft.
> >>>
> >>
> >> Hi Lukas
> >> It is actually mentioned in the kernel docs that if a user is signing
> >> off as a co-developer, they should also use Signed-off-by line
> >> followed by their co-developed-by tag.
> >> Link:
> >> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
> >>
> >> Please note this line: "Since Co-developed-by: denotes authorship,
> >> every Co-developed-by: must be immediately followed by a
> >> Signed-off-by: of the associated co-author."
> >>
> >> (Please note the usage of "must" in the above quoted sentence.)
> >>
> >> So, what we are doing here is just the same, ie if a user is signing
> >> off as a Co-developer, he should also include his Signed-off-by line. ie,
> >>
> >> 1) If he has missed to add it, we will add it.
> >
> > Only the person with that identity is allowed to do that. Nobody else.
> >
> Okay.
>
> > How do you know who is running this script?
> >
>
> We can probably check it using git config to get user's signature and
> match it against the signature we are going to add. Only if they
> match, we add it.
> But again the user may not be aware that his sign-off got added by
> running the script.
> I suggest, we can probably give them a MISSING_SIGN_OFF warning for
> it, and if they choose to fix that, we can probably add the line. What
> do you think?
>
>
> > Nobody is allowed to add a Sign-off-by: for another person.
> > The documentation should clearly state that as well.
> >
> >> 2) However, if he has added it, but in some other line than after his
> >> Co-developed-by line, we are placing it in the preferred line, so we
> >> aren't actually deleting his sign-off.
> >>
> >
> > That might be legitimate, but the order of Signed-off-by: has a
> > meaning itself, so maybe you are breaking that then as well.
> >
> >>
> >>> As a patch author, you would need to really carefully think if this
> >>> fix is valid or not. Do we have examples of patches where this is
> >>> wrong and where your fix can actually be run by a specific person?
> >>>
> >>
> >> Yes, there are 129 cases of this warning over v4.13..v5.8.
> >>
> >> I have tried this fix over few patches where it gave desired result.
> >> For eg,
> >>
> >> E.g. 1) for Commit 951531691c4b ("mm/usercopy: use memory range to be
> >> accessed for wraparound check"), the signed-off-by line is misplaced,
> >> and is fixed using this fix option.
> >> ie.
> >> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
> >> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> >> Co-developed-by: Prasad Sodagudi <psodagud@codeaurora.org>
> >> Reviewed-by: William Kucharski <william.kucharski@oracle.com>
> >>
> >> becomes:
> >> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> >> Co-developed-by: Prasad Sodagudi <psodagud@codeaurora.org>
> >> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
> >> Reviewed-by: William Kucharski <william.kucharski@oracle.com>
> >>
> >> Please note that "Signed-off-by: Prasad Sodagudi
> >> <psodagud@codeaurora.org>" line just gets moved after their
> >> corresponding Co-developed-by line.
> >>
> >
> > Reordering might make sense.
> >
> >>
> >> E.g. 2) for Commit 31d851407f90 ("cpuidle: haltpoll: Take 'idle='
> >> override into account"), the Signed-off-by line is missing, ie does
> >> not exist for the author, so it is added.
> >>
> >> ie,
> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> >> Co-developed-by: Joao Martins <joao.m.martins@oracle.com>
> >> [ rjw: Subject ]
> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>
> >> becomes:
> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> >> Co-developed-by: Joao Martins <joao.m.martins@oracle.com>
> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> >> [ rjw: Subject ]
> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>
> >> which I feel is the legitimate correction, as the doc states that
> >> Co-developed-by tag denotes authorship, so the co-developer should
> >> also sign-off.
> >> It is maybe similar to any patch, where user has to take
> >> responsibility of their changes by adding their Signed-off-by line,
> >> otherwise the patches aren't accepted.
> >>
> >
> > Yes, but how do you check that only Joao Martins runs this script and
> > nobody else?
> >
> > That is why this feature is ultimately broken, and I would be very
> > careful if someone really just said I ran checkpatch --fix for that.
> >
>
> I think for this fix, we should limit to reordering sign-off after
> their corresponding Co-developed-by: tag and not adding new sign-off.
>
> Although we can check if the same user is running checkpatch, by using
> git-config, but he may still be unaware that his sign-off gets added
> by running the script.
>
> What do you think?
>
Well, these are all good ideas, but this is a lot of complexity for
something simple if we simply propose the fix with a clear warning on
Signed-off-by semantics and the user actively needs to acknowledge it.
Let us start with the reordering and see what Joe says. It could be
though that he will also not be really convinced.
Lukas
> Thanks
> Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: add fix option for BAD_SIGN_OFF
2020-12-09 11:51 ` Lukas Bulwahn
@ 2020-12-12 10:55 ` Aditya
0 siblings, 0 replies; 9+ messages in thread
From: Aditya @ 2020-12-12 10:55 UTC (permalink / raw)
To: Lukas Bulwahn; +Cc: linux-kernel-mentees
On 9/12/20 5:21 pm, Lukas Bulwahn wrote:
> On Wed, Dec 9, 2020 at 12:46 PM Aditya <yashsri421@gmail.com> wrote:
>>
>> On 9/12/20 2:37 am, Lukas Bulwahn wrote:
>>> On Tue, Dec 8, 2020 at 6:44 PM Aditya <yashsri421@gmail.com> wrote:
>>>>
>>>> On 8/12/20 10:24 pm, Lukas Bulwahn wrote:
>>>>> On Tue, Dec 8, 2020 at 5:44 PM Aditya <yashsri421@gmail.com> wrote:
>>>>>>
>>>>>> On 3/12/20 12:06 am, Aditya Srivastava wrote:
>>>>>>> Currently checkpatch warns us if the Co-developed-by line for an author
>>>>>>> isn't immediately followed by the Signed-off-by line.
>>>>>>>
>>>>>>> Generally, this warning occurs because of:
>>>>>>> 1) Absence of Signed-off-by line for the author from the patch
>>>>>>> 2) Misplaced Signed-off-by line for the author in the patch
>>>>>>>
>>>>>>> Provide a simple fix by:
>>>>>>> 1) Inserting Signed-off-by line for the author immediately after the
>>>>>>> Co-developed-by line
>>>>>>>
>>>>>>> 2) Removing Signed-off-by line for the author (if it pre-existed on
>>>>>>> some other line)
>>>>>>>
>>>>>>> Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
>>>>>>> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
>>>>>>> ---
>>>>>>> scripts/checkpatch.pl | 28 ++++++++++++++++++++++++----
>>>>>>> 1 file changed, 24 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>>>>>> index 5c9f13a97c12..d8f9d4d8b13d 100755
>>>>>>> --- a/scripts/checkpatch.pl
>>>>>>> +++ b/scripts/checkpatch.pl
>>>>>>> @@ -2509,6 +2509,9 @@ sub process {
>>>>>>> my @setup_docs = ();
>>>>>>> my $setup_docs = 0;
>>>>>>>
>>>>>>> + my %signed_off_by_lines;
>>>>>>> + my %co_developed_by_lines;
>>>>>>> +
>>>>>>> my $camelcase_file_seeded = 0;
>>>>>>>
>>>>>>> my $checklicenseline = 1;
>>>>>>> @@ -2794,6 +2797,7 @@ sub process {
>>>>>>> my $ctx = $1;
>>>>>>> my ($email_name, $email_comment, $email_address, $comment1) = parse_email($ctx);
>>>>>>> my ($author_name, $author_comment, $author_address, $comment2) = parse_email($author);
>>>>>>> + $signed_off_by_lines{$ctx} = $fixlinenr;
>>>>>>>
>>>>>>> if ($email_address eq $author_address && $email_name eq $author_name) {
>>>>>>> $author_sob = $ctx;
>>>>>>> @@ -2989,11 +2993,17 @@ sub process {
>>>>>>> }
>>>>>>> }
>>>>>>> if (!defined $lines[$linenr]) {
>>>>>>> - WARN("BAD_SIGN_OFF",
>>>>>>> - "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline);
>>>>>>> + if (WARN("BAD_SIGN_OFF",
>>>>>>> + "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline) &&
>>>>>>> + $fix) {
>>>>>>> + $co_developed_by_lines{$email} = $fixlinenr;
>>>>>>> + }
>>>>>>> } elsif ($rawlines[$linenr] !~ /^\s*signed-off-by:\s*(.*)/i) {
>>>>>>> - WARN("BAD_SIGN_OFF",
>>>>>>> - "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
>>>>>>> + if (WARN("BAD_SIGN_OFF",
>>>>>>> + "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]) &&
>>>>>>> + $fix) {
>>>>>>> + $co_developed_by_lines{$email} = $fixlinenr;
>>>>>>> + }
>>>>>>> } elsif ($1 ne $email) {
>>>>>>> WARN("BAD_SIGN_OFF",
>>>>>>> "Co-developed-by and Signed-off-by: name/email do not match \n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
>>>>>>> @@ -7203,6 +7213,16 @@ sub process {
>>>>>>> ERROR("NOT_UNIFIED_DIFF",
>>>>>>> "Does not appear to be a unified-diff format patch\n");
>>>>>>> }
>>>>>>> +
>>>>>>> + if (%co_developed_by_lines) {
>>>>>>> + foreach my $co_developer (keys %co_developed_by_lines) {
>>>>>>> + if (exists($signed_off_by_lines{$co_developer})) {
>>>>>>> + fix_delete_line($signed_off_by_lines{$co_developer}, "Signed-off-by: $co_developer");
>>>>>>> + }
>>>>>>> + fix_insert_line($co_developed_by_lines{$co_developer} + 1, "Signed-off-by: $co_developer");
>>>>>>> + }
>>>>>>> + }
>>>>>>> +
>>>>>>> if ($is_patch && $has_commit_log && $chk_signoff) {
>>>>>>> if ($signoff == 0) {
>>>>>>> ERROR("MISSING_SIGN_OFF",
>>>>>>>
>>>>>>
>>>>>
>>>>> Not a big fan of that. Especially because adding and removing
>>>>> Signed-off-by: from a trail has a specific meaning.
>>>>>
>>>>> So that fix can only be applied under very specific conditions. You
>>>>> cannot just sign a document in the name of another person, that is
>>>>> clearly fraud and identity theft.
>>>>>
>>>>
>>>> Hi Lukas
>>>> It is actually mentioned in the kernel docs that if a user is signing
>>>> off as a co-developer, they should also use Signed-off-by line
>>>> followed by their co-developed-by tag.
>>>> Link:
>>>> https://www.kernel.org/doc/html/latest/process/submitting-patches.html
>>>>
>>>> Please note this line: "Since Co-developed-by: denotes authorship,
>>>> every Co-developed-by: must be immediately followed by a
>>>> Signed-off-by: of the associated co-author."
>>>>
>>>> (Please note the usage of "must" in the above quoted sentence.)
>>>>
>>>> So, what we are doing here is just the same, ie if a user is signing
>>>> off as a Co-developer, he should also include his Signed-off-by line. ie,
>>>>
>>>> 1) If he has missed to add it, we will add it.
>>>
>>> Only the person with that identity is allowed to do that. Nobody else.
>>>
>> Okay.
>>
>>> How do you know who is running this script?
>>>
>>
>> We can probably check it using git config to get user's signature and
>> match it against the signature we are going to add. Only if they
>> match, we add it.
>> But again the user may not be aware that his sign-off got added by
>> running the script.
>> I suggest, we can probably give them a MISSING_SIGN_OFF warning for
>> it, and if they choose to fix that, we can probably add the line. What
>> do you think?
>>
>>
>>> Nobody is allowed to add a Sign-off-by: for another person.
>>> The documentation should clearly state that as well.
>>>
>>>> 2) However, if he has added it, but in some other line than after his
>>>> Co-developed-by line, we are placing it in the preferred line, so we
>>>> aren't actually deleting his sign-off.
>>>>
>>>
>>> That might be legitimate, but the order of Signed-off-by: has a
>>> meaning itself, so maybe you are breaking that then as well.
>>>
>>>>
>>>>> As a patch author, you would need to really carefully think if this
>>>>> fix is valid or not. Do we have examples of patches where this is
>>>>> wrong and where your fix can actually be run by a specific person?
>>>>>
>>>>
>>>> Yes, there are 129 cases of this warning over v4.13..v5.8.
>>>>
>>>> I have tried this fix over few patches where it gave desired result.
>>>> For eg,
>>>>
>>>> E.g. 1) for Commit 951531691c4b ("mm/usercopy: use memory range to be
>>>> accessed for wraparound check"), the signed-off-by line is misplaced,
>>>> and is fixed using this fix option.
>>>> ie.
>>>> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
>>>> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
>>>> Co-developed-by: Prasad Sodagudi <psodagud@codeaurora.org>
>>>> Reviewed-by: William Kucharski <william.kucharski@oracle.com>
>>>>
>>>> becomes:
>>>> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
>>>> Co-developed-by: Prasad Sodagudi <psodagud@codeaurora.org>
>>>> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
>>>> Reviewed-by: William Kucharski <william.kucharski@oracle.com>
>>>>
>>>> Please note that "Signed-off-by: Prasad Sodagudi
>>>> <psodagud@codeaurora.org>" line just gets moved after their
>>>> corresponding Co-developed-by line.
>>>>
>>>
>>> Reordering might make sense.
>>>
>>>>
>>>> E.g. 2) for Commit 31d851407f90 ("cpuidle: haltpoll: Take 'idle='
>>>> override into account"), the Signed-off-by line is missing, ie does
>>>> not exist for the author, so it is added.
>>>>
>>>> ie,
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
>>>> Co-developed-by: Joao Martins <joao.m.martins@oracle.com>
>>>> [ rjw: Subject ]
>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>
>>>> becomes:
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
>>>> Co-developed-by: Joao Martins <joao.m.martins@oracle.com>
>>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>>> [ rjw: Subject ]
>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>
>>>> which I feel is the legitimate correction, as the doc states that
>>>> Co-developed-by tag denotes authorship, so the co-developer should
>>>> also sign-off.
>>>> It is maybe similar to any patch, where user has to take
>>>> responsibility of their changes by adding their Signed-off-by line,
>>>> otherwise the patches aren't accepted.
>>>>
>>>
>>> Yes, but how do you check that only Joao Martins runs this script and
>>> nobody else?
>>>
>>> That is why this feature is ultimately broken, and I would be very
>>> careful if someone really just said I ran checkpatch --fix for that.
>>>
>>
>> I think for this fix, we should limit to reordering sign-off after
>> their corresponding Co-developed-by: tag and not adding new sign-off.
>>
>> Although we can check if the same user is running checkpatch, by using
>> git-config, but he may still be unaware that his sign-off gets added
>> by running the script.
>>
>> What do you think?
>>
>
> Well, these are all good ideas, but this is a lot of complexity for
> something simple if we simply propose the fix with a clear warning on
> Signed-off-by semantics and the user actively needs to acknowledge it.
>
> Let us start with the reordering and see what Joe says. It could be
> though that he will also not be really convinced.
>
>
Hi Lukas
This correction does not work for multiple lines of addition and removal.
I tried traversing the hash in sorted order of line-number which works
perfectly when the signed-off-by and co-developed-by are in the same
order, ie
Signed-off-by: XYZ<foo@xyz.com>
Signed-off-by: JKL<foo@jkl.com>
Co-developed-by: XYZ<foo@xyz.com>
Co-developed-by: JKL<foo@jkl.com>
(XYZ before JKL in both Signed-off-by and Co-developed-by)
But again, it does not work as expected when they are jumbled-up.
I guess I'll have to update the line numbers after each insertion and
removal to get the desired result, but again it seems like a rather
complex mechanism.
What do you think? Should I proceed with this fix? Is there any
simpler mechanism perhaps?
Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-12-12 10:55 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-02 18:36 [Linux-kernel-mentees] [PATCH] checkpatch: add fix option for BAD_SIGN_OFF Aditya Srivastava
2020-12-08 16:44 ` Aditya
2020-12-08 16:54 ` Lukas Bulwahn
2020-12-08 17:44 ` Aditya
2020-12-08 17:52 ` Aditya Srivastava
2020-12-08 21:07 ` Lukas Bulwahn
2020-12-09 11:46 ` Aditya
2020-12-09 11:51 ` Lukas Bulwahn
2020-12-12 10:55 ` Aditya
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).