[Linux-kernel-mentees] [PATCH] checkpatch: add fix option for BAD_SIGN_OFF
Aditya
yashsri421 at gmail.com
Tue Dec 8 17:44:25 UTC 2020
On 8/12/20 10:24 pm, Lukas Bulwahn wrote:
> On Tue, Dec 8, 2020 at 5:44 PM Aditya <yashsri421 at 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 at gmail.com>
>>> Signed-off-by: Aditya Srivastava <yashsri421 at 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 at codeaurora.org>
Signed-off-by: Isaac J. Manjarres <isaacm at codeaurora.org>
Co-developed-by: Prasad Sodagudi <psodagud at codeaurora.org>
Reviewed-by: William Kucharski <william.kucharski at oracle.com>
becomes:
Signed-off-by: Isaac J. Manjarres <isaacm at codeaurora.org>
Co-developed-by: Prasad Sodagudi <psodagud at codeaurora.org>
Signed-off-by: Prasad Sodagudi <psodagud at codeaurora.org>
Reviewed-by: William Kucharski <william.kucharski at oracle.com>
Please note that "Signed-off-by: Prasad Sodagudi
<psodagud at 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 at oracle.com>
Co-developed-by: Joao Martins <joao.m.martins at oracle.com>
[ rjw: Subject ]
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki at intel.com>
becomes:
Signed-off-by: Zhenzhong Duan <zhenzhong.duan at oracle.com>
Co-developed-by: Joao Martins <joao.m.martins at oracle.com>
Signed-off-by: Joao Martins <joao.m.martins at oracle.com>
[ rjw: Subject ]
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki at 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
>
More information about the Linux-kernel-mentees
mailing list