[Linux-kernel-mentees] [PATCH] checkpatch: add fixes for BAD_SIGN_OFF

Aditya yashsri421 at gmail.com
Wed Nov 4 09:29:20 UTC 2020


On 4/11/20 1:17 pm, Aditya wrote:
> On 4/11/20 12:27 pm, Lukas Bulwahn wrote:
>>
>>
>> On Tue, 3 Nov 2020, Aditya wrote:
>>
>>> On 3/11/20 9:04 pm, Lukas Bulwahn wrote:
>>>> On Tue, Nov 3, 2020 at 3:47 PM Aditya <yashsri421 at gmail.com> wrote:
>>>>>
>>>>> On 3/11/20 7:09 pm, Lukas Bulwahn wrote:
>>>>>> On Tue, Nov 3, 2020 at 2:31 PM Aditya Srivastava <yashsri421 at gmail.com> wrote:
>>>>>>>
>>>>>>> Currently, checkpatch warns if the author signs-off two or more times
>>>>>>> for the same role.
>>>>>>> E.g., for commit 6dd47d9754ff ("mac80211: fix
>>>>>>> ieee80211_txq_setup_flows() failure path") we get warning:
>>>>>>>
>>>>>>> WARNING: Duplicate signature
>>>>>>> Signed-off-by: Johannes Berg <johannes.berg at intel.com>
>>>>>>>
>>>>>>> Here there are two Signed-off-by lines present for the same user. So,
>>>>>>> we get a warning to remove one.
>>>>>>>
>>>>>>> Similarly, we get warning if the author of the commit signs-off under
>>>>>>> co-developed-by.
>>>>>>> E.g. for commit 6e88559470f5 ("Documentation: Add section about
>>>>>>> CPU vulnerabilities for Spectre") we get:
>>>>>>>
>>>>>>> WARNING: Co-developed-by: should not be used to attribute nominal
>>>>>>> patch author 'Tim Chen <tim.c.chen at linux.intel.com>'
>>>>>>> Co-developed-by: Tim Chen <tim.c.chen at linux.intel.com>
>>>>>>>
>>>>>>> Provide fixes by removing the duplicate signature line and the
>>>>>>> co-developed-by line from the commit
>>>>>>>
>>>>>>
>>>>>>
>>>>>> The duplicate signature fix is simply wrong, because the indication of
>>>>>> the duplicate signature is imprecise.
>>>>>>
>>>>>> Probably, we would first really need to understand the valid cases of
>>>>>> duplicate signature warnings compared to the invalid ones.
>>>>>>
>>>>>> Please rework.
>>>>>>
>>>>>>
>>>>>> Lukas
>>>>>>
>>>>>
>>>>> Oh, Alright. I get it now.
>>>>> I ran a script over WARNING: Duplicate signature.
>>>>> I found out that it is occurring with these prefixes and their
>>>>> frequencies:
>>>>>
>>>>> Acked-by -- 6
>>>>> Signed-off-by -- 218
>>>>> Tested-by -- 3
>>>>> Reported-by -- 1
>>>>> Reviewed-by -- 8
>>>>> Cc -- 28
>>>>>
>>>>> IMO largely Signed-off-by warnings in this frequency might be false
>>>>> positives. This is so because we are simply checking for duplicate
>>>>> occurrences in the sign-offs in checkpatch.pl
>>>>>
>>>>>> Probably, we would first really need to understand the valid cases
>>>>>> of duplicate signature warnings compared to the invalid ones.
>>>>>>
>>>>>
>>>>> Is there any way I can know that the second sign-off is intended?
>>>>>
>>>>> In my observation, this particular commit was prefixed by '^Link:.*$'
>>>>> Alternatively, we could use maintainers list to ensure that the signer
>>>>> is maintainer and avoid this warning.
>>>>>
>>>>
>>>> That is somehow difficult to say, but probably it is best to
>>>> deactivate this multiple Signed-off-by: warning on git commits.
>>>>
>>>> On patches it makes sense to warn about duplicate Signed-off-by: ; on
>>>> commits, not so much.
>>>>
>>>>> How about other prefixes. Can we remove them?
>>>>>
>>>>
>>>> Can you show the duplicate Tested-by, Acked-by, Reported-by,
>>>> Reviewed-by, Cc in some structured format to quickly check those?
>>>>
>>>> Probably for those, we can really assess them to be all true positives
>>>> (maybe we can find out the reasons as well) and hence a fix for them
>>>> would make sense.
>>>>
>>>
>>> I generated git logs of all such commits, which might help us get some
>>> idea about mailing list or sign offs as well.
>>>
>>> Acked-by:
>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/acked_by.txt
>>>
>>> Reported-by:
>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/reported_by.txt
>>>
>>> Reviewed by:
>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/reviewed_by.txt
>>>
>>> Tested-by:
>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/tested_by.txt
>>>
>>> Cc: https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/cc.txt
>>>
>>>
>>>>> I observed one of the 'Tested-by' warnings and it was because of these
>>>>> sign offs:
>>>>>
>>>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl at googlemail.com>
>>>>> Tested-by:Liang Yang <liang.yang at amlogic.com>
>>>>> Acked-by: Liang Yang <liang.yang at amlogic.com>
>>>>> Tested-by:Liang Yang <liang.yang at amlogic.com>
>>>>> Acked-by: Liang Yang <liang.yang at amlogic.com>
>>>>> Signed-off-by: Miquel Raynal <miquel.raynal at bootlin.com>
>>>>>
>>>>> Here tested-by is occurring 2 times.
>>>>>
>>>>
>>>> That example seems to be a clear true positive. It would be
>>>> interesting to see how that happened to be added twice in a commit.
>>>> What was the state on the mailing list and what did the maintainer do
>>>> to add these tags twice (was this done manually or is there some
>>>> automatic tool involved?).
>>>>
>>>
>>> I found the mailing list state corresponding to that day. It indeed
>>> seems like a true positive.
>>>
>>> Link1:
>>> https://lore.kernel.org/linux-mtd/CAFBinCB9Z_H4wX3_g2hVL-XnS6Oq-KtPHa4Z1ws79FGp5jCapQ@mail.gmail.com/
>>>
>>> Link2 (reply):
>>> https://lore.kernel.org/linux-mtd/8ff232c3-f325-5b91-4de1-a39e63939df2@amlogic.com/
>>>
>>
>> Yes, and it suggest to fix patchwork :)
>>
>> How are your python programming skills?
>>
>> Do you want to try fixing patchwork for this issue?
>>
>> Lukas
>>
> 
> Sure, I'd like to try. Please guide me for it.
> 

I might have got it wrong. Did you mean fixing the patch we are
working on currently?
I must be intermediate with Python, not too expert though(like 3.5/5)

Thanks
Aditya


More information about the Linux-kernel-mentees mailing list