Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
From:   "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To:     Jaehee Park <jhpark1013@gmail.com>
Cc:     Larry.Finger@lwfinger.net, phil@philpotter.co.uk,
        gregkh@linuxfoundation.org, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org, outreachy@lists.linux.dev,
        jhpark1013@gmail.com
Subject: Re: [PATCH v4 1/7] staging: r8188eu: remove unnecessary braces in single statement block
Date:   Fri, 22 Apr 2022 01:20:29 +0200
Message-ID: <3485895.R56niFO833@leap>
In-Reply-To: <16185752.0ZKypZ73Fx@leap>
References: <cover.1650568579.git.jhpark1013@gmail.com> <b96ae957e2ddc519e56813a1c56e770168f67a2d.1650568579.git.jhpark1013@gmail.com> <16185752.0ZKypZ73Fx@leap>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="utf-8"
Precedence: bulk

On gioved=C3=AC 21 aprile 2022 21:52:14 CEST Fabio M. De Francesco wrote:
> On gioved=C3=AC 21 aprile 2022 21:20:26 CEST Jaehee Park wrote:
> > Remove braces for single statement block to minimize the number of
> > empty lines, without loss of readability. Issue found with checkpatch.
> > WARNING: braces {} are not necessary for single statement blocks
> >=20
> > Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
> > ---
> >  drivers/staging/r8188eu/core/rtw_mlme.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >=20
> > diff --git a/drivers/staging/r8188eu/core/rtw_mlme.c b/drivers/staging/
> r8188eu/core/rtw_mlme.c
> > index 3e9882f89f76..d3f4d7cdfa08 100644
> > --- a/drivers/staging/r8188eu/core/rtw_mlme.c
> > +++ b/drivers/staging/r8188eu/core/rtw_mlme.c
> > @@ -112,9 +112,8 @@ void _rtw_free_mlme_priv(struct mlme_priv=20
*pmlmepriv)
> > =20
> >  	rtw_free_mlme_priv_ie_data(pmlmepriv);
> > =20
> > -	if (pmlmepriv) {
> > +	if (pmlmepriv)
> >  		vfree(pmlmepriv->free_bss_buf);
> > -	}
> > =20
> >  }
> > =20
> > --=20
> > 2.25.1
> >=20
>=20
> Hi Jaehee,
>=20
> If you just want to remove the braces, your patch is good.
>=20
> But... Did you ask yourself why this code needs to test if "pmlmepriv" is=
=20
> valid before calling vfree() on it? Actually, the code doesn't need this=
=20
> test, it just needs to call "vfree(pmlmepriv)" unconditionally.

When copy-pasting I overlooked a couple of things... However the argument=20
is the same: the code doesn't need that 'if' statement before vfree().

I wanted to write "[] it just needs to call "vfree(pmlmepriv-
>free_bss_buf)" unconditionally".

I apologize for not reading twice before sending my email too soon after=20
writing and moving sentences around.

> OK, I know that this is not directly related to your patch or any other=20
> that is explicitly made just for removing unnecessary braces...=20

"pmlmepriv" is valid because we know it from the previous call to=20
rtw_free_mlme_priv_ie_data(). Therefore the code can dereference=20
"pmlmepriv".

=46urthermore, even if "free_bss_buf" were NULL it wouldn't matter because=
=20
"vfree(pmlmepriv->free_bss_buf)" would be a no-op.

Sorry if this inaccurate wording might have confused you about what I was=20
trying to say.

Thanks,

=46abio=20