[dpdk-dev] [PATCH] librte_cmdline: fix parsing initialisation

[dpdk-dev] [PATCH] librte_cmdline: fix parsing initialisation

[Bernard Iremonger]

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
the while(inst) loop.

Fixes: 4fffc05a2b2c ("cmdline: support dynamic tokens")

CC: stable@dpdk.org
Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
 lib/librte_cmdline/cmdline_parse.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_cmdline/cmdline_parse.c b/lib/librte_cmdline/cmdline_parse.c
index b814880..9ee28b8 100644
--- a/lib/librte_cmdline/cmdline_parse.c
+++ b/lib/librte_cmdline/cmdline_parse.c
@@ -276,7 +276,6 @@
 		return CMDLINE_PARSE_BAD_ARGS;
 
 	ctx = cl->ctx;
-	memset(&dyn_tokens, 0, sizeof(dyn_tokens));
 
 	/*
 	 * - look if the buffer contains at least one line
@@ -319,6 +318,7 @@
 	inst = ctx[inst_num];
 	while (inst) {
 		debug_printf("INST %d\n", inst_num);
+		memset(&dyn_tokens, 0, sizeof(dyn_tokens));
 
 		/* fully parsed */
 		tok = match_inst(inst, buf, 0, tmp_result.buf,
-- 
1.9.1

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

[Adrien Mazarguil]

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;
-- 
2.1.4

Re: [dpdk-dev] [PATCH v2] librte_cmdline: fix parsing initialisation

[Olivier Matz]

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

[dpdk-dev] [PATCH v3 0/3] librte_cmdline: fix parsing initialisation

[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

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

-- 
2.1.4

[dpdk-dev] [PATCH v3 1/3] cmdline: fix dynamic tokens initialization

[Adrien Mazarguil]

To avoid redundant calls to the token-generating function and provide it
with helpful context, a temporary fixed-size array allocated on the stack
of cmdline_parse() and cmdline_complete() keeps the address of preceding
tokens for the command instance being processed (cmdline_parse_inst_t).

Like the static tokens array in cmdline_parse_inst_t, it must be
NULL-terminated, however this is not properly enforced as it is initialized
once at the beginning of each function and a NULL terminator is never
appended once replaced with generated tokens.

When several commands rely on dynamic tokens, subsequent ones inherit an
already initialized array whose tokens are not regenerated, which causes
various issues such as mixed and repeated tokens from the first command.

Enforcing NULL termination of the dynamic tokens array and reinitializing
its first entry at each iteration solves these issues. Doing so is also
less expensive than a full memset() at each iteration.

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>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
---
 lib/librte_cmdline/cmdline_parse.c | 78 +++++++++++++++------------------
 1 file changed, 36 insertions(+), 42 deletions(-)

diff --git a/lib/librte_cmdline/cmdline_parse.c b/lib/librte_cmdline/cmdline_parse.c
index b814880..67e452d 100644
--- a/lib/librte_cmdline/cmdline_parse.c
+++ b/lib/librte_cmdline/cmdline_parse.c
@@ -139,6 +139,32 @@ nb_common_chars(const char * s1, const char * s2)
 	return i;
 }
 
+/** Retrieve either static or dynamic token at a given index. */
+static cmdline_parse_token_hdr_t *
+get_token(cmdline_parse_inst_t *inst,
+	  unsigned int index,
+	  cmdline_parse_token_hdr_t
+		*(*dyn_tokens)[CMDLINE_PARSE_DYNAMIC_TOKENS])
+{
+	/* check presence of static tokens first */
+	if (inst->tokens[0] || !inst->f)
+		return inst->tokens[index];
+	/* for dynamic tokens, make sure index does not overflow */
+	if (index >= CMDLINE_PARSE_DYNAMIC_TOKENS - 1)
+		return NULL;
+	/* in case token is already generated, return it */
+	if ((*dyn_tokens)[index])
+		return (*dyn_tokens)[index];
+	/* generate token */
+	inst->f(&(*dyn_tokens)[index], NULL, dyn_tokens);
+	/* return immediately if there are no more tokens to expect */
+	if (!(*dyn_tokens)[index])
+		return NULL;
+	/* terminate list with a NULL entry */
+	(*dyn_tokens)[index + 1] = NULL;
+	return (*dyn_tokens)[index];
+}
+
 /**
  * try to match the buffer with an instruction (only the first
  * nb_match_token tokens if != 0). Return 0 if we match all the
@@ -156,12 +182,7 @@ match_inst(cmdline_parse_inst_t *inst, const char *buf,
 	int n = 0;
 	struct cmdline_token_hdr token_hdr;
 
-	token_p = inst->tokens[token_num];
-	if (!token_p && dyn_tokens && inst->f) {
-		if (!(*dyn_tokens)[0])
-			inst->f(&(*dyn_tokens)[0], NULL, dyn_tokens);
-		token_p = (*dyn_tokens)[0];
-	}
+	token_p = get_token(inst, token_num, dyn_tokens);
 	if (token_p)
 		memcpy(&token_hdr, token_p, sizeof(token_hdr));
 
@@ -203,17 +224,7 @@ match_inst(cmdline_parse_inst_t *inst, const char *buf,
 		buf += n;
 
 		token_num ++;
-		if (!inst->tokens[0]) {
-			if (token_num < (CMDLINE_PARSE_DYNAMIC_TOKENS - 1)) {
-				if (!(*dyn_tokens)[token_num])
-					inst->f(&(*dyn_tokens)[token_num],
-						NULL,
-						dyn_tokens);
-				token_p = (*dyn_tokens)[token_num];
-			} else
-				token_p = NULL;
-		} else
-			token_p = inst->tokens[token_num];
+		token_p = get_token(inst, token_num, dyn_tokens);
 		if (token_p)
 			memcpy(&token_hdr, token_p, sizeof(token_hdr));
 	}
@@ -276,7 +287,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
@@ -317,6 +327,7 @@ cmdline_parse(struct cmdline *cl, const char * buf)
 
 	/* parse it !! */
 	inst = ctx[inst_num];
+	dyn_tokens[0] = NULL;
 	while (inst) {
 		debug_printf("INST %d\n", inst_num);
 
@@ -354,6 +365,7 @@ cmdline_parse(struct cmdline *cl, const char * buf)
 
 		inst_num ++;
 		inst = ctx[inst_num];
+		dyn_tokens[0] = NULL;
 	}
 
 	/* call func */
@@ -400,7 +412,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++) {
@@ -421,6 +432,7 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state,
 		nb_non_completable = 0;
 
 		inst = ctx[inst_num];
+		dyn_tokens[0] = NULL;
 		while (inst) {
 			/* parse the first tokens of the inst */
 			if (nb_token &&
@@ -429,18 +441,7 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state,
 				goto next;
 
 			debug_printf("instruction match\n");
-			if (!inst->tokens[0]) {
-				if (nb_token <
-				    (CMDLINE_PARSE_DYNAMIC_TOKENS - 1)) {
-					if (!dyn_tokens[nb_token])
-						inst->f(&dyn_tokens[nb_token],
-							NULL,
-							&dyn_tokens);
-					token_p = dyn_tokens[nb_token];
-				} else
-					token_p = NULL;
-			} else
-				token_p = inst->tokens[nb_token];
+			token_p = get_token(inst, nb_token, &dyn_tokens);
 			if (token_p)
 				memcpy(&token_hdr, token_p, sizeof(token_hdr));
 
@@ -494,6 +495,7 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state,
 			debug_printf("next\n");
 			inst_num ++;
 			inst = ctx[inst_num];
+			dyn_tokens[0] = NULL;
 		}
 
 		debug_printf("total choices %d for this completion\n",
@@ -526,6 +528,7 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state,
 
 	inst_num = 0;
 	inst = ctx[inst_num];
+	dyn_tokens[0] = NULL;
 	while (inst) {
 		/* we need to redo it */
 		inst = ctx[inst_num];
@@ -534,17 +537,7 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state,
 		    match_inst(inst, buf, nb_token, NULL, 0, &dyn_tokens))
 			goto next2;
 
-		if (!inst->tokens[0]) {
-			if (nb_token < (CMDLINE_PARSE_DYNAMIC_TOKENS - 1)) {
-				if (!dyn_tokens[nb_token])
-					inst->f(&dyn_tokens[nb_token],
-						NULL,
-						&dyn_tokens);
-				token_p = dyn_tokens[nb_token];
-			} else
-				token_p = NULL;
-		} else
-			token_p = inst->tokens[nb_token];
+		token_p = get_token(inst, nb_token, &dyn_tokens);
 		if (token_p)
 			memcpy(&token_hdr, token_p, sizeof(token_hdr));
 
@@ -617,6 +610,7 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state,
 	next2:
 		inst_num ++;
 		inst = ctx[inst_num];
+		dyn_tokens[0] = NULL;
 	}
 	return 0;
 }
-- 
2.1.4

[dpdk-dev] [PATCH v3 2/3] cmdline: fix dynamic tokens interface

[Adrien Mazarguil]

Support for dynamic tokens was added in order to implement the flow command
in testpmd, for which static tokens were not versatile enough due to the
large number of possible parameter combinations.

However, due to its reliance on a temporary array to store dynamic tokens,
this interface suffers from various limitations that need to be addressed
in order to implement more commands in the future:

- The maximum number of dynamic tokens is determined at compilation time
  (CMDLINE_PARSE_DYNAMIC_TOKENS). The larger this value, the more stack
  space is wasted (one pointer per potential token, i.e. 1kB of stack space
  on 64-bit architectures with the default value).

- This temporary array is actually a cache in which entries already present
  are not regenerated. This behavior is not documented, which makes dynamic
  tokens practically unusable by applications as they do not know which
  token is current.

- The cache does not really reduce the number of function calls needed to
  retrieve tokens, it was mainly deemed useful to provide context about
  other tokens to the generator callback.

- Like testpmd, most users will likely use repeated pointers to a fixed
  token header structure (cmdline_token_hdr_t), with internal context-aware
  callbacks that do not need to look at other entries; knowing the index of
  the current token is enough.

Getting rid of the temporary array and properly documenting usage of the
token generator callback greatly simplifies this interface.

Fixes: 4fffc05a2b2c ("cmdline: support dynamic tokens")
Fixes: 19c90af6285c ("app/testpmd: add flow command")
Cc: stable@dpdk.org

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
Cc: Bernard Iremonger <bernard.iremonger@intel.com>
---
 app/test-pmd/cmdline_flow.c        | 15 ++------
 lib/librte_cmdline/cmdline_parse.c | 63 ++++++++++-----------------------
 lib/librte_cmdline/cmdline_parse.h | 50 ++++++++++++++++++++------
 3 files changed, 60 insertions(+), 68 deletions(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 25d5982..9533df1 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -224,7 +224,6 @@ struct context {
 	enum index prev; /**< Index of the last token seen. */
 	int next_num; /**< Number of entries in next[]. */
 	int args_num; /**< Number of entries in args[]. */
-	uint32_t reparse:1; /**< Start over from the beginning. */
 	uint32_t eol:1; /**< EOL has been detected. */
 	uint32_t last:1; /**< No more arguments. */
 	uint16_t port; /**< Current port ID (for completions). */
@@ -2612,7 +2611,6 @@ cmd_flow_context_init(struct context *ctx)
 	ctx->prev = ZERO;
 	ctx->next_num = 0;
 	ctx->args_num = 0;
-	ctx->reparse = 0;
 	ctx->eol = 0;
 	ctx->last = 0;
 	ctx->port = 0;
@@ -2633,9 +2631,6 @@ cmd_flow_parse(cmdline_parse_token_hdr_t *hdr, const char *src, void *result,
 	int i;
 
 	(void)hdr;
-	/* Restart as requested. */
-	if (ctx->reparse)
-		cmd_flow_context_init(ctx);
 	token = &token_list[ctx->curr];
 	/* Check argument length. */
 	ctx->eol = 0;
@@ -2711,8 +2706,6 @@ cmd_flow_complete_get_nb(cmdline_parse_token_hdr_t *hdr)
 	int i;
 
 	(void)hdr;
-	/* Tell cmd_flow_parse() that context must be reinitialized. */
-	ctx->reparse = 1;
 	/* Count number of tokens in current list. */
 	if (ctx->next_num)
 		list = ctx->next[ctx->next_num - 1];
@@ -2746,8 +2739,6 @@ cmd_flow_complete_get_elt(cmdline_parse_token_hdr_t *hdr, int index,
 	int i;
 
 	(void)hdr;
-	/* Tell cmd_flow_parse() that context must be reinitialized. */
-	ctx->reparse = 1;
 	/* Count number of tokens in current list. */
 	if (ctx->next_num)
 		list = ctx->next[ctx->next_num - 1];
@@ -2782,8 +2773,6 @@ cmd_flow_get_help(cmdline_parse_token_hdr_t *hdr, char *dst, unsigned int size)
 	const struct token *token = &token_list[ctx->prev];
 
 	(void)hdr;
-	/* Tell cmd_flow_parse() that context must be reinitialized. */
-	ctx->reparse = 1;
 	if (!size)
 		return -1;
 	/* Set token type and update global help with details. */
@@ -2809,12 +2798,12 @@ static struct cmdline_token_hdr cmd_flow_token_hdr = {
 /** Populate the next dynamic token. */
 static void
 cmd_flow_tok(cmdline_parse_token_hdr_t **hdr,
-	     cmdline_parse_token_hdr_t *(*hdrs)[])
+	     cmdline_parse_token_hdr_t **hdr_inst)
 {
 	struct context *ctx = &cmd_flow_context;
 
 	/* Always reinitialize context before requesting the first token. */
-	if (!(hdr - *hdrs))
+	if (!(hdr_inst - cmd_flow.tokens))
 		cmd_flow_context_init(ctx);
 	/* Return NULL when no more tokens are expected. */
 	if (!ctx->next_num && ctx->curr) {
diff --git a/lib/librte_cmdline/cmdline_parse.c b/lib/librte_cmdline/cmdline_parse.c
index 67e452d..56491ea 100644
--- a/lib/librte_cmdline/cmdline_parse.c
+++ b/lib/librte_cmdline/cmdline_parse.c
@@ -141,28 +141,17 @@ nb_common_chars(const char * s1, const char * s2)
 
 /** Retrieve either static or dynamic token at a given index. */
 static cmdline_parse_token_hdr_t *
-get_token(cmdline_parse_inst_t *inst,
-	  unsigned int index,
-	  cmdline_parse_token_hdr_t
-		*(*dyn_tokens)[CMDLINE_PARSE_DYNAMIC_TOKENS])
+get_token(cmdline_parse_inst_t *inst, unsigned int index)
 {
+	cmdline_parse_token_hdr_t *token_p;
+
 	/* check presence of static tokens first */
 	if (inst->tokens[0] || !inst->f)
 		return inst->tokens[index];
-	/* for dynamic tokens, make sure index does not overflow */
-	if (index >= CMDLINE_PARSE_DYNAMIC_TOKENS - 1)
-		return NULL;
-	/* in case token is already generated, return it */
-	if ((*dyn_tokens)[index])
-		return (*dyn_tokens)[index];
-	/* generate token */
-	inst->f(&(*dyn_tokens)[index], NULL, dyn_tokens);
-	/* return immediately if there are no more tokens to expect */
-	if (!(*dyn_tokens)[index])
-		return NULL;
-	/* terminate list with a NULL entry */
-	(*dyn_tokens)[index + 1] = NULL;
-	return (*dyn_tokens)[index];
+	/* generate dynamic token */
+	token_p = NULL;
+	inst->f(&token_p, NULL, &inst->tokens[index]);
+	return token_p;
 }
 
 /**
@@ -172,22 +161,20 @@ get_token(cmdline_parse_inst_t *inst,
  */
 static int
 match_inst(cmdline_parse_inst_t *inst, const char *buf,
-	   unsigned int nb_match_token, void *resbuf, unsigned resbuf_size,
-	   cmdline_parse_token_hdr_t
-		*(*dyn_tokens)[CMDLINE_PARSE_DYNAMIC_TOKENS])
+	   unsigned int nb_match_token, void *resbuf, unsigned resbuf_size)
 {
-	unsigned int token_num=0;
 	cmdline_parse_token_hdr_t * token_p;
 	unsigned int i=0;
 	int n = 0;
 	struct cmdline_token_hdr token_hdr;
 
-	token_p = get_token(inst, token_num, dyn_tokens);
-	if (token_p)
+	/* check if we match all tokens of inst */
+	while (!nb_match_token || i < nb_match_token) {
+		token_p = get_token(inst, i);
+		if (!token_p)
+			break;
 		memcpy(&token_hdr, token_p, sizeof(token_hdr));
 
-	/* check if we match all tokens of inst */
-	while (token_p && (!nb_match_token || i<nb_match_token)) {
 		debug_printf("TK\n");
 		/* skip spaces */
 		while (isblank2(*buf)) {
@@ -222,11 +209,6 @@ match_inst(cmdline_parse_inst_t *inst, const char *buf,
 		debug_printf("TK parsed (len=%d)\n", n);
 		i++;
 		buf += n;
-
-		token_num ++;
-		token_p = get_token(inst, token_num, dyn_tokens);
-		if (token_p)
-			memcpy(&token_hdr, token_p, sizeof(token_hdr));
 	}
 
 	/* does not match */
@@ -270,7 +252,6 @@ cmdline_parse(struct cmdline *cl, const char * buf)
 		char buf[CMDLINE_PARSE_RESULT_BUFSIZE];
 		long double align; /* strong alignment constraint for buf */
 	} result, tmp_result;
-	cmdline_parse_token_hdr_t *dyn_tokens[CMDLINE_PARSE_DYNAMIC_TOKENS];
 	void (*f)(void *, struct cmdline *, void *) = NULL;
 	void *data = NULL;
 	int comment = 0;
@@ -327,13 +308,12 @@ cmdline_parse(struct cmdline *cl, const char * buf)
 
 	/* parse it !! */
 	inst = ctx[inst_num];
-	dyn_tokens[0] = NULL;
 	while (inst) {
 		debug_printf("INST %d\n", inst_num);
 
 		/* fully parsed */
 		tok = match_inst(inst, buf, 0, tmp_result.buf,
-				 sizeof(tmp_result.buf), &dyn_tokens);
+				 sizeof(tmp_result.buf));
 
 		if (tok > 0) /* we matched at least one token */
 			err = CMDLINE_PARSE_BAD_ARGS;
@@ -365,7 +345,6 @@ cmdline_parse(struct cmdline *cl, const char * buf)
 
 		inst_num ++;
 		inst = ctx[inst_num];
-		dyn_tokens[0] = NULL;
 	}
 
 	/* call func */
@@ -392,7 +371,6 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state,
 	cmdline_parse_token_hdr_t *token_p;
 	struct cmdline_token_hdr token_hdr;
 	char tmpbuf[CMDLINE_BUFFER_SIZE], comp_buf[CMDLINE_BUFFER_SIZE];
-	cmdline_parse_token_hdr_t *dyn_tokens[CMDLINE_PARSE_DYNAMIC_TOKENS];
 	unsigned int partial_tok_len;
 	int comp_len = -1;
 	int tmp_len = -1;
@@ -432,16 +410,14 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state,
 		nb_non_completable = 0;
 
 		inst = ctx[inst_num];
-		dyn_tokens[0] = NULL;
 		while (inst) {
 			/* parse the first tokens of the inst */
 			if (nb_token &&
-			    match_inst(inst, buf, nb_token, NULL, 0,
-				       &dyn_tokens))
+			    match_inst(inst, buf, nb_token, NULL, 0))
 				goto next;
 
 			debug_printf("instruction match\n");
-			token_p = get_token(inst, nb_token, &dyn_tokens);
+			token_p = get_token(inst, nb_token);
 			if (token_p)
 				memcpy(&token_hdr, token_p, sizeof(token_hdr));
 
@@ -495,7 +471,6 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state,
 			debug_printf("next\n");
 			inst_num ++;
 			inst = ctx[inst_num];
-			dyn_tokens[0] = NULL;
 		}
 
 		debug_printf("total choices %d for this completion\n",
@@ -528,16 +503,15 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state,
 
 	inst_num = 0;
 	inst = ctx[inst_num];
-	dyn_tokens[0] = NULL;
 	while (inst) {
 		/* we need to redo it */
 		inst = ctx[inst_num];
 
 		if (nb_token &&
-		    match_inst(inst, buf, nb_token, NULL, 0, &dyn_tokens))
+		    match_inst(inst, buf, nb_token, NULL, 0))
 			goto next2;
 
-		token_p = get_token(inst, nb_token, &dyn_tokens);
+		token_p = get_token(inst, nb_token);
 		if (token_p)
 			memcpy(&token_hdr, token_p, sizeof(token_hdr));
 
@@ -610,7 +584,6 @@ cmdline_complete(struct cmdline *cl, const char *buf, int *state,
 	next2:
 		inst_num ++;
 		inst = ctx[inst_num];
-		dyn_tokens[0] = NULL;
 	}
 	return 0;
 }
diff --git a/lib/librte_cmdline/cmdline_parse.h b/lib/librte_cmdline/cmdline_parse.h
index 65b18d4..13e086f 100644
--- a/lib/librte_cmdline/cmdline_parse.h
+++ b/lib/librte_cmdline/cmdline_parse.h
@@ -83,9 +83,6 @@ extern "C" {
 /* maximum buffer size for parsed result */
 #define CMDLINE_PARSE_RESULT_BUFSIZE 8192
 
-/* maximum number of dynamic tokens */
-#define CMDLINE_PARSE_DYNAMIC_TOKENS 128
-
 /**
  * Stores a pointer to the ops struct, and the offset: the place to
  * write the parsed result in the destination structure.
@@ -137,20 +134,53 @@ struct cmdline;
  * When no tokens are defined (tokens[0] == NULL), they are retrieved
  * dynamically by calling f() as follows:
  *
- *  f((struct cmdline_token_hdr **)&token_hdr,
- *    NULL,
- *    (struct cmdline_token_hdr *[])tokens));
+ * @code
+ *
+ * f((struct cmdline_token_hdr **)&token_p,
+ *   NULL,
+ *   (struct cmdline_token_hdr **)&inst->tokens[num]);
+ *
+ * @endcode
  *
  * The address of the resulting token is expected at the location pointed by
  * the first argument. Can be set to NULL to end the list.
  *
  * The cmdline argument (struct cmdline *) is always NULL.
  *
- * The last argument points to the NULL-terminated list of dynamic tokens
- * defined so far. Since token_hdr points to an index of that list, the
- * current index can be derived as follows:
+ * The last argument points to the inst->tokens[] entry to retrieve, which
+ * is not necessarily inside allocated memory and should neither be read nor
+ * written. Its sole purpose is to deduce the token entry index of interest
+ * as described in the example below.
+ *
+ * Note about constraints:
+ *
+ * - Only the address of these tokens is dynamic, their storage should be
+ *   static like normal tokens.
+ * - Dynamic token lists that need to maintain an internal context (e.g. in
+ *   order to determine the next token) must store it statically also. This
+ *   context must be reinitialized when the first token is requested, that
+ *   is, when &inst->tokens[0] is provided as the third argument.
+ * - Dynamic token lists must be NULL-terminated to generate usable
+ *   commands.
+ *
+ * @code
+ *
+ * // Assuming first and third arguments are respectively named "token_p"
+ * // and "token":
+ *
+ * int index = token - inst->tokens;
+ *
+ * if (!index) {
+ *     [...] // Clean up internal context if any.
+ * }
+ * [...] // Then set up dyn_token according to index.
+ *
+ * if (no_more_tokens)
+ *     *token_p = NULL;
+ * else
+ *     *token_p = &dyn_token;
  *
- *  int index = token_hdr - &(*tokens)[0];
+ * @endcode
  */
 struct cmdline_inst {
 	/* f(parsed_struct, data) */
-- 
2.1.4

[dpdk-dev] [PATCH v3 3/3] app/testpmd: fix token matching in flow command

[Adrien Mazarguil]

While matching user input against a token name or any other fixed string,
comparison stops at the end of user input if shorter (e.g. "foo" matches
token name "foobar").

Although the unintended consequence of this behavior allows users to
abbreviate command names and various parameters yet generate valid
commands, the parser was not designed to support this and does not prevent
ambiguous tokens.

For instance, entering "i" for a pattern item matches "ipv4", "ipv6" and
"icmp" then defaults to one of them in an unspecified manner.

Prevent this behavior by taking the length of fixed strings into account.

Fixes: 19c90af6285c ("app/testpmd: add flow command")
Fixes: 5ac3502ed1be ("app/testpmd: add flow query command")
Fixes: abc3d81aca1b ("app/testpmd: add item raw to flow command")
Fixes: 05d34c6e9d2c ("app/testpmd: add queue actions to flow command")
Cc: stable@dpdk.org

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 app/test-pmd/cmdline_flow.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
index 9533df1..a17a004 100644
--- a/app/test-pmd/cmdline_flow.c
+++ b/app/test-pmd/cmdline_flow.c
@@ -1624,6 +1624,19 @@ arg_entry_bf_fill(void *dst, uintmax_t val, const struct arg *arg)
 	return len;
 }
 
+/** Compare a string with a partial one of a given length. */
+static int
+strcmp_partial(const char *full, const char *partial, size_t partial_len)
+{
+	int r = strncmp(full, partial, partial_len);
+
+	if (r)
+		return r;
+	if (strlen(full) <= partial_len)
+		return 0;
+	return full[partial_len];
+}
+
 /**
  * Parse a prefix length and generate a bit-mask.
  *
@@ -1706,7 +1719,7 @@ parse_default(struct context *ctx, const struct token *token,
 	(void)ctx;
 	(void)buf;
 	(void)size;
-	if (strncmp(str, token->name, len))
+	if (strcmp_partial(token->name, str, len))
 		return -1;
 	return len;
 }
@@ -1949,7 +1962,7 @@ parse_vc_action_rss_queue(struct context *ctx, const struct token *token,
 	if (ctx->curr != ACTION_RSS_QUEUE)
 		return -1;
 	i = ctx->objdata >> 16;
-	if (!strncmp(str, "end", len)) {
+	if (!strcmp_partial("end", str, len)) {
 		ctx->objdata &= 0xffff;
 		return len;
 	}
@@ -2084,7 +2097,7 @@ parse_action(struct context *ctx, const struct token *token,
 		const struct parse_action_priv *priv;
 
 		token = &token_list[next_action[i]];
-		if (strncmp(token->name, str, len))
+		if (strcmp_partial(token->name, str, len))
 			continue;
 		priv = token->priv;
 		if (!priv)
@@ -2451,7 +2464,7 @@ parse_boolean(struct context *ctx, const struct token *token,
 	if (!arg)
 		return -1;
 	for (i = 0; boolean_name[i]; ++i)
-		if (!strncmp(str, boolean_name[i], len))
+		if (!strcmp_partial(boolean_name[i], str, len))
 			break;
 	/* Process token as integer. */
 	if (boolean_name[i])
-- 
2.1.4

Re: [dpdk-dev] [PATCH v3 0/3] librte_cmdline: fix parsing initialisation

[Thomas Monjalon]

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