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 8F2DEA0554 for ; Fri, 10 Jun 2022 16:08:57 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 808FB427F9; Fri, 10 Jun 2022 16:08:57 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by mails.dpdk.org (Postfix) with ESMTP id 412BC40689; Fri, 10 Jun 2022 16:08:55 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1654870135; x=1686406135; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=zz5uGXMae8I94pHXYVy92B7cnhZVm4vKsBbCdY75IUY=; b=MjwtTumcbqJ+YeMFPl4gQLltKKtHysTHK0lNpqP8sksdsNpMWgpFzFsN tqFSMjv1e861mNg6RqxIiMa0lbMIylvuIuO/QzeMN8HAMBvuUpHuRGz60 mhkP4y8LgDeLnmKNO4Uv55cOePxQt0zn1K9K9cCpwkBGpnt0y+M0nKI2U +ij2wjAIDP3zxMYU/gSCPNOU6FY+C31GFCxjzF3bsUllBJNIHYIx8PZ+C 4hg2TIV15IAz2znxGF08kVOCnX3lpg+jA8RKo3qL9pHkk0MY8CwfCHD0F 6ZdJVsoJ431nHGYseM81PXRi6G/HY6WD1KsahXWXw1/faxOxncPthdUAg Q==; X-IronPort-AV: E=McAfee;i="6400,9594,10373"; a="275171110" X-IronPort-AV: E=Sophos;i="5.91,290,1647327600"; d="scan'208";a="275171110" Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Jun 2022 07:08:54 -0700 X-IronPort-AV: E=Sophos;i="5.91,290,1647327600"; d="scan'208";a="828269861" Received: from bricha3-mobl.ger.corp.intel.com ([10.55.133.106]) by fmsmga006-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 10 Jun 2022 07:08:52 -0700 Date: Fri, 10 Jun 2022 15:08:49 +0100 From: Bruce Richardson To: Olivier Matz 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: X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org 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 > > --- > > 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) > 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 > > + > > #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 >