[PATCH v2] staging: rtl8723bs: os_dep: simplify the return statement.

[PATCH v2] staging: rtl8723bs: os_dep: simplify the return statement.

[Saurav Girepunje]

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.

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;

+err_zmalloc:
+	free_netdev(mon_ndev);
+	*ndev = mon_ndev = NULL;
+out:
 	return ret;
 }

--
2.32.0

Re: [PATCH v2] staging: rtl8723bs: os_dep: simplify the return statement.

[Fabio M. De Francesco]

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.
> 
> 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;

I think this is the right thing to do in order to remove the bug you 
introduced in v1. Well done.

> 
> -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;
> 
> +err_zmalloc:
> +	free_netdev(mon_ndev);
> +	*ndev = mon_ndev = NULL;

Not sure about what the Linux coding guidelines say, but I think that 
assigning NULL to local (on stack) pointers (mon_wdev, mon_ndev) at this 
point is unnecessary. There is no risk of reusing them after the "out" 
labelled block, because at function exit they are destroyed when the stack is 
unwound.

If you decide to remove these assignments, take care to leave "*ndev = NULL;" 
as-is (why?).

Aside from this minor objection...

Acked-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Thanks,

Fabio

> +out:
>  	return ret;
>  }
> 
> --
> 2.32.0
> 
> 
> 

Re: [PATCH v2] staging: rtl8723bs: os_dep: simplify the return statement.

[Pavel Skripkin]

On 10/10/21 08:06, 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>

[code snip]

>   	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;
> 


This looks confusing for readers. This is success path and ret is 
guaranteed to be 0 at this point, so isn't `return 0;` enough here?

Thanks


> +out:
>   	return ret;
>   }
> 



With regards,
Pavel Skripkin

Re: [PATCH v2] staging: rtl8723bs: os_dep: simplify the return statement.

[Greg KH]

On Sun, Oct 10, 2021 at 10:36:05AM +0530, 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.
> 
> 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;

There is no need to set a local variable like this to NULL.

thanks,

greg k-h

Re: [PATCH v2] staging: rtl8723bs: os_dep: simplify the return statement.

[Fabio M. De Francesco]

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
> 
> 
> 

Re: [PATCH v2] staging: rtl8723bs: os_dep: simplify the return statement.

[Dan Carpenter]

On Sun, Oct 10, 2021 at 10:36:05AM +0530, 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>

No, sorry but the goto bunny hops, the pointless do-nothing gotos, and
the come from labels are all terrible.

regards,
dan carpenter

Re: [PATCH v2] staging: rtl8723bs: os_dep: simplify the return statement.

[Fabio M. De Francesco]

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@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;

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
> > 
> > 
> > 
> 
> 
> 
> 
> 

Re: [PATCH v2] staging: rtl8723bs: os_dep: simplify the return statement.

[Saurav Girepunje]

On 11/10/21 2:24 pm, 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@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. 
> 
My intension was to mention changes done on V2 with respect to V1. 
I will try to be more clear on changelog messages. 
>>
>> 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.".
> 
Yes, Agree this is another improvement possible on this function.
> So please submit a v3. With the two changes requested above, my "acked-by" 
> tag is confirmed again.
> 
> Thanks,
> 
> Fabio
>
I will submit a v3.
>>
>> +err_zmalloc:
>> +	free_netdev(mon_ndev);
>> +	*ndev = mon_ndev = NULL;
>> +out:
>>  	return ret;
>>  }
>>
>> --
>> 2.32.0
>>
>>
>>
> 
> 
> 
> 

Re: [PATCH v2] staging: rtl8723bs: os_dep: simplify the return statement.

[Saurav Girepunje]

On 10/10/21 12:54 pm, 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@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.
>>
>> 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;
> 
> I think this is the right thing to do in order to remove the bug you 
> introduced in v1. Well done.
> 
Most of the people suggesting to use return 0 from here instead of goto.
I will send another patch.
>>
>> -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;
>>
>> +err_zmalloc:
>> +	free_netdev(mon_ndev);
>> +	*ndev = mon_ndev = NULL;
> 
> Not sure about what the Linux coding guidelines say, but I think that 
> assigning NULL to local (on stack) pointers (mon_wdev, mon_ndev) at this 
> point is unnecessary. There is no risk of reusing them after the "out" 
> labelled block, because at function exit they are destroyed when the stack is 
> unwound.
> 
> If you decide to remove these assignments, take care to leave "*ndev = NULL;" 
> as-is (why?).
> 
Yes, This is also improvement needed on this function.
> Aside from this minor objection...
> 
> Acked-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> 
> Thanks,
> 
> Fabio
> 
>> +out:
>>  	return ret;
>>  }
>>
>> --
>> 2.32.0
>>
>>
>>
> 
> 
> 
> 
Regards,
Saurav

Re: [PATCH v2] staging: rtl8723bs: os_dep: simplify the return statement.

[Saurav Girepunje]

On 11/10/21 6:00 pm, Dan Carpenter wrote:
> On Sun, Oct 10, 2021 at 10:36:05AM +0530, 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>
> 
> No, sorry but the goto bunny hops, the pointless do-nothing gotos, and
> the come from labels are all terrible.
> 
> regards,
> dan carpenter
> 

Thanks dan for your input.

Regards,
Saurav

Re: [PATCH v2] staging: rtl8723bs: os_dep: simplify the return statement.

[Saurav Girepunje]

On 11/10/21 6:03 pm, Fabio M. De Francesco wrote:
> 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@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;
> 
> 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.
> 
Yes, Logic does not change. However I was also not sure which one is more preferable 
therefore asked for input on v1 whether to use goto or simply return value from here.
It seems return from here is better choice for readability point of view. 
 
> 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
>>>
>>>
>>>
>>
>>
>>
>>
>>
> 
> 
> 
> 
Regards,
Saurav