[Linux-kernel-mentees] [PATCH] checkpatch: add fix option for BAD_SIGN_OFF
Aditya
yashsri421 at gmail.com
Wed Dec 9 11:46:40 UTC 2020
On 9/12/20 2:37 am, Lukas Bulwahn wrote:
> On Tue, Dec 8, 2020 at 6:44 PM Aditya <yashsri421 at gmail.com> wrote:
>>
>> 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.
>
> 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 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.
>>
>
> 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 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.
>>
>
> 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
More information about the Linux-kernel-mentees
mailing list