From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id B5571A0543; Tue, 7 Jun 2022 10:08:32 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 59B5E4021D; Tue, 7 Jun 2022 10:08:32 +0200 (CEST) Received: from smtpservice.6wind.com (unknown [185.13.181.2]) by mails.dpdk.org (Postfix) with ESMTP id 3D82840156; Tue, 7 Jun 2022 10:08:31 +0200 (CEST) Received: from 6wind.com (unknown [10.17.250.37]) by smtpservice.6wind.com (Postfix) with ESMTP id 19B1C60020; Tue, 7 Jun 2022 10:08:31 +0200 (CEST) Date: Tue, 7 Jun 2022 10:08:30 +0200 From: Olivier Matz To: Bruce Richardson Cc: dev@dpdk.org, stable@dpdk.org, weiyuanx.li@intel.com, Ray Kinsella Subject: Re: [PATCH v2 1/2] cmdline: add function to verify valid commands Message-ID: References: <20220520145631.137962-1-bruce.richardson@intel.com> <20220520151240.139566-1-bruce.richardson@intel.com> <20220520151240.139566-2-bruce.richardson@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220520151240.139566-2-bruce.richardson@intel.com> X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 > --- > 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 > #include > #include > +#include > > #include > > @@ -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 > + > #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