[PATCH 0/2] fix uncallable unit tests (Bugzilla 1002)

[PATCH 0/2] fix uncallable unit tests (Bugzilla 1002)

[Bruce Richardson]

The dump* unit tests are uncallable from our test framework because the command
validation in the unit test binary is incorrect. Rather than trying to fix this
directly in the binary, a better approach is to add command validation to the
cmdline library - which already has 99% of what we need already - and use that
from the test app.

Bruce Richardson (2):
  cmdline: add function to verify valid commands
  test: use cmdline library to validate args

 app/test/commands.c         | 11 -----------
 app/test/test.c             | 25 ++++++++-----------------
 lib/cmdline/cmdline_parse.c | 20 +++++++++++++++++---
 lib/cmdline/cmdline_parse.h | 17 +++++++++++++++--
 lib/cmdline/version.map     |  3 +++
 5 files changed, 43 insertions(+), 33 deletions(-)

--
2.34.1

[PATCH 1/2] cmdline: add function to verify valid commands

[Bruce Richardson]

The cmdline library cmdline_parse() function parses a command and
executes the action automatically too. The cmdline_valid_buffer function
also uses this function to validate commands, meaning that there is no
function to validate a command as ok without executing it.

To fix this omission, we extract the body of cmdline_parse into a new
static inline function with an extra parameter to indicate whether the
action should be performed or not. Then we create two wrappers around
that - a replacement for the existing cmdline_parse function where the
extra parameter is "true" to execute the command, and a new function
"cmdline_parse_check" which passes the parameter as "false" to perform
cmdline validation only.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/cmdline/cmdline_parse.c | 20 +++++++++++++++++---
 lib/cmdline/cmdline_parse.h | 17 +++++++++++++++--
 lib/cmdline/version.map     |  3 +++
 3 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/lib/cmdline/cmdline_parse.c b/lib/cmdline/cmdline_parse.c
index 349ec87bd7..75ea74db81 100644
--- a/lib/cmdline/cmdline_parse.c
+++ b/lib/cmdline/cmdline_parse.c
@@ -7,6 +7,7 @@
 #include <stdio.h>
 #include <errno.h>
 #include <string.h>
+#include <stdbool.h>
 
 #include <rte_string_fns.h>
 
@@ -182,8 +183,8 @@ match_inst(cmdline_parse_inst_t *inst, const char *buf,
 }
 
 
-int
-cmdline_parse(struct cmdline *cl, const char * buf)
+static inline int
+__cmdline_parse(struct cmdline *cl, const char * buf, bool call_fn)
 {
 	unsigned int inst_num=0;
 	cmdline_parse_inst_t *inst;
@@ -284,7 +285,8 @@ cmdline_parse(struct cmdline *cl, const char * buf)
 
 	/* call func */
 	if (f) {
-		f(result.buf, cl, data);
+		if (call_fn)
+			f(result.buf, cl, data);
 	}
 
 	/* no match */
@@ -296,6 +298,18 @@ cmdline_parse(struct cmdline *cl, const char * buf)
 	return linelen;
 }
 
+int
+cmdline_parse(struct cmdline *cl, const char * buf)
+{
+	return __cmdline_parse(cl, buf, true);
+}
+
+int
+cmdline_parse_check(struct cmdline *cl, const char * buf)
+{
+	return __cmdline_parse(cl, buf, false);
+}
+
 int
 cmdline_complete(struct cmdline *cl, const char *buf, int *state,
 		 char *dst, unsigned int size)
diff --git a/lib/cmdline/cmdline_parse.h b/lib/cmdline/cmdline_parse.h
index e4d802fff7..6dd210d843 100644
--- a/lib/cmdline/cmdline_parse.h
+++ b/lib/cmdline/cmdline_parse.h
@@ -7,6 +7,8 @@
 #ifndef _CMDLINE_PARSE_H_
 #define _CMDLINE_PARSE_H_
 
+#include <rte_compat.h>
+
 #ifdef __cplusplus
 extern "C" {
 #endif
@@ -149,11 +151,22 @@ typedef cmdline_parse_inst_t *cmdline_parse_ctx_t;
  * argument buf must ends with "\n\0". The function returns
  * CMDLINE_PARSE_AMBIGUOUS, CMDLINE_PARSE_NOMATCH or
  * CMDLINE_PARSE_BAD_ARGS on error. Else it calls the associated
- * function (defined in the context) and returns 0
- * (CMDLINE_PARSE_SUCCESS).
+ * function (defined in the context) and returns the parsed line length (>= 0)
  */
 int cmdline_parse(struct cmdline *cl, const char *buf);
 
+/**
+ * Try to parse a buffer according to the specified context, but do not
+ * perform any function calls if parse is successful.
+ *
+ * The argument buf must ends with "\n\0".
+ * The function returns CMDLINE_PARSE_AMBIGUOUS, CMDLINE_PARSE_NOMATCH or
+ * CMDLINE_PARSE_BAD_ARGS on error and returns the parsed line length (>=0).
+ * on successful parse
+ */
+__rte_experimental
+int cmdline_parse_check(struct cmdline *cl, const char *buf);
+
 /**
  * complete() must be called with *state==0 (try to complete) or
  * with *state==-1 (just display choices), then called without
diff --git a/lib/cmdline/version.map b/lib/cmdline/version.map
index b9bbb87510..fc7fdd6ea4 100644
--- a/lib/cmdline/version.map
+++ b/lib/cmdline/version.map
@@ -81,5 +81,8 @@ EXPERIMENTAL {
 	rdline_get_history_buffer_size;
 	rdline_get_opaque;
 
+	# added in 22.07
+	cmdline_parse_check;
+
 	local: *;
 };
-- 
2.34.1

[PATCH 2/2] test: use cmdline library to validate args

[Bruce Richardson]

When passing in test names to run via either the DPDK_TEST environment
variable or via extra argv parameters, the checks run on those commands
can miss valid commands that are registered with the cmdline library in
the initial context used to set it up. This is seen in the fact that the
"dump_*" set of commands are not callable via argv parameters, but can
be called manually.

To fix this, just use the commandline library to validate each command
before executing it, stopping execution when an error is encountered.
This also has the benefit of not having the test binrary drop to
interactive mode if all commandline parameters given are invalid.

Fixes: 9b848774a5dc ("test: use env variable to run tests")
Fixes: ace2f054ed43 ("test: take test names from command line")
Bugzilla ID: 1002

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/commands.c | 11 -----------
 app/test/test.c     | 25 ++++++++-----------------
 2 files changed, 8 insertions(+), 28 deletions(-)

diff --git a/app/test/commands.c b/app/test/commands.c
index 887cabad64..31259e5c21 100644
--- a/app/test/commands.c
+++ b/app/test/commands.c
@@ -378,14 +378,3 @@ int commands_init(void)
 	cmd_autotest_autotest.string_data.str = commands;
 	return 0;
 }
-
-int command_valid(const char *cmd)
-{
-	struct test_command *t;
-
-	TAILQ_FOREACH(t, &commands_list, next) {
-		if (strcmp(t->command, cmd) == 0)
-			return 1;
-	}
-	return 0;
-}
diff --git a/app/test/test.c b/app/test/test.c
index e69cae3eea..5b308ea6a3 100644
--- a/app/test/test.c
+++ b/app/test/test.c
@@ -186,22 +186,10 @@ main(int argc, char **argv)
 #ifdef RTE_LIB_CMDLINE
 	char *dpdk_test = getenv("DPDK_TEST");
 
-	if (dpdk_test && strlen(dpdk_test) == 0)
-		dpdk_test = NULL;
-
-	if (dpdk_test && !command_valid(dpdk_test)) {
-		RTE_LOG(WARNING, APP, "Invalid DPDK_TEST value '%s'\n", dpdk_test);
-		dpdk_test = NULL;
-	}
-
-	if (dpdk_test)
+	if (dpdk_test && strlen(dpdk_test) > 0)
 		tests[test_count++] = dpdk_test;
-	for (i = 1; i < argc; i++) {
-		if (!command_valid(argv[i]))
-			RTE_LOG(WARNING, APP, "Invalid test requested: '%s'\n", argv[i]);
-		else
-			tests[test_count++] = argv[i];
-	}
+	for (i = 1; i < argc; i++)
+		tests[test_count++] = argv[i];
 
 	if (test_count > 0) {
 		char buf[1024];
@@ -214,9 +202,12 @@ main(int argc, char **argv)
 
 		for (i = 0; i < test_count; i++) {
 			snprintf(buf, sizeof(buf), "%s\n", tests[i]);
-			if (cmdline_in(cl, buf, strlen(buf)) < 0) {
+			if (cmdline_parse_check(cl, buf) < 0) {
+				printf("Error: invalid test command: '%s'\n", tests[i]);
+				ret = -1;
+			}
+			else if (cmdline_in(cl, buf, strlen(buf)) < 0) {
 				printf("error on cmdline input\n");
-
 				ret = -1;
 			} else
 				ret = last_test_result;
-- 
2.34.1

[PATCH v2 0/2] fix uncallable unit tests (Bugzilla 1002)

[Bruce Richardson]

The dump* unit tests are uncallable from our test framework because the command
validation in the unit test binary is incorrect. Rather than trying to fix this
directly in the binary, a better approach is to add command validation to the
cmdline library - which already has 99% of what we need already - and use that
from the test app.

V2:
* Fix checkpatch issues

Bruce Richardson (2):
  cmdline: add function to verify valid commands
  test: use cmdline library to validate args

 app/test/commands.c         | 11 -----------
 app/test/test.c             | 24 +++++++-----------------
 lib/cmdline/cmdline_parse.c | 20 +++++++++++++++++---
 lib/cmdline/cmdline_parse.h | 17 +++++++++++++++--
 lib/cmdline/version.map     |  3 +++
 5 files changed, 42 insertions(+), 33 deletions(-)

--
2.34.1

[PATCH v2 1/2] cmdline: add function to verify valid commands

[Bruce Richardson]

The cmdline library cmdline_parse() function parses a command and
executes the action automatically too. The cmdline_valid_buffer function
also uses this function to validate commands, meaning that there is no
function to validate a command as ok without executing it.

To fix this omission, we extract the body of cmdline_parse into a new
static inline function with an extra parameter to indicate whether the
action should be performed or not. Then we create two wrappers around
that - a replacement for the existing cmdline_parse function where the
extra parameter is "true" to execute the command, and a new function
"cmdline_parse_check" which passes the parameter as "false" to perform
cmdline validation only.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/cmdline/cmdline_parse.c | 20 +++++++++++++++++---
 lib/cmdline/cmdline_parse.h | 17 +++++++++++++++--
 lib/cmdline/version.map     |  3 +++
 3 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/lib/cmdline/cmdline_parse.c b/lib/cmdline/cmdline_parse.c
index 349ec87bd7..b7fdc67ae5 100644
--- a/lib/cmdline/cmdline_parse.c
+++ b/lib/cmdline/cmdline_parse.c
@@ -7,6 +7,7 @@
 #include <stdio.h>
 #include <errno.h>
 #include <string.h>
+#include <stdbool.h>
 
 #include <rte_string_fns.h>
 
@@ -182,8 +183,8 @@ match_inst(cmdline_parse_inst_t *inst, const char *buf,
 }
 
 
-int
-cmdline_parse(struct cmdline *cl, const char * buf)
+static inline int
+__cmdline_parse(struct cmdline *cl, const char *buf, bool call_fn)
 {
 	unsigned int inst_num=0;
 	cmdline_parse_inst_t *inst;
@@ -284,7 +285,8 @@ cmdline_parse(struct cmdline *cl, const char * buf)
 
 	/* call func */
 	if (f) {
-		f(result.buf, cl, data);
+		if (call_fn)
+			f(result.buf, cl, data);
 	}
 
 	/* no match */
@@ -296,6 +298,18 @@ cmdline_parse(struct cmdline *cl, const char * buf)
 	return linelen;
 }
 
+int
+cmdline_parse(struct cmdline *cl, const char *buf)
+{
+	return __cmdline_parse(cl, buf, true);
+}
+
+int
+cmdline_parse_check(struct cmdline *cl, const char *buf)
+{
+	return __cmdline_parse(cl, buf, false);
+}
+
 int
 cmdline_complete(struct cmdline *cl, const char *buf, int *state,
 		 char *dst, unsigned int size)
diff --git a/lib/cmdline/cmdline_parse.h b/lib/cmdline/cmdline_parse.h
index e4d802fff7..6dd210d843 100644
--- a/lib/cmdline/cmdline_parse.h
+++ b/lib/cmdline/cmdline_parse.h
@@ -7,6 +7,8 @@
 #ifndef _CMDLINE_PARSE_H_
 #define _CMDLINE_PARSE_H_
 
+#include <rte_compat.h>
+
 #ifdef __cplusplus
 extern "C" {
 #endif
@@ -149,11 +151,22 @@ typedef cmdline_parse_inst_t *cmdline_parse_ctx_t;
  * argument buf must ends with "\n\0". The function returns
  * CMDLINE_PARSE_AMBIGUOUS, CMDLINE_PARSE_NOMATCH or
  * CMDLINE_PARSE_BAD_ARGS on error. Else it calls the associated
- * function (defined in the context) and returns 0
- * (CMDLINE_PARSE_SUCCESS).
+ * function (defined in the context) and returns the parsed line length (>= 0)
  */
 int cmdline_parse(struct cmdline *cl, const char *buf);
 
+/**
+ * Try to parse a buffer according to the specified context, but do not
+ * perform any function calls if parse is successful.
+ *
+ * The argument buf must ends with "\n\0".
+ * The function returns CMDLINE_PARSE_AMBIGUOUS, CMDLINE_PARSE_NOMATCH or
+ * CMDLINE_PARSE_BAD_ARGS on error and returns the parsed line length (>=0).
+ * on successful parse
+ */
+__rte_experimental
+int cmdline_parse_check(struct cmdline *cl, const char *buf);
+
 /**
  * complete() must be called with *state==0 (try to complete) or
  * with *state==-1 (just display choices), then called without
diff --git a/lib/cmdline/version.map b/lib/cmdline/version.map
index b9bbb87510..fc7fdd6ea4 100644
--- a/lib/cmdline/version.map
+++ b/lib/cmdline/version.map
@@ -81,5 +81,8 @@ EXPERIMENTAL {
 	rdline_get_history_buffer_size;
 	rdline_get_opaque;
 
+	# added in 22.07
+	cmdline_parse_check;
+
 	local: *;
 };
-- 
2.34.1

[PATCH v2 2/2] test: use cmdline library to validate args

[Bruce Richardson]

When passing in test names to run via either the DPDK_TEST environment
variable or via extra argv parameters, the checks run on those commands
can miss valid commands that are registered with the cmdline library in
the initial context used to set it up. This is seen in the fact that the
"dump_*" set of commands are not callable via argv parameters, but can
be called manually.

To fix this, just use the commandline library to validate each command
before executing it, stopping execution when an error is encountered.
This also has the benefit of not having the test binrary drop to
interactive mode if all commandline parameters given are invalid.

Fixes: 9b848774a5dc ("test: use env variable to run tests")
Fixes: ace2f054ed43 ("test: take test names from command line")
Bugzilla ID: 1002

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/commands.c | 11 -----------
 app/test/test.c     | 24 +++++++-----------------
 2 files changed, 7 insertions(+), 28 deletions(-)

diff --git a/app/test/commands.c b/app/test/commands.c
index 887cabad64..31259e5c21 100644
--- a/app/test/commands.c
+++ b/app/test/commands.c
@@ -378,14 +378,3 @@ int commands_init(void)
 	cmd_autotest_autotest.string_data.str = commands;
 	return 0;
 }
-
-int command_valid(const char *cmd)
-{
-	struct test_command *t;
-
-	TAILQ_FOREACH(t, &commands_list, next) {
-		if (strcmp(t->command, cmd) == 0)
-			return 1;
-	}
-	return 0;
-}
diff --git a/app/test/test.c b/app/test/test.c
index e69cae3eea..ce25f468a6 100644
--- a/app/test/test.c
+++ b/app/test/test.c
@@ -186,22 +186,10 @@ main(int argc, char **argv)
 #ifdef RTE_LIB_CMDLINE
 	char *dpdk_test = getenv("DPDK_TEST");
 
-	if (dpdk_test && strlen(dpdk_test) == 0)
-		dpdk_test = NULL;
-
-	if (dpdk_test && !command_valid(dpdk_test)) {
-		RTE_LOG(WARNING, APP, "Invalid DPDK_TEST value '%s'\n", dpdk_test);
-		dpdk_test = NULL;
-	}
-
-	if (dpdk_test)
+	if (dpdk_test && strlen(dpdk_test) > 0)
 		tests[test_count++] = dpdk_test;
-	for (i = 1; i < argc; i++) {
-		if (!command_valid(argv[i]))
-			RTE_LOG(WARNING, APP, "Invalid test requested: '%s'\n", argv[i]);
-		else
-			tests[test_count++] = argv[i];
-	}
+	for (i = 1; i < argc; i++)
+		tests[test_count++] = argv[i];
 
 	if (test_count > 0) {
 		char buf[1024];
@@ -214,9 +202,11 @@ main(int argc, char **argv)
 
 		for (i = 0; i < test_count; i++) {
 			snprintf(buf, sizeof(buf), "%s\n", tests[i]);
-			if (cmdline_in(cl, buf, strlen(buf)) < 0) {
+			if (cmdline_parse_check(cl, buf) < 0) {
+				printf("Error: invalid test command: '%s'\n", tests[i]);
+				ret = -1;
+			} else if (cmdline_in(cl, buf, strlen(buf)) < 0) {
 				printf("error on cmdline input\n");
-
 				ret = -1;
 			} else
 				ret = last_test_result;
-- 
2.34.1

RE: [PATCH v2 1/2] cmdline: add function to verify valid commands

[Li, WeiyuanX]

> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Friday, May 20, 2022 11:13 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Li, WeiyuanX <weiyuanx.li@intel.com>; Richardson,
> Bruce <bruce.richardson@intel.com>; Olivier Matz
> <olivier.matz@6wind.com>; Ray Kinsella <mdr@ashroe.eu>
> Subject: [PATCH v2 1/2] cmdline: add function to verify valid commands
> 
> The cmdline library cmdline_parse() function parses a command and
> executes the action automatically too. The cmdline_valid_buffer function
> also uses this function to validate commands, meaning that there is no
> function to validate a command as ok without executing it.
> 
> To fix this omission, we extract the body of cmdline_parse into a new static
> inline function with an extra parameter to indicate whether the action should
> be performed or not. Then we create two wrappers around that - a
> replacement for the existing cmdline_parse function where the extra
> parameter is "true" to execute the command, and a new function
> "cmdline_parse_check" which passes the parameter as "false" to perform
> cmdline validation only.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---

Tested-by:  Weiyuan Li  <weiyuanx.li@intel.com>

Regards,
Li, Weiyuan

Re: [PATCH 1/2] cmdline: add function to verify valid commands

[Ray Kinsella]

Bruce Richardson <bruce.richardson@intel.com> writes:

> The cmdline library cmdline_parse() function parses a command and
> executes the action automatically too. The cmdline_valid_buffer function
> also uses this function to validate commands, meaning that there is no
> function to validate a command as ok without executing it.
>
> To fix this omission, we extract the body of cmdline_parse into a new
> static inline function with an extra parameter to indicate whether the
> action should be performed or not. Then we create two wrappers around
> that - a replacement for the existing cmdline_parse function where the
> extra parameter is "true" to execute the command, and a new function
> "cmdline_parse_check" which passes the parameter as "false" to perform
> cmdline validation only.
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  lib/cmdline/cmdline_parse.c | 20 +++++++++++++++++---
>  lib/cmdline/cmdline_parse.h | 17 +++++++++++++++--
>  lib/cmdline/version.map     |  3 +++
>  3 files changed, 35 insertions(+), 5 deletions(-)

Acked-by: Ray Kinsella <mdr@ashroe.eu>

Re: [PATCH v2 1/2] cmdline: add function to verify valid commands

[Olivier Matz]

Hi Bruce,

Just few minor comments below.

On Fri, May 20, 2022 at 04:12:39PM +0100, Bruce Richardson wrote:
> The cmdline library cmdline_parse() function parses a command and
> executes the action automatically too. The cmdline_valid_buffer function
> also uses this function to validate commands, meaning that there is no
> function to validate a command as ok without executing it.
> 
> To fix this omission, we extract the body of cmdline_parse into a new
> static inline function with an extra parameter to indicate whether the
> action should be performed or not. Then we create two wrappers around
> that - a replacement for the existing cmdline_parse function where the
> extra parameter is "true" to execute the command, and a new function
> "cmdline_parse_check" which passes the parameter as "false" to perform
> cmdline validation only.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  lib/cmdline/cmdline_parse.c | 20 +++++++++++++++++---
>  lib/cmdline/cmdline_parse.h | 17 +++++++++++++++--
>  lib/cmdline/version.map     |  3 +++
>  3 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/cmdline/cmdline_parse.c b/lib/cmdline/cmdline_parse.c
> index 349ec87bd7..b7fdc67ae5 100644
> --- a/lib/cmdline/cmdline_parse.c
> +++ b/lib/cmdline/cmdline_parse.c
> @@ -7,6 +7,7 @@
>  #include <stdio.h>
>  #include <errno.h>
>  #include <string.h>
> +#include <stdbool.h>
>  
>  #include <rte_string_fns.h>
>  
> @@ -182,8 +183,8 @@ match_inst(cmdline_parse_inst_t *inst, const char *buf,
>  }
>  
>  
> -int
> -cmdline_parse(struct cmdline *cl, const char * buf)
> +static inline int
> +__cmdline_parse(struct cmdline *cl, const char *buf, bool call_fn)
>  {
>  	unsigned int inst_num=0;
>  	cmdline_parse_inst_t *inst;
> @@ -284,7 +285,8 @@ cmdline_parse(struct cmdline *cl, const char * buf)
>  
>  	/* call func */
>  	if (f) {
> -		f(result.buf, cl, data);
> +		if (call_fn)
> +			f(result.buf, cl, data);

Maybe nicer to test in one line:

if (f && call_fn)


>  	}
>  
>  	/* no match */
> @@ -296,6 +298,18 @@ cmdline_parse(struct cmdline *cl, const char * buf)
>  	return linelen;
>  }
>  
> +int
> +cmdline_parse(struct cmdline *cl, const char *buf)
> +{
> +	return __cmdline_parse(cl, buf, true);
> +}
> +
> +int
> +cmdline_parse_check(struct cmdline *cl, const char *buf)
> +{
> +	return __cmdline_parse(cl, buf, false);
> +}
> +
>  int
>  cmdline_complete(struct cmdline *cl, const char *buf, int *state,
>  		 char *dst, unsigned int size)
> diff --git a/lib/cmdline/cmdline_parse.h b/lib/cmdline/cmdline_parse.h
> index e4d802fff7..6dd210d843 100644
> --- a/lib/cmdline/cmdline_parse.h
> +++ b/lib/cmdline/cmdline_parse.h
> @@ -7,6 +7,8 @@
>  #ifndef _CMDLINE_PARSE_H_
>  #define _CMDLINE_PARSE_H_
>  
> +#include <rte_compat.h>
> +
>  #ifdef __cplusplus
>  extern "C" {
>  #endif
> @@ -149,11 +151,22 @@ typedef cmdline_parse_inst_t *cmdline_parse_ctx_t;
>   * argument buf must ends with "\n\0". The function returns
>   * CMDLINE_PARSE_AMBIGUOUS, CMDLINE_PARSE_NOMATCH or
>   * CMDLINE_PARSE_BAD_ARGS on error. Else it calls the associated
> - * function (defined in the context) and returns 0
> - * (CMDLINE_PARSE_SUCCESS).
> + * function (defined in the context) and returns the parsed line length (>= 0)

Can we add a dot at the end?

>   */
>  int cmdline_parse(struct cmdline *cl, const char *buf);
>  
> +/**
> + * Try to parse a buffer according to the specified context, but do not
> + * perform any function calls if parse is successful.
> + *
> + * The argument buf must ends with "\n\0".
> + * The function returns CMDLINE_PARSE_AMBIGUOUS, CMDLINE_PARSE_NOMATCH or
> + * CMDLINE_PARSE_BAD_ARGS on error and returns the parsed line length (>=0).
> + * on successful parse

Same here.

> + */
> +__rte_experimental
> +int cmdline_parse_check(struct cmdline *cl, const char *buf);
> +
>  /**
>   * complete() must be called with *state==0 (try to complete) or
>   * with *state==-1 (just display choices), then called without
> diff --git a/lib/cmdline/version.map b/lib/cmdline/version.map
> index b9bbb87510..fc7fdd6ea4 100644
> --- a/lib/cmdline/version.map
> +++ b/lib/cmdline/version.map
> @@ -81,5 +81,8 @@ EXPERIMENTAL {
>  	rdline_get_history_buffer_size;
>  	rdline_get_opaque;
>  
> +	# added in 22.07
> +	cmdline_parse_check;
> +
>  	local: *;
>  };
> -- 
> 2.34.1
> 

With these changes:
Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [PATCH v2 2/2] test: use cmdline library to validate args

[Olivier Matz]

On Fri, May 20, 2022 at 04:12:40PM +0100, Bruce Richardson wrote:
> When passing in test names to run via either the DPDK_TEST environment
> variable or via extra argv parameters, the checks run on those commands
> can miss valid commands that are registered with the cmdline library in
> the initial context used to set it up. This is seen in the fact that the
> "dump_*" set of commands are not callable via argv parameters, but can
> be called manually.
> 
> To fix this, just use the commandline library to validate each command
> before executing it, stopping execution when an error is encountered.
> This also has the benefit of not having the test binrary drop to
> interactive mode if all commandline parameters given are invalid.
> 
> Fixes: 9b848774a5dc ("test: use env variable to run tests")
> Fixes: ace2f054ed43 ("test: take test names from command line")
> Bugzilla ID: 1002
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [PATCH v2 1/2] cmdline: add function to verify valid commands

[Bruce Richardson]

On Tue, Jun 07, 2022 at 10:08:30AM +0200, Olivier Matz wrote:
> Hi Bruce,
> 
> Just few minor comments below.
> 
> On Fri, May 20, 2022 at 04:12:39PM +0100, Bruce Richardson wrote:
> > The cmdline library cmdline_parse() function parses a command and
> > executes the action automatically too. The cmdline_valid_buffer function
> > also uses this function to validate commands, meaning that there is no
> > function to validate a command as ok without executing it.
> > 
> > To fix this omission, we extract the body of cmdline_parse into a new
> > static inline function with an extra parameter to indicate whether the
> > action should be performed or not. Then we create two wrappers around
> > that - a replacement for the existing cmdline_parse function where the
> > extra parameter is "true" to execute the command, and a new function
> > "cmdline_parse_check" which passes the parameter as "false" to perform
> > cmdline validation only.
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  lib/cmdline/cmdline_parse.c | 20 +++++++++++++++++---
> >  lib/cmdline/cmdline_parse.h | 17 +++++++++++++++--
> >  lib/cmdline/version.map     |  3 +++
> >  3 files changed, 35 insertions(+), 5 deletions(-)
> > 
> > diff --git a/lib/cmdline/cmdline_parse.c b/lib/cmdline/cmdline_parse.c
> > index 349ec87bd7..b7fdc67ae5 100644
> > --- a/lib/cmdline/cmdline_parse.c
> > +++ b/lib/cmdline/cmdline_parse.c
> > @@ -7,6 +7,7 @@
> >  #include <stdio.h>
> >  #include <errno.h>
> >  #include <string.h>
> > +#include <stdbool.h>
> >  
> >  #include <rte_string_fns.h>
> >  
> > @@ -182,8 +183,8 @@ match_inst(cmdline_parse_inst_t *inst, const char *buf,
> >  }
> >  
> >  
> > -int
> > -cmdline_parse(struct cmdline *cl, const char * buf)
> > +static inline int
> > +__cmdline_parse(struct cmdline *cl, const char *buf, bool call_fn)
> >  {
> >  	unsigned int inst_num=0;
> >  	cmdline_parse_inst_t *inst;
> > @@ -284,7 +285,8 @@ cmdline_parse(struct cmdline *cl, const char * buf)
> >  
> >  	/* call func */
> >  	if (f) {
> > -		f(result.buf, cl, data);
> > +		if (call_fn)
> > +			f(result.buf, cl, data);
> 
> Maybe nicer to test in one line:
> 
> if (f && call_fn)
> 

If we do so, then we need to also change the "else" leg to "else if
(!call_fn)" because we don't want to have the debug_printf being output in
the case that we have call_fn == false. A better alternative is to slightly
restructure the whole block, to have the error leg first, which removes the
need for two condition checks before calling the function:

        /* no match */
        if (f == NULL) {
                debug_printf("No match err=%d\n", err);
                return err;
        }

        /* call func if requested*/
        if (call_fn)
                f(result.buf, cl, data);

        return linelen;

I think this latter option is better, so will implement in v3.

> 
> >  	}
> >  
> >  	/* no match */
> > @@ -296,6 +298,18 @@ cmdline_parse(struct cmdline *cl, const char * buf)
> >  	return linelen;
> >  }
> >  
> > +int
> > +cmdline_parse(struct cmdline *cl, const char *buf)
> > +{
> > +	return __cmdline_parse(cl, buf, true);
> > +}
> > +
> > +int
> > +cmdline_parse_check(struct cmdline *cl, const char *buf)
> > +{
> > +	return __cmdline_parse(cl, buf, false);
> > +}
> > +
> >  int
> >  cmdline_complete(struct cmdline *cl, const char *buf, int *state,
> >  		 char *dst, unsigned int size)
> > diff --git a/lib/cmdline/cmdline_parse.h b/lib/cmdline/cmdline_parse.h
> > index e4d802fff7..6dd210d843 100644
> > --- a/lib/cmdline/cmdline_parse.h
> > +++ b/lib/cmdline/cmdline_parse.h
> > @@ -7,6 +7,8 @@
> >  #ifndef _CMDLINE_PARSE_H_
> >  #define _CMDLINE_PARSE_H_
> >  
> > +#include <rte_compat.h>
> > +
> >  #ifdef __cplusplus
> >  extern "C" {
> >  #endif
> > @@ -149,11 +151,22 @@ typedef cmdline_parse_inst_t *cmdline_parse_ctx_t;
> >   * argument buf must ends with "\n\0". The function returns
> >   * CMDLINE_PARSE_AMBIGUOUS, CMDLINE_PARSE_NOMATCH or
> >   * CMDLINE_PARSE_BAD_ARGS on error. Else it calls the associated
> > - * function (defined in the context) and returns 0
> > - * (CMDLINE_PARSE_SUCCESS).
> > + * function (defined in the context) and returns the parsed line length (>= 0)
> 
> Can we add a dot at the end?

Ack.

> 
> >   */
> >  int cmdline_parse(struct cmdline *cl, const char *buf);
> >  
> > +/**
> > + * Try to parse a buffer according to the specified context, but do not
> > + * perform any function calls if parse is successful.
> > + *
> > + * The argument buf must ends with "\n\0".
> > + * The function returns CMDLINE_PARSE_AMBIGUOUS, CMDLINE_PARSE_NOMATCH or
> > + * CMDLINE_PARSE_BAD_ARGS on error and returns the parsed line length (>=0).
> > + * on successful parse
> 
> Same here.

Ack.

> 
> > + */
> > +__rte_experimental
> > +int cmdline_parse_check(struct cmdline *cl, const char *buf);
> > +
> >  /**
> >   * complete() must be called with *state==0 (try to complete) or
> >   * with *state==-1 (just display choices), then called without
> > diff --git a/lib/cmdline/version.map b/lib/cmdline/version.map
> > index b9bbb87510..fc7fdd6ea4 100644
> > --- a/lib/cmdline/version.map
> > +++ b/lib/cmdline/version.map
> > @@ -81,5 +81,8 @@ EXPERIMENTAL {
> >  	rdline_get_history_buffer_size;
> >  	rdline_get_opaque;
> >  
> > +	# added in 22.07
> > +	cmdline_parse_check;
> > +
> >  	local: *;
> >  };
> > -- 
> > 2.34.1
> > 
> 
> With these changes:
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
> 

Re: [PATCH v2 1/2] cmdline: add function to verify valid commands

[Olivier Matz]

On Fri, Jun 10, 2022 at 03:08:49PM +0100, Bruce Richardson wrote:
> On Tue, Jun 07, 2022 at 10:08:30AM +0200, Olivier Matz wrote:
> > Hi Bruce,
> > 
> > Just few minor comments below.
> > 
> > On Fri, May 20, 2022 at 04:12:39PM +0100, Bruce Richardson wrote:
> > > The cmdline library cmdline_parse() function parses a command and
> > > executes the action automatically too. The cmdline_valid_buffer function
> > > also uses this function to validate commands, meaning that there is no
> > > function to validate a command as ok without executing it.
> > > 
> > > To fix this omission, we extract the body of cmdline_parse into a new
> > > static inline function with an extra parameter to indicate whether the
> > > action should be performed or not. Then we create two wrappers around
> > > that - a replacement for the existing cmdline_parse function where the
> > > extra parameter is "true" to execute the command, and a new function
> > > "cmdline_parse_check" which passes the parameter as "false" to perform
> > > cmdline validation only.
> > > 
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > ---
> > >  lib/cmdline/cmdline_parse.c | 20 +++++++++++++++++---
> > >  lib/cmdline/cmdline_parse.h | 17 +++++++++++++++--
> > >  lib/cmdline/version.map     |  3 +++
> > >  3 files changed, 35 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/lib/cmdline/cmdline_parse.c b/lib/cmdline/cmdline_parse.c
> > > index 349ec87bd7..b7fdc67ae5 100644
> > > --- a/lib/cmdline/cmdline_parse.c
> > > +++ b/lib/cmdline/cmdline_parse.c
> > > @@ -7,6 +7,7 @@
> > >  #include <stdio.h>
> > >  #include <errno.h>
> > >  #include <string.h>
> > > +#include <stdbool.h>
> > >  
> > >  #include <rte_string_fns.h>
> > >  
> > > @@ -182,8 +183,8 @@ match_inst(cmdline_parse_inst_t *inst, const char *buf,
> > >  }
> > >  
> > >  
> > > -int
> > > -cmdline_parse(struct cmdline *cl, const char * buf)
> > > +static inline int
> > > +__cmdline_parse(struct cmdline *cl, const char *buf, bool call_fn)
> > >  {
> > >  	unsigned int inst_num=0;
> > >  	cmdline_parse_inst_t *inst;
> > > @@ -284,7 +285,8 @@ cmdline_parse(struct cmdline *cl, const char * buf)
> > >  
> > >  	/* call func */
> > >  	if (f) {
> > > -		f(result.buf, cl, data);
> > > +		if (call_fn)
> > > +			f(result.buf, cl, data);
> > 
> > Maybe nicer to test in one line:
> > 
> > if (f && call_fn)
> > 
> 
> If we do so, then we need to also change the "else" leg to "else if

Oh yes I missed the else part!


> (!call_fn)" because we don't want to have the debug_printf being output in
> the case that we have call_fn == false. A better alternative is to slightly
> restructure the whole block, to have the error leg first, which removes the
> need for two condition checks before calling the function:
> 
>         /* no match */
>         if (f == NULL) {
>                 debug_printf("No match err=%d\n", err);
>                 return err;
>         }
> 
>         /* call func if requested*/
>         if (call_fn)
>                 f(result.buf, cl, data);
> 
>         return linelen;
> 
> I think this latter option is better, so will implement in v3.

Yes, it looks good to me, thanks!


> 
> > 
> > >  	}
> > >  
> > >  	/* no match */
> > > @@ -296,6 +298,18 @@ cmdline_parse(struct cmdline *cl, const char * buf)
> > >  	return linelen;
> > >  }
> > >  
> > > +int
> > > +cmdline_parse(struct cmdline *cl, const char *buf)
> > > +{
> > > +	return __cmdline_parse(cl, buf, true);
> > > +}
> > > +
> > > +int
> > > +cmdline_parse_check(struct cmdline *cl, const char *buf)
> > > +{
> > > +	return __cmdline_parse(cl, buf, false);
> > > +}
> > > +
> > >  int
> > >  cmdline_complete(struct cmdline *cl, const char *buf, int *state,
> > >  		 char *dst, unsigned int size)
> > > diff --git a/lib/cmdline/cmdline_parse.h b/lib/cmdline/cmdline_parse.h
> > > index e4d802fff7..6dd210d843 100644
> > > --- a/lib/cmdline/cmdline_parse.h
> > > +++ b/lib/cmdline/cmdline_parse.h
> > > @@ -7,6 +7,8 @@
> > >  #ifndef _CMDLINE_PARSE_H_
> > >  #define _CMDLINE_PARSE_H_
> > >  
> > > +#include <rte_compat.h>
> > > +
> > >  #ifdef __cplusplus
> > >  extern "C" {
> > >  #endif
> > > @@ -149,11 +151,22 @@ typedef cmdline_parse_inst_t *cmdline_parse_ctx_t;
> > >   * argument buf must ends with "\n\0". The function returns
> > >   * CMDLINE_PARSE_AMBIGUOUS, CMDLINE_PARSE_NOMATCH or
> > >   * CMDLINE_PARSE_BAD_ARGS on error. Else it calls the associated
> > > - * function (defined in the context) and returns 0
> > > - * (CMDLINE_PARSE_SUCCESS).
> > > + * function (defined in the context) and returns the parsed line length (>= 0)
> > 
> > Can we add a dot at the end?
> 
> Ack.
> 
> > 
> > >   */
> > >  int cmdline_parse(struct cmdline *cl, const char *buf);
> > >  
> > > +/**
> > > + * Try to parse a buffer according to the specified context, but do not
> > > + * perform any function calls if parse is successful.
> > > + *
> > > + * The argument buf must ends with "\n\0".
> > > + * The function returns CMDLINE_PARSE_AMBIGUOUS, CMDLINE_PARSE_NOMATCH or
> > > + * CMDLINE_PARSE_BAD_ARGS on error and returns the parsed line length (>=0).
> > > + * on successful parse
> > 
> > Same here.
> 
> Ack.
> 
> > 
> > > + */
> > > +__rte_experimental
> > > +int cmdline_parse_check(struct cmdline *cl, const char *buf);
> > > +
> > >  /**
> > >   * complete() must be called with *state==0 (try to complete) or
> > >   * with *state==-1 (just display choices), then called without
> > > diff --git a/lib/cmdline/version.map b/lib/cmdline/version.map
> > > index b9bbb87510..fc7fdd6ea4 100644
> > > --- a/lib/cmdline/version.map
> > > +++ b/lib/cmdline/version.map
> > > @@ -81,5 +81,8 @@ EXPERIMENTAL {
> > >  	rdline_get_history_buffer_size;
> > >  	rdline_get_opaque;
> > >  
> > > +	# added in 22.07
> > > +	cmdline_parse_check;
> > > +
> > >  	local: *;
> > >  };
> > > -- 
> > > 2.34.1
> > > 
> > 
> > With these changes:
> > Acked-by: Olivier Matz <olivier.matz@6wind.com>
> >