On Thu, Apr 01, 2021 at 05:32:36PM +0300, Dan Carpenter wrote:
> On Thu, Apr 01, 2021 at 03:55:37PM +0200, Fabio Aiuto wrote:
> >
> > 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.
>
> rtw_createbss_cmd() can only fail if this allocation fails:
>
> pcmd = rtw_zmalloc(sizeof(struct cmd_obj));
>
> In current kernels, that size of small allocation will never fail. But
> we alway write code as if every allocation can fail.
>
> Normally when an allocation fails then we want to return -ENOMEM and
> clean up. But this code is an event handler for firmware events and
> there isn't any real clean up to do. Since there is nothing we can do
> then this is basically working and fine.
>
> How I would write this is:
>
> ret = rtw_createbss_cmd(adapter);
> if (ret != _SUCCESS)
> goto unlock;
> }
> }
> unlock:
> spin_unlock_bh(&pmlmepriv->lock);
> }
>
> That doesn't change how the code works but it signals to the the reader
> what your intention is. If we just remove the error handling then it's
> ambiguous.
>
> rtw_createbss_cmd(adapter);
> }
> }
> <-- Futurue programmer decides to add code here then figuring
> that rtw_createbss_cmd() can fail is a problem.
>
> spin_unlock_bh(&pmlmepriv->lock);
> }
>
> But for something like this which is maybe more subtle than just a
> straight delete of lines of code, then consider pulling it out into its
> own separate patch. That makes it easier to review. Put all the stuff
> that I said in the commit message:
>
> ---
> [PATCH] tidy up some error handling
>
> The RT_TRACE() output is not useful so we want to delete it. In this
> case there is no cleanup for rtw_createbss_cmd() required or even
> possible. I've deleted the RT_TRACE() output and added a goto unlock
> to show that we can't continue if rtw_createbss_cmd() fails.
>
> ---
>
> >
> > 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.
>
> Don't send 40 patches. Just send 10 at a time until you get a better
> feel for which ones are going to get applied or not. :P It's not
> arbitrary, and I'm definitely not trying to NAK your patches. Once you
> learn the rules I hope that it's predictable and straight forward.
>
> regards,
> dan carpenter
>
Hi Dan,
sorry again. In this case:
@@ -828,10 +829,11 @@ void rtw_surveydone_event_callback(struct adapter *adapter, u8 *pbuf)
pmlmepriv->fw_state = WIFI_ADHOC_MASTER_STATE;
- if (rtw_createbss_cmd(adapter) != _SUCCESS)
- ;
-
pmlmepriv->to_join = false;
+
+ ret = rtw_createbss_cmd(adapter);
+ if (ret != _SUCCESS)
+ goto unlock;
}
}
I decided to move the set to false of pmlepriv->to_join before
the rtw_createbss_cmd(). In old code that statement was executed
unconditionally and seems not to be tied to the failure of
rtw_createbss_cmd().
The eventual goto would skip this instruction so I moved it
before.
What do you think?
Thank you,
fabio
