From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f194.google.com (mail-wr0-f194.google.com [209.85.128.194]) by dpdk.org (Postfix) with ESMTP id 15C92199B6 for ; Fri, 8 Dec 2017 16:04:46 +0100 (CET) Received: by mail-wr0-f194.google.com with SMTP id k61so11087880wrc.4 for ; Fri, 08 Dec 2017 07:04:46 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=BPER6IV/q2ba6vR0NIs5fD97/tXNN2qdjwE8Lx3eVak=; b=zZBEw5t/UdGDa9+PTxkBROdxRM6kujtx79sXylNCOru6pljGLPKZRU4J/uPyyFq9ni POUPMEokpzD+r7RVGI8g23jH/AtGG15fkP97lHQtHM+1Djo8gdtOtgXldesxLaO7l+cR h4evOV79XMU8L3D2jivcQEtwedi0BtC6k+YPU/K+SqJUqSOGk21JJfn//9kIAswNcHz6 SRlKt9HH9LHPimBOBvB5TMyCTEt8t7PdsSRfV0fdDnv4EdSi1229hob9nqlU6LagdhNv 8wLkGRtEQwt9KYlClrg0e7VDDyKt7oI1cbofb87xOOWiCZb8LEJ0+L/Z54coXb4Opyaw Kr7Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=BPER6IV/q2ba6vR0NIs5fD97/tXNN2qdjwE8Lx3eVak=; b=J2NKaKiVLpZkmxa4yvhlLGcmkP1gO9UiD0edVSCJD5FPH4TL1k6/xemvATEPDtmIKi pB5Q1O8JPbEzXfh9b9Lu3PTxmi83hYrNU6/Ks2jC+iiUnqrftEZHKrQuzoaMaBN26ucZ tzPAujCMFIz+Fxk+H00Y1rgQRpjBjwrwfuAUbCgn6v0zpf1sF/a/1nnxxXHVtVQsLZGi sfYWNF7KZvrwFV0h5oky2FfiGHrsi7uAAcJus9UG3/4wyUfJNr6Wt/ivefWmIZ8VOjwP Xqivpqwh92SfXu2I6iMwwhkui0LIoTimwtPI/JafFRLoDDHNq6mbpMKS33BL4IZ9uu/e ipnw== X-Gm-Message-State: AJaThX6/gNrpXgnBmORLTCFbbeU/zX4TIpXSOA67v8wYl9sK+nf69yfz RyBjEWJarltJw4npYaiC1tje7Bcf X-Google-Smtp-Source: AGs4zMa7qRykiBIosxET2R8Pw264T9ilJ3Jnl0Ayc+gEYlk/vQ9sHsqEnGFoXRC2vZunoqEoII6iGA== X-Received: by 10.223.134.5 with SMTP id 5mr24003987wrv.186.1512745485792; Fri, 08 Dec 2017 07:04:45 -0800 (PST) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id n17sm1137956wmd.22.2017.12.08.07.04.44 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 08 Dec 2017 07:04:44 -0800 (PST) Date: Fri, 8 Dec 2017 16:04:33 +0100 From: Adrien Mazarguil To: Olivier MATZ Cc: Xueming Li , Wenzhuo Lu , dev@dpdk.org Message-ID: <20171208150433.GU4062@6wind.com> References: <20171115155402.9967-1-xuemingl@mellanox.com> <20171208070244.24094-1-xuemingl@mellanox.com> <20171208122726.GT4062@6wind.com> <20171208135114.wzkoidddxudbzyuy@platinum> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171208135114.wzkoidddxudbzyuy@platinum> 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:04:46 -0000 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); } -- Adrien Mazarguil 6WIND