linux-kernel - Re: [PATCH 1/2] staging: rtl8192u: use max macro instead of ternary operator

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: <20220407072803.GI3293@kadam>
Date:   Thu, 7 Apr 2022 10:28:03 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Rebecca Mckeever <remckee0@...il.com>
Cc:     outreachy@...ts.linux.dev,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] staging: rtl8192u: use max macro instead of ternary
 operator

On Wed, Apr 06, 2022 at 07:09:43PM -0500, Rebecca Mckeever wrote:
> Replace ternary operator with max macro to increase readability
> and conform to Linux kernel coding style.
> Found with minmax coccinelle script.
> 
> Signed-off-by: Rebecca Mckeever <remckee0@...il.com>
> ---
>  drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
> index 78cc8f357bbc..a10c1303695b 100644
> --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
> +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
> @@ -470,7 +470,7 @@ int ieee80211_wx_get_encode(struct ieee80211_device *ieee,
>  		return 0;
>  	}
>  	len = crypt->ops->get_key(keybuf, SCM_KEY_LEN, NULL, crypt->priv);
> -	erq->length = (len >= 0 ? len : 0);
> +	erq->length = max(len, 0);

Neither before nor after is really readable.  It would be better to
write it as:

	len = crypt->ops->get_key(keybuf, SCM_KEY_LEN, NULL, crypt->priv);
	if (len < 0)
		len = 0;
	erq->length = len;

There are few rules/patterns that apply here:
1) Do error handling right away.
2) Do error handling not success handling.
3) Try to keep the error path and the success path separate.
4) Try to keep the success path indented one tab and the error path
   indented two tabs.  It looks like this to the reader:

	a = frob();
	if (fail)
		cleanup;
	b = frob();
	if (fail)
		cleanup;


	success
	check
		fail
	success
	check
		fail

So then if you want to understand what a function does at a high level
you can just skim the code which is indented one tab and ignore the
error handling code.

regards,
dan carpenter