From patchwork Tue Nov 13 05:29:55 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Chen Gang X-Patchwork-Id: 198547 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id AD5F62C00B0 for ; Tue, 13 Nov 2012 16:29:00 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752580Ab2KMF25 (ORCPT ); Tue, 13 Nov 2012 00:28:57 -0500 Received: from intranet.asianux.com ([58.214.24.6]:44502 "EHLO intranet.asianux.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751627Ab2KMF25 (ORCPT ); Tue, 13 Nov 2012 00:28:57 -0500 Received: by intranet.asianux.com (Postfix, from userid 103) id 4C1B918402D6; Tue, 13 Nov 2012 13:28:55 +0800 (CST) X-Spam-Score: -100.7 X-Spam-Checker-Version: SpamAssassin 3.1.9 (2007-02-13) on intranet.asianux.com X-Spam-Level: X-Spam-Status: No, score=-100.7 required=5.0 tests=AWL,BAYES_00, RATWARE_GECKO_BUILD, TW_VN, USER_IN_WHITELIST autolearn=no version=3.1.9 Received: from [10.1.0.143] (unknown [219.143.36.82]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by intranet.asianux.com (Postfix) with ESMTP id EE53F184026D; Tue, 13 Nov 2012 13:28:54 +0800 (CST) Message-ID: <50A1DAD3.40305@asianux.com> Date: Tue, 13 Nov 2012 13:29:55 +0800 From: Chen Gang User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120911 Thunderbird/15.0.1 MIME-Version: 1.0 To: David Miller CC: sam@ravnborg.org, sparclinux@vger.kernel.org Subject: Re: [PATCH] arch/sparc: additional len check in loop for prom_getbootargs References: <20121108163348.GA7729@merkur.ravnborg.org> <50A1BDB1.5050003@asianux.com> <50A1D099.9090707@asianux.com> <20121113.000618.708059907927180435.davem@davemloft.net> In-Reply-To: <20121113.000618.708059907927180435.davem@davemloft.net> Sender: sparclinux-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: sparclinux@vger.kernel.org 于 2012年11月13日 13:06, David Miller 写道: > From: Chen Gang > Date: Tue, 13 Nov 2012 12:46:17 +0800 > >> 1) although I agree with what you said. >> >> 2) but for the patch code: >> after "*cp++ = ' ';", also need "break checking" for "for (; ;)" outside of "while()" >> >> 3) please check, and still it will be better to notice coding style, too. > > Why is every single one of your replies a set of 3 bullet points? You > aren't giving a PowerPoint presentation. > > Just talk and discuss things normally, using sentences and paragraphs > for your structure. > > Everything you say and do looks very unnatural, awkward, and forced. > > Excuse me,, my English is not quite well, just as you said (I need improving) after "*cp++ = ' ';" at line 42, also need "break checking" for "for (; ;)" outside of "while()" the reply from sam not check it, (the patch what I sent check it). this is the original source code of function prom_getbootargs in arch/sparc/prom/bootstr_32.c. ------------------------------------------------------------------------------------------------ 27 case PROM_V0: 28 cp = barg_buf; 29 /* Start from 1 and go over fd(0,0,0)kernel */ 30 for(iter = 1; iter < 8; iter++) { 31 arg = (*(romvec->pv_v0bootargs))->argv[iter]; 32 if (arg == NULL) 33 break; 34 while(*arg != 0) { 35 /* Leave place for space and null. */ 36 if(cp >= barg_buf + BARG_LEN-2){ 37 /* We might issue a warning here. */ 38 break; 39 } 40 *cp++ = *arg++; 41 } 42 *cp++ = ' '; 43 } 44 *cp = 0; 45 break; --------------------------------------------------------------------------------------------------- this is the original reply from sam: -------------------------------------------------------------------------------------------------- Hi Cheng. On Thu, Nov 08, 2012 at 11:41:39AM +0800, Chen Gang wrote: > > when cp >= barg_buf + BARG_LEN-2, it only break internel loop (while) > but outside loop (for) still has effect, and "*cp++ = ' '" repeating > so need additional checking for it. > > > Signed-off-by: Chen Gang I wonder how you found this bug?!?! Anyway please consider this alternative fix: Adding the conditional inside the while loop makes the logic simpler. And the patch actually deletes more lines than it adds. And please take care to follow coding style too. In particular spaces around operators. The old code does not follow coding style - but this is no excuse. Note - the above is not even build tested! If you use the above code-snippet you can add my: Acked-by: Sam Ravnborg Sam ------------------------------------------------------------------------------------------ diff --git a/arch/sparc/prom/bootstr_32.c b/arch/sparc/prom/bootstr_32.c index f5ec32e..4ce602f 100644 --- a/arch/sparc/prom/bootstr_32.c +++ b/arch/sparc/prom/bootstr_32.c @@ -31,14 +31,10 @@ prom_getbootargs(void) arg = (*(romvec->pv_v0bootargs))->argv[iter]; if (arg == NULL) break; - while(*arg != 0) { - /* Leave place for space and null. */ - if(cp >= barg_buf + BARG_LEN-2){ - /* We might issue a warning here. */ - break; - } + while (*arg != 0 && cp < (barg_buf + BARG_LEN - 2)) *cp++ = *arg++; - } + + /* Append trailing space + null */ *cp++ = ' '; } *cp = 0;