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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 09/10/21 10:29 pm, Fabio M. De Francesco wrote:
On Saturday, October 9, 2021 5:39:12 PM 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>
---
  .../staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 29 ++++++++++---------
  1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/
staging/rtl8723bs/os_dep/ioctl_cfg80211.c
index 0868f56e2979..574fdb6adce7 100644
--- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
+++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
@@ -2282,18 +2282,18 @@ static int rtw_cfg80211_add_monitor_if(struct
adapter *padapter, char *name, str

  	if (!name) {
  		ret = -EINVAL;
-		goto out;
+		goto err_out;
  	}

  	if (pwdev_priv->pmon_ndev) {
  		ret = -EBUSY;
-		goto out;
+		goto err_out;
  	}

  	mon_ndev = alloc_etherdev(sizeof(struct
rtw_netdev_priv_indicator));
  	if (!mon_ndev) {
  		ret = -ENOMEM;
-		goto out;
+		goto err_out;
  	}

  	mon_ndev->type = ARPHRD_IEEE80211_RADIOTAP;
@@ -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,22 +2322,23 @@ 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);

-out:
-	if (ret && mon_wdev) {
-		kfree(mon_wdev);
-		mon_wdev = NULL;
-	}
+err_register:

-	if (ret && mon_ndev) {
-		free_netdev(mon_ndev);
-		*ndev = mon_ndev = NULL;
-	}
+	kfree(mon_wdev);
+	mon_wdev = NULL;
+
+err_zmalloc:
+
+	free_netdev(mon_ndev);
+	*ndev = mon_ndev = NULL;
+
+err_out:

  	return ret;
  }
--
2.32.0

At a quick look it seems that now you unconditionally free memory and free a
network device immediately after the last memcpy().

Why are you doing this even if no errors are returned from device and memory
allocations (i.e., while "ret" is still equal to zero)?

Thanks,

Fabio



Thanks Fabio for review. Yes, That is missed.
I can do that in either one of the way.
Either I should check for ret before free or I should use goto out(may be I will rename err_out goto label to "out" only) just after the memcpy statement so that function will not free for no error case and return ret value only.

Please let me know you input on above.

Regards,
Saurav Girepunje




[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux