On Thu, Apr 01, 2021 at 12:50:18PM +0300, Dan Carpenter wrote:
> On Thu, Apr 01, 2021 at 11:20:38AM +0200, Fabio Aiuto wrote:
> > @@ -677,9 +663,8 @@ u8 rtw_createbss_cmd(struct adapter *padapter)
> > u8 res = _SUCCESS;
> >
> > if (pmlmepriv->assoc_ssid.SsidLength == 0)
> > - RT_TRACE(_module_rtl871x_cmd_c_, _drv_info_, (" createbss for Any SSid:%s\n", pmlmepriv->assoc_ssid.Ssid));
> > + ;
> > else
> > - RT_TRACE(_module_rtl871x_cmd_c_, _drv_info_, (" createbss for SSid:%s\n", pmlmepriv->assoc_ssid.Ssid));
> >
> > pcmd = rtw_zmalloc(sizeof(struct cmd_obj));
>
> This is a bug. Smatch has a check for this which hopefully would have
> detected it (I haven't tested).
>
> There are some more similar issues below as well. So generally the rule
> is don't adjust the indenting if it's not related to your patch. In
> some cases you have been fixing the indenting but it should be done in
> a separate patch. But the other rule is that if your patch introduces
> a checkpatch warning then you need to fix it in the same patch. In
> this block the whole if statement should be removed. But also if you
> have something like:
>
> if (foo) {
> RT_TRACE(blha blah blah);
> return;
> }
>
> Then checkpatch will complain that the the curly braces are not
> required. (Checkpatch might not complain for your patch but it will
> complain when we re-run it with the -f option over the whole file). So
> you should update this to:
>
> if (foo)
> return;
>
> That's all considered part of deleting the RT_TRACE(). Also if there
> are empty curly braces then delete those in the same patch.
>
> I have looked over patches 1-7 and those seem basically fine. I'm not
> going to review any further into this patchset because you're going to
> have to redo them and I will be reviewing the v2 set later anyway. So
> just look it over yourself and check for any similar issues.
>
> regards,
> dan carpenter
>
Hi Dan,
I have the following:
if (rtw_createbss_cmd(adapter) != _SUCCESS)
- RT_TRACE(_module_rtl871x_mlme_c_, _drv_err_, ("Error =>rtw_createbss_cmd status FAIL\n"));
+ ;
will I leave
if (rtw_createbss_cmd(adapter) != _SUCCESS)
;
or just
rtw_createbss_cmd(adapter);
?
what's best from the static analysis point of view?
smatch and sparse says nothing about that.
Checkpatch too seems to ignore it, maybe the first one is good,
but I would like to be sure before sending another over 40 patches
long patchset.
thank you,
fabio
