Re: [PATCH 6/9] staging: brcm80211: fix checkpatch error 'assignment in if condition'

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

 



Jason,

On Wed, Sep 29, 2010 at 11:19:18AM -0700, jason wrote:
> Henry,
> 
> Henry Ptasinski wrote:
> > On Mon, Sep 27, 2010 at 01:37:18PM -0700, Jason Cooper wrote:
> >> @@ -1847,7 +1858,12 @@ dhd_add_if(dhd_info_t *dhd, int ifidx, void *handle, char *name,
> >>  	ASSERT(dhd && (ifidx < DHD_MAX_IFS));
> >>  
> >>  	ifp = dhd->iflist[ifidx];
> >> -	if (!ifp && !(ifp = MALLOC(dhd->pub.osh, sizeof(dhd_if_t)))) {
> >> +	if (!ifp) {
> >> +		DHD_ERROR(("%s: dhd->iflist[ifidx] null\n", __func__));
> >> +		return -EINVAL;
> >> +	}
> >> +	ifp = MALLOC(dhd->pub.osh, sizeof(dhd_if_t));
> >> +	if (!ifp) {
> >>  		DHD_ERROR(("%s: OOM - dhd_if_t\n", __func__));
> >>  		return -ENOMEM;
> >>  	}
> > 
> > I think you changed the logic here from AND to OR.  I believe this would
> > be more correct:
> > 
> > 	ifp = MALLOC(dhd->pub.osh, sizeof(dhd_if_t));
> > 	if (!(dhd->iflist[ifidx]) && (!ifp)) {
> > 		DHD_ERROR(("%s: OOM - dhd_if_t\n", __func__));
> > 		return -ENOMEM;
> > 	}
> > 
> 
> I was attempting to remove the checkpatch.pl error with as little
> interpretation as possible.  The current code executes the MALLOC() if and
> only if ifp != NULL.  eg, if and only if dhd->iflist[ifidx] != NULL.
 
Ah, missed that.  My attempted fix has a memory leak.

> Do you really want to bail only when _both_ are NULL?  What if the MALLOC()
> failed?  Or, what if the dhd->iflist[ifidx] was NULL?
> 

Yes, it should bail only when both are null.  A few lines later,
dhd->iflist[ifidx] gets set to ifp, so the net result of this code is to
initialize dhd->iflist[ifidx] if it's not initialized, and fail otherwise.

Here's take two.  With this change applied, your patch series passes a quick
smoke test (scan, associate, ping).

- Henry

diff --git a/drivers/staging/brcm80211/brcmfmac/dhd_linux.c b/drivers/staging/brcm80211/brcmfmac/dhd_linux.c
index db8f7bf..86eee74 100644
--- a/drivers/staging/brcm80211/brcmfmac/dhd_linux.c
+++ b/drivers/staging/brcm80211/brcmfmac/dhd_linux.c
@@ -1859,13 +1859,11 @@ dhd_add_if(dhd_info_t *dhd, int ifidx, void *handle, char *name,
 
 	ifp = dhd->iflist[ifidx];
 	if (!ifp) {
-		DHD_ERROR(("%s: dhd->iflist[ifidx] null\n", __func__));
-		return -EINVAL;
-	}
-	ifp = MALLOC(dhd->pub.osh, sizeof(dhd_if_t));
-	if (!ifp) {
-		DHD_ERROR(("%s: OOM - dhd_if_t\n", __func__));
-		return -ENOMEM;
+		ifp = MALLOC(dhd->pub.osh, sizeof(dhd_if_t));
+		if (!ifp) {
+			DHD_ERROR(("%s: OOM - dhd_if_t\n", __func__));
+			return -ENOMEM;
+		}
 	}
 
 	memset(ifp, 0, sizeof(dhd_if_t));
-- 


_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel


[Index of Archives]     [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