| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAKXUXMx8siSm67jkBP_r+OKyKALfT2EDcX_SfX7JGBy3YisXcQ@mail.gmail.com>
Date: Mon, 23 Nov 2020 16:16:17 +0100
From: Lukas Bulwahn <lukas.bulwahn@...il.com>
To: Aditya Srivastava <yashsri421@...il.com>
Cc: Joe Perches <joe@...ches.com>,
linux-kernel-mentees@...ts.linuxfoundation.org,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] checkpatch: add fix and improve warning msg for
Non-standard signature
On Mon, Nov 23, 2020 at 4:04 PM Aditya Srivastava <yashsri421@...il.com> wrote:
>
> Currently, checkpatch.pl warns for BAD_SIGN_OFF on non-standard signature
> styles.
>
> This warning occurs because of incorrect use of signature tags,
> e.g. an evaluation on v4.13..v5.8 showed the use of following incorrect
> signature tags, which may seem correct, but are not standard:
>
> 1) Requested-by (count: 48) => Suggested-by
> Rationale: In an open-source project, there are no 'requests', just
> 'suggestions' to convince a maintainer to accept your patch
>
> 2) Co-authored-by (count: 43) => Co-developed-by
> Rationale: Co-developed-by and Co-authored-by are synonyms
>
> 3) Analyzed-by (count: 22) / Analysed-by (count: 5) => Co-developed-by
> Rationale: Analyzing is a part of Software Development, so
> 'Co-developed-by' is perfectly fine, even if contributor did not create
> code
>
> 4) Improvements-by (count: 19) => Co-developed-by
>
> 5) Noticed-by (count: 11) => Reported-by
>
> 6) Inspired-by (count: 11) => Suggested-by
>
> 7) Verified-by (count: 8) => Tested-by
> Rationale: Used by a single user. On reading mailing list, it seems
> Tested-by might be a suitable alternative
>
> 8) Okay-ished-by (count: 8) => Acked-by
> Rationale: Used by a single user. On reading mailing list, it seems
> Acked-by must be suitable alternative
>
> 9) Acked-for-MFD-by (count: 6) => Acked-by
>
> 10) Reviewed-off-by (count: 5) => Reviewed-by
>
> 11) Proposed-by (count: 5) => Suggested-by
> Rationale: On observing the mailing list, this tag is always used for a
> maintainer. It seems that the changes might have been suggested by them
> and the tag is used as acknowledgment for the same
>
> 12) Fixed-by (count: 3) => Co-developed-by
> Rationale: Fixing bug is a part of Software Development, so
> 'Co-developed-by' is perfectly fine, even if contributor did not create
> code
>
> 13) Pointed-out-by (count: 3) / Pointed-at-by (count: 2) => Suggested-by
> Rationale: The tags are used for maintainers. It seems that the changes
> might have been suggested by them and the tag is used as acknowledgment
> for the same
> E.g., Pointed-at-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>
> 14) Suggestions-by (count: 3) => Suggested-by
>
> 15) Generated-by (count: 17) => remove the tag
> On observing the mailing list, this tag is always used for quoting the
> tool or script, which might have been used to generate the patch.
> E.g. Generated-by: scripts/coccinelle/api/alloc/kzalloc-simple.cocci
>
> 16) Celebrated-by (count: 3) => remove the tag
> This tag was used for only one commit. On observing mailing list, it seem
> like the celebration for a particular patch and changes.
>
> Provide a fix by:
> 1) replacing the non-standard signature with its standard equivalent
> 2) removing the signature if it is not required
>
> Also, improve warning messages correspondingly, providing users
> suggestions to either replace or remove the signature. Also provide
> suitable rationale to the user for the suggestion made.
>
> Signed-off-by: Aditya Srivastava <yashsri421@...il.com>
> ---
> changes in v2: replace commit specific example with brief evaluation
>
> changes in v3: provide rationale to users for every signature tag suggestion;
> modify commit message describing arrival to conclusion in a structured way
>
> scripts/checkpatch.pl | 101 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 99 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index fdfd5ec09be6..f402c9c3958f 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -506,6 +506,81 @@ our $signature_tags = qr{(?xi:
> Cc:
> )};
>
> +our %standard_signature_fix = (
> + "Requested-by:" => {
> + suggestion => "Suggested-by:",
> + rationale => "In an open-source project, there are no 'requests', just 'suggestions' to convince a maintainer to accept your patch",
> + },
> + "Co-authored-by:" => {
> + suggestion => "Co-developed-by:",
> + rationale => "Co-developed-by and Co-authored-by are synonyms",
> + },
Maybe this reads better:
Co-developed-by is the standard tag for patches developed by multiple authors.
> + "Analyzed-by:" => {
> + suggestion => "Co-developed-by:",
> + rationale => "Analyzing is a part of Software Development, so 'Co-developed-by' is perfectly fine, even if contributor did not create code",
> + },
> + "Analysed-by:" => {
> + suggestion => "Co-developed-by:",
> + rationale => "Analysing is a part of Software Development, so 'Co-developed-by' is perfectly fine, even if contributor did not create code",
> + },
> + "Improvements-by:" => {
> + suggestion => "Co-developed-by:",
> + rationale => "Performing improvements are a part of Software Development, so 'Co-developed-by' is perfectly fine, even if contributor did not create code",
> + },
> + "Noticed-by:" => {
> + suggestion => "Reported-by:",
> + rationale => "Reported-by and Noticed-by are synonyms",
> + },
Similarly:
Reported-by is the standard tag for denoting any notice or report.
> + "Inspired-by:" => {
> + suggestion => "Suggested-by:",
> + rationale => "Suggested-by is the standard signature tag for acknowledging user for their suggestions",
> + },
> + "Verified-by:" => {
> + suggestion => "Tested-by:",
> + rationale => "Tested-by and Verified-by are synonyms",
> + },
Similarly here.
> + "Okay-ished-by:" => {
> + suggestion => "Acked-by:",
> + rationale => "Acked-by is the standard signature tag for recording your approval",
> + },
I think "your" is wrong here; the person that runs checkpatch may not
be the person providing the approval.
So maybe:
- for recording one's approval.
or:
- for recording acknowledgement.
> + "Acked-for-MFD-by:" => {
> + suggestion => "Acked-by:",
> + rationale => "Acked-by is the standard signature tag for recording your approval",
> + },
> + "Reviewed-off-by:" => {
> + suggestion => "Reviewed-by:",
> + rationale => "Reviewed-by is the standard signature tag for recording your approval",
> + },
for recording the review.
> + "Proposed-by:" => {
> + suggestion => "Suggested-by:",
> + rationale => "Proposing changes is same as suggesting changes, so Suggested-by seems perfectly fine",
> + },
Similarly, the sentence as above could be used.
> + "Fixed-by:" => {
> + suggestion => "Co-developed-by:",
> + rationale => "Fixing bug is a part of Software Development, so 'Co-developed-by' is perfectly fine, even if contributor did not create code",
> + },
> + "Pointed-out-by:" => {
> + suggestion => "Suggested-by:",
> + rationale => "Pointing out certain changes is synonymous to suggesting changes, so Suggested-by seems perfectly fine",
> + },
> + "Pointed-at-by:" => {
> + suggestion => "Suggested-by:",
> + rationale => "Pointing at certain changes is synonymous to suggesting changes, so Suggested-by seems perfectly fine",
> + },
> + "Suggestions-by:" => {
> + suggestion => "Suggested-by:",
> + rationale => "Suggested-by is the standard signature tag for acknowledging user for their suggestions",
> + },
> + "Generated-by:" => {
> + suggestion => "remove",
> + rationale => "Signature tags are used to acknowledge users for their contributions. It is advised to describe about tools in commit description instead",
> + },
> + "Celebrated-by:" => {
> + suggestion => "remove",
> + rationale => "Signature tags are used to acknowledge users for their contributions. This tag may not be required at all",
> + },
> +);
> +
> our @typeListMisordered = (
> qr{char\s+(?:un)?signed},
> qr{int\s+(?:(?:un)?signed\s+)?short\s},
> @@ -2773,8 +2848,30 @@ sub process {
> my $ucfirst_sign_off = ucfirst(lc($sign_off));
>
> if ($sign_off !~ /$signature_tags/) {
> - WARN("BAD_SIGN_OFF",
> - "Non-standard signature: $sign_off\n" . $herecurr);
> + my $suggested_signature = "";
> + my $rationale = "";
> + if (exists($standard_signature_fix{$sign_off})) {
> + $suggested_signature = $standard_signature_fix{$sign_off}{'suggestion'};
> + $rationale = $standard_signature_fix{$sign_off}{'rationale'};
> + }
> + if ($suggested_signature eq "") {
> + WARN("BAD_SIGN_OFF",
> + "Non-standard signature: $sign_off\n" . $herecurr);
> + }
> + elsif ($suggested_signature eq "remove") {
> + if (WARN("BAD_SIGN_OFF",
> + "Non-standard signature: $sign_off. Please consider removing this signature tag. $rationale\n" . $herecurr) &&
> + $fix) {
> + fix_delete_line($fixlinenr, $rawline);
> + }
> + }
> + else {
> + if (WARN("BAD_SIGN_OFF",
> + "Non-standard signature: $sign_off. Please use '$suggested_signature' instead. $rationale\n" . $herecurr) &&
> + $fix) {
> + $fixed[$fixlinenr] =~ s/$sign_off/$suggested_signature/;
> + }
> + }
> }
> if (defined $space_before && $space_before ne "") {
> if (WARN("BAD_SIGN_OFF",
> --
> 2.17.1
>
Powered by blists - more mailing lists