From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 122B7200 for ; Fri, 8 Dec 2017 16:26:23 +0100 (CET) Received: from lfbn-1-6068-189.w90-110.abo.wanadoo.fr ([90.110.3.189] helo=droids-corp.org) by mail.droids-corp.org with esmtpsa (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1eNKdg-0006PK-VS; Fri, 08 Dec 2017 16:32:38 +0100 Received: by droids-corp.org (sSMTP sendmail emulation); Fri, 08 Dec 2017 16:26:14 +0100 Date: Fri, 8 Dec 2017 16:26:14 +0100 From: Olivier MATZ To: Adrien Mazarguil Cc: Xueming Li , Wenzhuo Lu , dev@dpdk.org Message-ID: <20171208152612.75nttsciba3imn6x@platinum> References: <20171115155402.9967-1-xuemingl@mellanox.com> <20171208070244.24094-1-xuemingl@mellanox.com> <20171208122726.GT4062@6wind.com> <20171208135114.wzkoidddxudbzyuy@platinum> <20171208150433.GU4062@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171208150433.GU4062@6wind.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v1] lib/cmdline: init parse result memory X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 08 Dec 2017 15:26:24 -0000 Hi, On Fri, Dec 08, 2017 at 04:04:33PM +0100, Adrien Mazarguil wrote: > On Fri, Dec 08, 2017 at 02:51:15PM +0100, Olivier MATZ wrote: > > On Fri, Dec 08, 2017 at 01:27:26PM +0100, Adrien Mazarguil wrote: > > > On Fri, Dec 08, 2017 at 03:02:44PM +0800, Xueming Li wrote: > > > > Initialize binary result memory before parsing to avoid garbage in > > > > parsing result. > > > > > > > > Signed-off-by: Xueming Li > > > > > > Since you chose to move the break statement, maybe the original commit > > > mentioned in my previous message (9b3fbb051d2e "cmdline: fix parsing") can > > > be reverted afterward? I think it makes tmp_result redundant. > > > > > > Wenzhuo, as the author of that commit, can you confirm? > > > > > > Olivier, no problem with breaking the loop immediately after the first > > > successful match_inst() call instead of the last one? (I don't see why it > > > would be an issue but I may have missed something) > > > > Moving the break will change the behavior, it will never detect > > ambiguous commands (i.e when several commands match the same input). > > So I think we should not do it. > > > > IMO, only the memset() is enough. > > I agree it should be, however as reported by Xueming doing so somehow breaks > the flow command. In my previous reply I assumed that was caused by clearing > the result buffer of prior successful calls in cmdline_parse(), I just > checked and it appears not to be the case. Wenzhuo's patch works fine. > > The root cause is actually the flow command stores internal buffer addresses > in the output buffer, which happens to be tmp_result.buf since commit > 9b3fbb051d2e. > > When match_inst() returns successfully, tmp_result.buf is copied to > result.buf and the contents of tmp_result.buf are discarded by the next call > to match_inst(). Addresses stored inside result.buf still refer to locations > inside tmp_result.buf, memset()'ing that region only makes that bug manifest > itself. > > Another suggestion to address the underlying issue before adding memset(): > > diff --git a/lib/librte_cmdline/cmdline_parse.c b/lib/librte_cmdline/cmdline_parse.c > index 205f243..15a3482 100644 > --- a/lib/librte_cmdline/cmdline_parse.c > +++ b/lib/librte_cmdline/cmdline_parse.c > @@ -254,7 +254,7 @@ cmdline_parse(struct cmdline *cl, const char * buf) > union { > char buf[CMDLINE_PARSE_RESULT_BUFSIZE]; > long double align; /* strong alignment constraint for buf */ > - } result, tmp_result; > + } result, result_ok; > void (*f)(void *, struct cmdline *, void *) = NULL; > void *data = NULL; > int comment = 0; > @@ -315,16 +315,13 @@ cmdline_parse(struct cmdline *cl, const char * buf) > debug_printf("INST %d\n", inst_num); > > /* fully parsed */ > - tok = match_inst(inst, buf, 0, tmp_result.buf, > - sizeof(tmp_result.buf)); > + tok = match_inst(inst, buf, 0, result.buf, sizeof(result.buf)); > > if (tok > 0) /* we matched at least one token */ > err = CMDLINE_PARSE_BAD_ARGS; > > else if (!tok) { > debug_printf("INST fully parsed\n"); > - memcpy(&result, &tmp_result, > - sizeof(result)); > /* skip spaces */ > while (isblank2(*curbuf)) { > curbuf++; > @@ -344,6 +341,7 @@ cmdline_parse(struct cmdline *cl, const char * buf) > break; > } > } > + result_ok = result; > } > > inst_num ++; > @@ -352,6 +350,7 @@ cmdline_parse(struct cmdline *cl, const char * buf) > > /* call func */ > if (f) { > + result = result_ok; > f(result.buf, cl, data); > } > In addition to the memset() at the beginning of match_inst(), it looks good to me. A particular attention should be paid to the explanation of the issue and its solution in the commit log :)