* [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).