linux-kernel - Re: [PATCH 4/7] vt6655: Fixed most of the checkpatch warnings in wpa2

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20131231165258.GU28413@mwanda>
Date:	Tue, 31 Dec 2013 19:52:58 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Michael Gunselmann <michael.gunselmann@...dium.uni-erlangen.de>
Cc:	devel@...verdev.osuosl.org, linux-kernel@...cs.fau.de,
	gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
	martin.hofmann@...dium.uni-erlangen.de, forest@...ttletooquiet.net
Subject: Re: [PATCH 4/7] vt6655: Fixed most of the checkpatch warnings in wpa2

On Mon, Dec 30, 2013 at 03:52:34PM +0100, Michael Gunselmann wrote:
> wpa2.h: Checkpatch does no longer complain about anything
> 
> wpa2.c: Checkpatch complains about some lines that are longer than
> 	80 characters. Breaking them would deteriorate the readability
> 	so these lines were not touched.
> 
> vntwifi.c: Fixing style problems in wpa2.h made it necessary to adjust
> 	   variable declarations that depended on typedefs in wpa2.h.
> 	   The checkpatch cleanup of this file still remains.
> 

This patch changes too many things at once to review comfortably.  It
random changes to several files all jumbled together.  Break it apart
into logical chunks and resend.

>  
> -		if (pRSN->len >= 8) { // ver(2) + GK(4) + PK count(2)
> -			pBSSNode->wCSSPKCount = *((unsigned short *)&(pRSN->abyRSN[4]));
> +		if (pRSN->len >= 8) { /* ver(2) + GK(4) + PK count(2) */
> +			pBSSNode->wCSSPKCount =
> +				*((unsigned short *)&(pRSN->abyRSN[4]));
>  			j = 0;
>  			pbyOUI = &(pRSN->abyRSN[6]);
>  
> -			for (i = 0; (i < pBSSNode->wCSSPKCount) && (j < sizeof(pBSSNode->abyCSSPK)/sizeof(unsigned char)); i++) {
> -				if (pRSN->len >= 8+i*4+4) { // ver(2)+GK(4)+PKCnt(2)+PKS(4*i)
> +			for (i = 0; (i < pBSSNode->wCSSPKCount)
> +				     && (j < sizeof(pBSSNode->abyCSSPK)/sizeof(unsigned char));
> +			     i++) {


This for loop is a quite nasty.  It would be better to check "j"
separately inside the loop.  There should be spaces around the divide
operation but really the "j" condition should just be:

	j < ARRAY_SIZE(pBSSNode->abyCSSPK)

> +				/* ver(2)+GK(4)+PKCnt(2)+PKS(4*i) */
> +				if (pRSN->len >= 8+i*4+4) {
>  					if (!memcmp(pbyOUI, abyOUIGK, 4)) {
> -						pBSSNode->abyCSSPK[j++] = WLAN_11i_CSS_USE_GROUP;
> +						pBSSNode->abyCSSPK[j++] =
> +							WLAN_11i_CSS_USE_GROUP;
>  						bUseGK = true;
> -					} else if (!memcmp(pbyOUI, abyOUIWEP40, 4)) {
> -						// Invalid CSS, continue to parsing
> -					} else if (!memcmp(pbyOUI, abyOUITKIP, 4)) {
> -						if (pBSSNode->byCSSGK != WLAN_11i_CSS_CCMP)
> -							pBSSNode->abyCSSPK[j++] = WLAN_11i_CSS_TKIP;
> -						else
> -							; // Invalid CSS, continue to parsing
> -					} else if (!memcmp(pbyOUI, abyOUICCMP, 4)) {
> -						pBSSNode->abyCSSPK[j++] = WLAN_11i_CSS_CCMP;
> -					} else if (!memcmp(pbyOUI, abyOUIWEP104, 4)) {
> -						// Invalid CSS, continue to parsing
> -					} else {
> -						// any vendor checks here
> -						pBSSNode->abyCSSPK[j++] = WLAN_11i_CSS_UNKNOWN;
> -					}
> +					} else if (!memcmp(pbyOUI, abyOUIWEP40, 4))
> +						; /* Invalid CSS, continue to parsing */
> +					else if ((!memcmp(pbyOUI, abyOUITKIP, 4))
> +						 && (pBSSNode->byCSSGK != WLAN_11i_CSS_CCMP))
> +						pBSSNode->abyCSSPK[j++] =
> +							WLAN_11i_CSS_TKIP;
> +					else if (!memcmp(pbyOUI, abyOUICCMP, 4))
> +						pBSSNode->abyCSSPK[j++] =
> +							WLAN_11i_CSS_CCMP;
> +					else if (!memcmp(pbyOUI, abyOUIWEP104, 4))
> +						; /* Invalid CSS, continue to parsing */
> +					else
> +						/* any vendor checks here */
> +						pBSSNode->abyCSSPK[j++] =
> +							WLAN_11i_CSS_UNKNOWN;
>  					pbyOUI += 4;

You have changed the logic here without meaning to.  Can you spot the
bug?

I don't like the way the lines are broken at all.  This is a randomly
indented text jumble.  *SPLATCH!*.

If you need to break up a compound condition put the "&&" at the end
of the first line instead of at the start of the second line.
Multi-line indents get curly braces for readability, even though they
are not needed for syntax reasons.  So it would be:

				} else {
					/* any vendor checks here */
					pBSSNode->abyCSSPK[j++] = WLAN_11i_CSS_UNKNOWN;
				}

I know that is over the 80 character limit, so checkpatch.pl will
complain but checkpatch.pl is a perl script and not the king of the
world.  Don't make the code worse just to please a perl script.

The real reason we are going past the end of the line here is because
the code is indented 40 characters to begin with.  You could bring it in
one indent level by reversing the break condition.

			if (pRSN->len < 8 + i * 4 + 4)
				break;

> -					DBG_PRT(MSG_LEVEL_DEBUG, KERN_INFO "abyCSSPK[%d]: %X\n", j-1, pBSSNode->abyCSSPK[j-1]);
> +					DBG_PRT(MSG_LEVEL_DEBUG, KERN_INFO
> +						"abyCSSPK[%d]: %X\n", j-1,
> +						pBSSNode->abyCSSPK[j-1]);
>  				} else
>  					break;
> -			} //for
> +			} /* for */
                          ^^^^^^^^^
Delete these nonsense comments.

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/