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