linux-kernel - Re: [PATCH] staging: iio: meter: use min() for comparison and assignment

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y2kDTxE38epBN368@kadam>
Date:   Mon, 7 Nov 2022 16:08:31 +0300
From:   Dan Carpenter <error27@...il.com>
To:     Deepak R Varma <drv@...lo.com>
Cc:     Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-iio@...r.kernel.org, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: iio: meter: use min() for comparison and
 assignment

On Mon, Nov 07, 2022 at 09:40:00AM +0530, Deepak R Varma wrote:
> Simplify code by using recommended min helper macro for logical
> evaluation and value assignment. This issue is identified by
> coccicheck using the minmax.cocci file.
> 
> Signed-off-by: Deepak R Varma <drv@...lo.com>
> ---
>  drivers/staging/iio/meter/ade7854-i2c.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> index a9a06e8dda51..a6ce7b24cc8f 100644
> --- a/drivers/staging/iio/meter/ade7854-i2c.c
> +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> @@ -61,7 +61,7 @@ static int ade7854_i2c_write_reg(struct device *dev,
>  unlock:
>  	mutex_unlock(&st->buf_lock);
> 
> -	return ret < 0 ? ret : 0;
> +	return min(ret, 0);

The original code is better.

If it's a failure return the error code.  If it's not return zero.

You can only compare apples to apples.  min() makes sense if you're
talking about two lengths.  But here if ret is negative that's an error
code.  If it's positive that's the number of bytes.  If the error
code is less than the number of bytes then return that?  What???  It
makes no sense.

In terms of run time, this patch is fine but in terms of reading the
code using min() makes it less readable.

regards,
dan carpenter