linux-kernel - Re: [PATCH v4 1/4] staging: rtl8723bs: core: Remove true and false comparison

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: <20211103122159.GT2794@kadam>
Date:   Wed, 3 Nov 2021 15:21:59 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Kushal Kothari <kushalkothari285@...il.com>
Cc:     gregkh@...uxfoundation.org, kush19992810@...il.com,
        outreachy-kernel@...glegroups.com, fabioaiuto83@...il.com,
        ross.schm.dev@...il.com, fmdefrancesco@...il.com,
        marcocesati@...il.com, straube.linux@...il.com,
        philippesdixon@...il.com, manuelpalenzuelamerino@...il.com,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        mike.rapoport@...il.com, kushalkotharitest@...glegroups.com
Subject: Re: [PATCH v4 1/4] staging: rtl8723bs: core: Remove true and false
 comparison

On Sat, Oct 23, 2021 at 01:05:47PM +0530, Kushal Kothari wrote:
>  	struct mlme_priv *pmlmepriv = &(padapter->mlmepriv);
>  	u8 mstatus;
>  
> -	if ((check_fwstate(pmlmepriv, WIFI_ADHOC_MASTER_STATE) == true)
> -		|| (check_fwstate(pmlmepriv, WIFI_ADHOC_STATE) == true)) {
> +	if ((check_fwstate(pmlmepriv, WIFI_ADHOC_MASTER_STATE)) ||
> +		(check_fwstate(pmlmepriv, WIFI_ADHOC_STATE))) {
>  		return;
>  	}
>  

This is a "let it slide" moment.

Reviewed-by: Dan Carpenter <dan.carpenter@...cle.com>

Normally would keep my mouth shut but we had already discussed it a bit
and it's really important to know the rules.  In your original patch
you did this correctly, and then a reviewer made a comment about a
different set of parentheses and you modified this one as well so now
it's wrong.  The extra parens get removed in [PATCH 2/2] so, whatever,
it's fine.

The rule is that if you change a line of code you are allowed to make
small changes to fix the style to make checkpatch happy about *THAT
LINE*.  It's not required.  Try to avoid making too many unrelated
changes if it's going to make reviewing difficult.

But I don't want to see three patches fixing the style for a single line
of code.  You can take it too far in either direction.  We had a guy
who was re-writing all the error handling for a function but he would do
it in 5 to 8 patches.  It was crazy hard to review.  He introduced a lot
of bugs as well.  It would have been easier to review as one patch.

But if you change a line and your change introduces a checkpatch warning
then you *must* change the line.  So here, removing the == true, means
you *must* remove the extra parentheses.

But it's fine.  Now you know the rules and can do correctly going
forward.

regards,
dan carpenter