From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olivier Matz Subject: Re: [PATCH v2] librte_cmdline: fix parsing initialisation Date: Fri, 23 Jun 2017 11:04:19 +0200 Message-ID: <20170623110419.43b9ac89@platinum> References: <1497348447-1167-1-git-send-email-bernard.iremonger@intel.com> <327b5be12221f51fbf3a6d8e9d155de786992388.1497521374.git.adrien.mazarguil@6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: Bernard Iremonger , dev@dpdk.org, stable@dpdk.org To: Adrien Mazarguil Return-path: Received: from mail-wr0-f173.google.com (mail-wr0-f173.google.com [209.85.128.173]) by dpdk.org (Postfix) with ESMTP id D707C2C00 for ; Fri, 23 Jun 2017 11:04:22 +0200 (CEST) Received: by mail-wr0-f173.google.com with SMTP id r103so56809582wrb.0 for ; Fri, 23 Jun 2017 02:04:22 -0700 (PDT) In-Reply-To: <327b5be12221f51fbf3a6d8e9d155de786992388.1497521374.git.adrien.mazarguil@6wind.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Adrien, On Thu, 15 Jun 2017 12:15:48 +0200, Adrien Mazarguil wrote: > From: "Bernard.Iremonger" > > The dyn_tokens array is initialised at the beginning of the cmdline_parse > function. However when the inst_num variable is incremented later in the > function the dyn_tokens array is not reinitialised so the tokens from the > previous command are used. > > The solution is to initialise the dyn_tokens array in all while (inst) > loops, before calling match_inst(). > > Fixes: 4fffc05a2b2c ("cmdline: support dynamic tokens") > > CC: stable@dpdk.org > Signed-off-by: Bernard Iremonger > Signed-off-by: Adrien Mazarguil > > --- > > Nice catch Bernard. This issue can be seen when implementing several > flow-like commands in testpmd. > > While testing your original patch though, it appeared that it did not fully > address the issue, as completion remained partially broken. Actually all > match_inst() calls should be preceded by a memset(), so here's an updated > version instead of requesting you to do these changes. > --- > lib/librte_cmdline/cmdline_parse.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/lib/librte_cmdline/cmdline_parse.c b/lib/librte_cmdline/cmdline_parse.c > index b814880..c1d9f23 100644 > --- a/lib/librte_cmdline/cmdline_parse.c > +++ b/lib/librte_cmdline/cmdline_parse.c > @@ -276,7 +276,6 @@ cmdline_parse(struct cmdline *cl, const char * buf) > return CMDLINE_PARSE_BAD_ARGS; > > ctx = cl->ctx; > - memset(&dyn_tokens, 0, sizeof(dyn_tokens)); > > /* > * - look if the buffer contains at least one line > @@ -321,6 +320,7 @@ cmdline_parse(struct cmdline *cl, const char * buf) > debug_printf("INST %d\n", inst_num); > > /* fully parsed */ > + memset(&dyn_tokens, 0, sizeof(dyn_tokens)); > tok = match_inst(inst, buf, 0, tmp_result.buf, > sizeof(tmp_result.buf), &dyn_tokens); > > @@ -400,7 +400,6 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state, > > debug_printf("%s called\n", __func__); > memset(&token_hdr, 0, sizeof(token_hdr)); > - memset(&dyn_tokens, 0, sizeof(dyn_tokens)); > > /* count the number of complete token to parse */ > for (i=0 ; buf[i] ; i++) { > @@ -423,6 +422,7 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state, > inst = ctx[inst_num]; > while (inst) { > /* parse the first tokens of the inst */ > + memset(&dyn_tokens, 0, sizeof(dyn_tokens)); > if (nb_token && > match_inst(inst, buf, nb_token, NULL, 0, > &dyn_tokens)) > @@ -530,6 +530,7 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state, > /* we need to redo it */ > inst = ctx[inst_num]; > > + memset(&dyn_tokens, 0, sizeof(dyn_tokens)); > if (nb_token && > match_inst(inst, buf, nb_token, NULL, 0, &dyn_tokens)) > goto next2; It looks you call memset() before each call to match_inst(). So, I have 2 questions: - why not putting this memset() at the beginning of match_inst()? - does the following test at the beginning of match_inst() still make sense? if (!token_p && dyn_tokens && inst->f) { if (!(*dyn_tokens)[0]) /* <<<<<<<<< here */ inst->f(&(*dyn_tokens)[0], NULL, dyn_tokens); token_p = (*dyn_tokens)[0]; } Thanks, Olivier