linux-kernel - Re: [PATCH] I checked with chechpatch.pl the archive in drivers/staging/bcm/CmHost.c , and cleanup the Errors and warnings, I didn't change any function, only code style.

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]

Message-ID: <20111109210056.GV4751@mwanda>
Date:	Thu, 10 Nov 2011 00:00:56 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Tresca Agustin <treskita@...il.com>
Cc:	gregkh@...e.de, wfp5p@...ginia.edu, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] I checked with chechpatch.pl the archive in
 drivers/staging/bcm/CmHost.c , and cleanup the Errors and warnings, I didn't
 change any function, only code style.

Hi Tresca,

Welcome.  :)

1) This patch combines way too many things into one patch.  Only do
one logical thing per patch.

2) When you're writing the changelog write a one line summary at the
top, leave a blank line, and then write the changelog.  You didn't
have the summary so the Subject of the email was messed up.

3) No signed-off-by line.

On Mon, Nov 07, 2011 at 08:55:43PM -0300, Tresca Agustin wrote:
> From: Tresca Agustin <atresca@...esca-system.(none)>
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The email address is wrong here.

>
> ---
>  drivers/staging/bcm/CmHost.c | 3794 ++++++++++++++++++++++++-----------------
>  1 files changed, 2221 insertions(+), 1573 deletions(-)
> 
> diff --git a/drivers/staging/bcm/CmHost.c b/drivers/staging/bcm/CmHost.c
> index c0ee95a..8819c52 100644
> --- a/drivers/staging/bcm/CmHost.c
> +++ b/drivers/staging/bcm/CmHost.c
> @@ -4,18 +4,17 @@
>  *	Management.
>  ************************************************************/
>  
> -//#define CONN_MSG
> +/*#define CONN_MSG*/
>  #include "headers.h"
>  
> -typedef enum _E_CLASSIFIER_ACTION
> -{
> -	eInvalidClassifierAction,
> -	eAddClassifier,
> -	eReplaceClassifier,
> -	eDeleteClassifier
> -}E_CLASSIFIER_ACTION;
> +enum _E_CLASSIFIER_ACTION {
> +	eInvalidClsfrAction,
> +	eAddClsfr,
> +	eRplcClsfr,
> +	eDelClsfr
> +}; /*_E_CLASSIFIER_ACTION*/

These names are even worse than the original.

>  
> -static ULONG GetNextTargetBufferLocation(PMINI_ADAPTER Adapter,B_UINT16 tid);
> +static ULONG GetNextTargetBufferLocation(PMINI_ADAPTER Adapter, B_UINT16 tid);
>  
>  /************************************************************
>  * Function	  -	SearchSfid
> @@ -24,18 +23,18 @@ static ULONG GetNextTargetBufferLocation(PMINI_ADAPTER Adapter,B_UINT16 tid);
>  *				specified SFID as input parameter.
>  *
>  * Parameters  -	Adapter: Pointer to the Adapter structure
> -*			   	uiSfid : Given SFID for matching
> +*				uiSfid : Given SFID for matching
>  *
>  * Returns	  - Queue index for this SFID(If matched)
>  				Else Invalid Queue Index(If Not matched)
>  ************************************************************/
> -INT SearchSfid(PMINI_ADAPTER Adapter,UINT uiSfid)
> +INT SearchSfid(PMINI_ADAPTER Adapter, UINT uiSfid)
>  {
> -	INT 	iIndex=0;
> -	for(iIndex=(NO_OF_QUEUES-1); iIndex>=0; iIndex--)
> -		if(Adapter->PackInfo[iIndex].ulSFID==uiSfid)
> +	INT iIndex = 0;

blank line goes here.
INT should be changed to "int"
No need to initialize iIndex.

> +	for (iIndex = (NO_OF_QUEUES - 1); iIndex >= 0; iIndex--)
> +		if (Adapter->PackInfo[iIndex].ulSFID == uiSfid)
>  			return iIndex;

But curly braces around multi-line indent blocks for style reasons
even though it's not needed for semantic reasons.

> -	return NO_OF_QUEUES+1;
> +	return NO_OF_QUEUES + 1;

Anyway, this whole thing needs to be broken up into smaller logical
patches and resent.  Like maybe change fix add spaces around the
operators in one patch or something like that.  Fix one class of
checkpatch.pl warning at a time.

Don't worry that your first patch was not accepted.  Everyone's first
patch gets rejected.  :P

regards,
dan carpenter


Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)