lkml.org 
[lkml]   [2021]   [Oct]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v2] staging: rtl8723bs: os_dep: simplify the return statement.
    Date
    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@gmail.com>
    > ---
    >
    > 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;
    >
    > -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
    >
    >
    >




    \
     
     \ /
      Last update: 2021-10-11 10:55    [W:3.416 / U:0.164 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site