linux-kernel - Re: [PATCH 10/11] Staging: vt6656: Combined nested conditions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Thu, 2 Jan 2014 17:29:54 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Sebastian Rachuj <sebastian.rachuj@...dium.uni-erlangen.de>
Cc:	linux@...ionality.eu, devel@...verdev.osuosl.org,
	linux-kernel@...cs.fau.de, tvboxspy@...il.com,
	gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
	forest@...ttletooquiet.net, more.andres@...il.com
Subject: Re: [PATCH 10/11] Staging: vt6656: Combined nested conditions

On Thu, Dec 26, 2013 at 07:55:39PM +0100, Sebastian Rachuj wrote:
> -	if (pbyDesireSSID != NULL) {
> -		if (((PWLAN_IE_SSID) pbyDesireSSID)->len != 0)
> -			pSSID = (PWLAN_IE_SSID) pbyDesireSSID;
> -	}
> +	if ((pbyDesireSSID != NULL) &&
> +	    (((PWLAN_IE_SSID) pbyDesireSSID)->len != 0))
> +		pSSID = (PWLAN_IE_SSID) pbyDesireSSID;

That's a lot of extra parentheses!

This patch is fine, but for future reference, adding a double negative
" != NULL" almost never doesn't makes the code less readable.  If we
remove it then this fits on one line.

	if (pbyDesireSSID && ((PWLAN_IE_SSID) pbyDesireSSID)->len != 0)
		pSSID = (PWLAN_IE_SSID) pbyDesireSSID;

The casting is horrible and this is garbage staging code.  If the types
were cleaned up then it would look like:

	if (pbyDesireSSID && pbyDesireSSID->len != 0)
		pSSID = pbyDesireSSID;

The naming is also totally bogus, of course.  What does "pby" mean?  I
think "Desire" is clear but I don't think it's native English.  The "p"
in "pSSID" probably stands for pointer???  It's as if someone
deliberately made every single character of this code as terrible as can
be.

	if (new_ssid && new_ssid->len != 0)
		ssid = new_ssid;

Technically " != 0" is the same as " != NULL" but " != 0" tells a story
and makes the code more readable.

> +				} else if ((pMgmt->eConfigMode == WMAC_CONFIG_AUTO) ||
> +					((pMgmt->eConfigMode == WMAC_CONFIG_IBSS_STA) && WLAN_GET_CAP_INFO_IBSS(pCurrBSS->wCapInfo)) ||
> +					((pMgmt->eConfigMode == WMAC_CONFIG_ESS_STA) && WLAN_GET_CAP_INFO_ESS(pCurrBSS->wCapInfo))) {

Adding all these extra parentheses makes it harder to tell which are
needed and which are noise.

				} else if (pMgmt->eConfigMode == WMAC_CONFIG_AUTO ||
					   (pMgmt->eConfigMode == WMAC_CONFIG_IBSS_STA &&
					    WLAN_GET_CAP_INFO_IBSS(pCurrBSS->wCapInfo)) ||
					   (pMgmt->eConfigMode == WMAC_CONFIG_ESS_STA &&
					    WLAN_GET_CAP_INFO_ESS(pCurrBSS->wCapInfo)) {

In this case, the extra parentheses were there in the original code but
there are other times where this patch adds a number of pretty bogus
parens.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/