| Message ID | 20220520151240.139566-2-bruce.richardson@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Thomas Monjalon |
| Headers | show |
| Series | fix uncallable unit tests (Bugzilla 1002) | expand |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
> -----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
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>
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> >
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> > >
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: *; };
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(-)