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

Lukas Bulwahn lukas.bulwahn at gmail.com
Tue Nov 3 15:34:51 UTC 2020


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 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?).


Lukas


More information about the Linux-kernel-mentees mailing list