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

Lukas Bulwahn lukas.bulwahn at gmail.com
Wed Nov 4 06:57:38 UTC 2020



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


More information about the Linux-kernel-mentees mailing list