On Monday, October 11, 2021 10:54:11 AM CEST Fabio M. De Francesco wrote:
> On Sunday, October 10, 2021 7:06:05 AM CEST Saurav Girepunje wrote:
> > Remove the unneeded and redundant check of variable on goto out.
> > Simplify the return using multiple goto label to avoid
> > unneeded check.
> >
> > Signed-off-by: Saurav Girepunje <saurav.girepunje@xxxxxxxxx>
> > ---
> >
> > ChangeLog V2:
> > -Add goto out after the memcpy for no error case return with
> > ret only. Free is not required on no error case.
>
> Please write versions logs that reflect clearly and unequivocally what you
> changed between revisions and why. Subjects, Commit messages (Changelogs),
> and Versions logs are the "specifics" of your work. There must be no
> inconsistencies between these and the code or the history of the changes of
> the code.
>
> You may think that I'm pedantic, but since I acked your patch, I don't want
> to be misunderstood to be a promoter of approximate or clearly incorrect
> messages.
>
> "Free is not required on no error case" conveys the message that you have
> changed something that is not required but that is still potentially
allowed.
>
> This is not the case because the problem that you fix with v2 is _not_
> something that is merely not required and unnecessary. You have fixed a bug
> that is introduced in v1. Introducing bugs is not allowed. If you do
> something that is not allowed you cannot simply say that it is not
required.
>
> >
> > ChangeLog V1:
> > -Remove the unneeded and redundant check of variable on
> > goto out.
> > -Simplify the return using multiple goto label to avoid
> > unneeded check.
> >
> > .../staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 22 +++++++++----------
> > 1 file changed, 10 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/
> staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> > index 0868f56e2979..ae9579dc0848 100644
> > --- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> > +++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> > @@ -2312,7 +2312,7 @@ static int rtw_cfg80211_add_monitor_if(struct
adapter
> *padapter, char *name, str
> > mon_wdev = rtw_zmalloc(sizeof(struct wireless_dev));
> > if (!mon_wdev) {
> > ret = -ENOMEM;
> > - goto out;
> > + goto err_zmalloc;
> > }
> >
> > mon_wdev->wiphy = padapter->rtw_wdev->wiphy;
> > @@ -2322,23 +2322,21 @@ static int rtw_cfg80211_add_monitor_if(struct
> adapter *padapter, char *name, str
> >
> > ret = cfg80211_register_netdevice(mon_ndev);
> > if (ret) {
> > - goto out;
> > + goto err_register;
> > }
> >
> > *ndev = pwdev_priv->pmon_ndev = mon_ndev;
> > memcpy(pwdev_priv->ifname_mon, name, IFNAMSIZ+1);
> > + goto out;
As Pavel noticed, probably you'd better return 'ret' here. The logic does not
change, but I guess that this is what Linux developers usually do.
>From a review by Dan Carpenter of one of your other patches: "[] Do nothing
labels only hurt readability and introduce "forgot to set the error code
bugs.".
Thanks,
Fabio
> >
> > -out:
> > - if (ret && mon_wdev) {
> > - kfree(mon_wdev);
> > - mon_wdev = NULL;
> > - }
> > -
> > - if (ret && mon_ndev) {
> > - free_netdev(mon_ndev);
> > - *ndev = mon_ndev = NULL;
> > - }
> > +err_register:
> > + kfree(mon_wdev);
> > + mon_wdev = NULL;
>
> Probably you have already read a message by Greg Kroah-Hartman that
confirms
> what I wrote in another message: "There is no need to set a local variable
> like this to NULL.".
>
> So please submit a v3. With the two changes requested above, my "acked-by"
> tag is confirmed again.
>
> Thanks,
>
> Fabio
>
> >
> > +err_zmalloc:
> > + free_netdev(mon_ndev);
> > + *ndev = mon_ndev = NULL;
> > +out:
> > return ret;
> > }
> >
> > --
> > 2.32.0
> >
> >
> >
>
>
>
>
>
