linux-kernel - Re: [PATCH] timekeeping: Use min() to fix Coccinelle warning

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87h6cxuejr.ffs@tglx>
Date: Wed, 10 Jul 2024 23:23:36 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Thorsten Blum <thorsten.blum@...lux.com>
Cc: jstultz@...gle.com, sboyd@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] timekeeping: Use min() to fix Coccinelle warning

Thorsten!

On Sat, Jun 29 2024 at 16:12, Thorsten Blum wrote:
> On 24. Jun 2024, at 23:36, Thomas Gleixner <tglx@...utronix.de> wrote:
>> On Mon, Jun 24 2024 at 08:24, Thorsten Blum wrote:
>> 
>>> Fixes the following Coccinelle/coccicheck warning reported by
>>> minmax.cocci:
>>> 
>>> WARNING opportunity for min()
>> 
>> I'm fine with the change, but not so much with the change log.
>> 
>> You cannot fix a coccinelle warning. You can only fix the code which
>> triggers the warning, right?
>> 
>> 'Opportunity to use min()' is nothing else than an opportunity, but
>> what's the benefit of replacing correct code with it? What does this
>> fix?
>> 
>> It fixes nothing. So calling it a fix is confusing at best.
>
> I think it's pretty common to "fix a warning" -- there are thousands of
> commits in the kernel using this wording in the summary alone -- even
> when the change doesn't actually "fix" anything other than removing the
> warning.

Following the lemmings does not make a technical argument.

> However, how about 'resolve' instead?
>
> timekeeping: Use min() to resolve Coccinelle warning
>
>> What you want to say is something like this:
>> 
>> Subject: timekeeping: Replace open coded min()
>> 
>> Replace open coded min() because $GOOD_REASON
>> 
>> Discovered by minmax.cocci
>> 
>> $GOOD_REASON is not 'coccinelle emitted a warning'.
>
> Removing a warning can be a good reason in itself to refactor code,
> because fewer warnings make "real" warnings and potential problems
> become more noticeable and thus more likely to get fixed. In short, it
> improves maintainability.
>
> To me this is obvious, but I'm happy to add something like "refactor
> code to remove warning and improve overall maintainability" to the
> commit message.

Again. The code _is_ correct and replacing it with min() does neither
fix nor remove a warning.

What's wrong with saying:

       timekeeping: Replace open coded min()

       The open coded min implementation is correct, but triggers a
       Coccinelle warning in minmax.cocci.

       Replace the open coded variant with min() to reduce the noise of
       false positives.

That does not sound as spetacular as 'Fix', but it reflects the actual
technical reason for making this change.

Thanks,

        tglx