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

Lukas Bulwahn lukas.bulwahn at gmail.com
Mon Nov 9 08:30:50 UTC 2020


On Wed, Nov 4, 2020 at 10:29 AM Aditya <yashsri421 at gmail.com> wrote:
>
> 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)
>

No, I meant the program patchwork; read the thread and learn how the
problem was picked up in this patch.

Lukas


More information about the Linux-kernel-mentees mailing list