* [PATCH 0/2] fix uncallable unit tests (Bugzilla 1002)
@ 2022-05-20 14:56 Bruce Richardson
2022-05-20 14:56 ` [PATCH 1/2] cmdline: add function to verify valid commands Bruce Richardson
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Bruce Richardson @ 2022-05-20 14:56 UTC (permalink / raw)
To: dev; +Cc: stable, weiyuanx.li, xuemingx.zhang, 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
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/2] cmdline: add function to verify valid commands 2022-05-20 14:56 [PATCH 0/2] fix uncallable unit tests (Bugzilla 1002) Bruce Richardson @ 2022-05-20 14:56 ` Bruce Richardson 2022-05-24 14:57 ` Ray Kinsella 2022-05-20 14:56 ` [PATCH 2/2] test: use cmdline library to validate args Bruce Richardson 2022-05-20 15:12 ` [PATCH v2 0/2] fix uncallable unit tests (Bugzilla 1002) Bruce Richardson 2 siblings, 1 reply; 12+ messages in thread From: Bruce Richardson @ 2022-05-20 14:56 UTC (permalink / raw) To: dev Cc: stable, weiyuanx.li, xuemingx.zhang, Bruce Richardson, Olivier Matz, Ray Kinsella 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] cmdline: add function to verify valid commands 2022-05-20 14:56 ` [PATCH 1/2] cmdline: add function to verify valid commands Bruce Richardson @ 2022-05-24 14:57 ` Ray Kinsella 0 siblings, 0 replies; 12+ messages in thread From: Ray Kinsella @ 2022-05-24 14:57 UTC (permalink / raw) To: Bruce Richardson; +Cc: dev, stable, weiyuanx.li, xuemingx.zhang, Olivier Matz 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> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/2] test: use cmdline library to validate args 2022-05-20 14:56 [PATCH 0/2] fix uncallable unit tests (Bugzilla 1002) Bruce Richardson 2022-05-20 14:56 ` [PATCH 1/2] cmdline: add function to verify valid commands Bruce Richardson @ 2022-05-20 14:56 ` Bruce Richardson 2022-05-20 15:12 ` [PATCH v2 0/2] fix uncallable unit tests (Bugzilla 1002) Bruce Richardson 2 siblings, 0 replies; 12+ messages in thread From: Bruce Richardson @ 2022-05-20 14:56 UTC (permalink / raw) To: dev Cc: stable, weiyuanx.li, xuemingx.zhang, Bruce Richardson, Pablo de Lara, Harry van Haaren, Aaron Conole 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 0/2] fix uncallable unit tests (Bugzilla 1002) 2022-05-20 14:56 [PATCH 0/2] fix uncallable unit tests (Bugzilla 1002) Bruce Richardson 2022-05-20 14:56 ` [PATCH 1/2] cmdline: add function to verify valid commands Bruce Richardson 2022-05-20 14:56 ` [PATCH 2/2] test: use cmdline library to validate args Bruce Richardson @ 2022-05-20 15:12 ` Bruce Richardson 2022-05-20 15:12 ` [PATCH v2 1/2] cmdline: add function to verify valid commands Bruce Richardson 2022-05-20 15:12 ` [PATCH v2 2/2] test: use cmdline library to validate args Bruce Richardson 2 siblings, 2 replies; 12+ messages in thread From: Bruce Richardson @ 2022-05-20 15:12 UTC (permalink / raw) To: dev; +Cc: stable, weiyuanx.li, 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/2] cmdline: add function to verify valid commands 2022-05-20 15:12 ` [PATCH v2 0/2] fix uncallable unit tests (Bugzilla 1002) Bruce Richardson @ 2022-05-20 15:12 ` Bruce Richardson 2022-05-23 6:52 ` Li, WeiyuanX 2022-06-07 8:08 ` Olivier Matz 2022-05-20 15:12 ` [PATCH v2 2/2] test: use cmdline library to validate args Bruce Richardson 1 sibling, 2 replies; 12+ messages in thread From: Bruce Richardson @ 2022-05-20 15:12 UTC (permalink / raw) To: dev; +Cc: stable, weiyuanx.li, Bruce Richardson, Olivier Matz, Ray Kinsella 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v2 1/2] cmdline: add function to verify valid commands 2022-05-20 15:12 ` [PATCH v2 1/2] cmdline: add function to verify valid commands Bruce Richardson @ 2022-05-23 6:52 ` Li, WeiyuanX 2022-06-07 8:08 ` Olivier Matz 1 sibling, 0 replies; 12+ messages in thread From: Li, WeiyuanX @ 2022-05-23 6:52 UTC (permalink / raw) To: Richardson, Bruce, dev; +Cc: stable, Olivier Matz, Ray Kinsella > -----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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] cmdline: add function to verify valid commands 2022-05-20 15:12 ` [PATCH v2 1/2] cmdline: add function to verify valid commands Bruce Richardson 2022-05-23 6:52 ` Li, WeiyuanX @ 2022-06-07 8:08 ` Olivier Matz 2022-06-10 14:08 ` Bruce Richardson 1 sibling, 1 reply; 12+ messages in thread From: Olivier Matz @ 2022-06-07 8:08 UTC (permalink / raw) To: Bruce Richardson; +Cc: dev, stable, weiyuanx.li, Ray Kinsella 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> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] cmdline: add function to verify valid commands 2022-06-07 8:08 ` Olivier Matz @ 2022-06-10 14:08 ` Bruce Richardson 2022-06-10 14:21 ` Olivier Matz 0 siblings, 1 reply; 12+ messages in thread From: Bruce Richardson @ 2022-06-10 14:08 UTC (permalink / raw) To: Olivier Matz; +Cc: dev, stable, weiyuanx.li, Ray Kinsella 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> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] cmdline: add function to verify valid commands 2022-06-10 14:08 ` Bruce Richardson @ 2022-06-10 14:21 ` Olivier Matz 0 siblings, 0 replies; 12+ messages in thread From: Olivier Matz @ 2022-06-10 14:21 UTC (permalink / raw) To: Bruce Richardson; +Cc: dev, stable, weiyuanx.li, Ray Kinsella 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> > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] test: use cmdline library to validate args 2022-05-20 15:12 ` [PATCH v2 0/2] fix uncallable unit tests (Bugzilla 1002) Bruce Richardson 2022-05-20 15:12 ` [PATCH v2 1/2] cmdline: add function to verify valid commands Bruce Richardson @ 2022-05-20 15:12 ` Bruce Richardson 2022-06-07 8:08 ` Olivier Matz 1 sibling, 1 reply; 12+ messages in thread From: Bruce Richardson @ 2022-05-20 15:12 UTC (permalink / raw) To: dev Cc: stable, weiyuanx.li, Bruce Richardson, Pablo de Lara, Harry van Haaren, Aaron Conole 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] test: use cmdline library to validate args 2022-05-20 15:12 ` [PATCH v2 2/2] test: use cmdline library to validate args Bruce Richardson @ 2022-06-07 8:08 ` Olivier Matz 0 siblings, 0 replies; 12+ messages in thread From: Olivier Matz @ 2022-06-07 8:08 UTC (permalink / raw) To: Bruce Richardson Cc: dev, stable, weiyuanx.li, Pablo de Lara, Harry van Haaren, Aaron Conole 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> ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-06-10 14:21 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-20 14:56 [PATCH 0/2] fix uncallable unit tests (Bugzilla 1002) Bruce Richardson 2022-05-20 14:56 ` [PATCH 1/2] cmdline: add function to verify valid commands Bruce Richardson 2022-05-24 14:57 ` Ray Kinsella 2022-05-20 14:56 ` [PATCH 2/2] test: use cmdline library to validate args Bruce Richardson 2022-05-20 15:12 ` [PATCH v2 0/2] fix uncallable unit tests (Bugzilla 1002) Bruce Richardson 2022-05-20 15:12 ` [PATCH v2 1/2] cmdline: add function to verify valid commands Bruce Richardson 2022-05-23 6:52 ` Li, WeiyuanX 2022-06-07 8:08 ` Olivier Matz 2022-06-10 14:08 ` Bruce Richardson 2022-06-10 14:21 ` Olivier Matz 2022-05-20 15:12 ` [PATCH v2 2/2] test: use cmdline library to validate args Bruce Richardson 2022-06-07 8:08 ` Olivier Matz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).