[dpdk-dev,v2] librte_cmdline: fix parsing initialisation

  From: "Bernard.Iremonger" <Bernard.iremonger@intel.com>

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 <bernard.iremonger@intel.com>
Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

---

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(-)
  

Hi Adrien,

On Thu, 15 Jun 2017 12:15:48 +0200, Adrien Mazarguil <adrien.mazarguil@6wind.com> wrote:
> From: "Bernard.Iremonger" <Bernard.iremonger@intel.com>
> 
> 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 <bernard.iremonger@intel.com>
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> 
> ---
> 
> 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
  

Adding a cover letter to keep the same title for that thread, since I took
over the original patch from Bernard, modified it somewhat and made a couple
of additional fixes on top of it.

Olivier's comment [1] and Bernard's feedback about dynamic tokens made me
realize how the interface itself was not only difficult to use but also
broken.

This series starts by fixing the root cause of the original issue reported
by Bernard, then simplifies the interface itself and its only user (testpmd)
and finally fixes an unrelated issue found in testpmd. These fixes target
all stable releases since 17.02.

[1] http://dpdk.org/ml/archives/dev/2017-June/068605.html

Adrien Mazarguil (3):
  cmdline: fix dynamic tokens initialization
  cmdline: fix dynamic tokens interface
  app/testpmd: fix token matching in flow command

 app/test-pmd/cmdline_flow.c        | 36 +++++++-------
 lib/librte_cmdline/cmdline_parse.c | 85 ++++++++++-----------------------
 lib/librte_cmdline/cmdline_parse.h | 50 +++++++++++++++----
 3 files changed, 85 insertions(+), 86 deletions(-)
  

10/07/2017 15:09, Adrien Mazarguil:
> Adding a cover letter to keep the same title for that thread, since I took
> over the original patch from Bernard, modified it somewhat and made a couple
> of additional fixes on top of it.
> 
> Olivier's comment [1] and Bernard's feedback about dynamic tokens made me
> realize how the interface itself was not only difficult to use but also
> broken.
> 
> This series starts by fixing the root cause of the original issue reported
> by Bernard, then simplifies the interface itself and its only user (testpmd)
> and finally fixes an unrelated issue found in testpmd. These fixes target
> all stable releases since 17.02.
> 
> [1] http://dpdk.org/ml/archives/dev/2017-June/068605.html
> 
> Adrien Mazarguil (3):
>   cmdline: fix dynamic tokens initialization
>   cmdline: fix dynamic tokens interface
>   app/testpmd: fix token matching in flow command

Applied, thanks