[Linux-kernel-mentees] [PATCH] checkpatch: add fix option for BAD_SIGN_OFF
Aditya
yashsri421 at gmail.com
Sat Dec 12 10:55:30 UTC 2020
On 9/12/20 5:21 pm, Lukas Bulwahn wrote:
> On Wed, Dec 9, 2020 at 12:46 PM Aditya <yashsri421 at gmail.com> wrote:
>>
>> 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?
>>
>
> 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 at xyz.com>
Signed-off-by: JKL<foo at jkl.com>
Co-developed-by: XYZ<foo at xyz.com>
Co-developed-by: JKL<foo at 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
More information about the Linux-kernel-mentees
mailing list