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 93E7EA0553; Fri, 10 Jun 2022 16:21:34 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E6BF4410E5; Fri, 10 Jun 2022 16:21:33 +0200 (CEST) Received: from mail-wm1-f43.google.com (mail-wm1-f43.google.com [209.85.128.43]) by mails.dpdk.org (Postfix) with ESMTP id 2EE1D4069C for ; Fri, 10 Jun 2022 16:21:33 +0200 (CEST) Received: by mail-wm1-f43.google.com with SMTP id z17so9133509wmi.1 for ; Fri, 10 Jun 2022 07:21:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=7D0TtzN0ZwMBTxZym0hNj2lIariOezb033ON8TQalfg=; b=S7Kqwj2VaXdumacpvoIV7IoOybvqqke2AqwPZeX7HOiR7EiUtOrM4/LNviuimnB/3q boXleQKItb+rGBbfDpS80qtSSYBWpla0kUidIsJLJTz2NYzO4J88YRA4/VUBDF8Mv1G7 hwYpa7uem2fIwbGNRpLEekB07xJBF7MjDjXIn9q1uT8+09GAdWqgMO8PO2hES4zRR1fj wEt+kACPjRHEMDNxDwuD13PO3Il4O2XVDlZkOzv5+tfrCrGv+FwadQZ0PrAQS0brWSfD vtIMWC8Yijlh9wkkvCq0CqFbD/IXF2sEwXutlxxa6nwr+L2G7XcNayw3vbQyNIGyouJZ Ceag== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=7D0TtzN0ZwMBTxZym0hNj2lIariOezb033ON8TQalfg=; b=ueY26xa1b6vVR3hXMjelA4c5/ttMulgqopx5+8fLJgEWuVNVmFLJe9cZj/ZgZELqKq xF4vtd5egv804GwYOQ7EUW78sigqPbinpEJfomrSNJ3ZoPljAy1v5AQmR+xSiSISj1/J gBB8EjjAbHO4WX59t6IcVkpgAGsulOFMvjFpSHx/0QGDlGYubSwlZDbXYQUloWshlgHg devnOYgFkf9hXeNBt2XGSw9Ow6djUsu0ms8K/xv+b5UkPlN7KDW1hIMs2oEc+G5QXbOc SLSQGufoBy3I1BNcM5/c7bzlh7sjiao0yyqkSqt9hwSmusb9lGCA/SnG3rEnWMS5qwSO wWIQ== X-Gm-Message-State: AOAM532qkHmMDqB1L8hSd4VtxplI2sOA7hFwxHDZ6E9wZ+BQKn+Tij4n 0kOdcZC/4fzh70eGi7Sg2VnsPw== X-Google-Smtp-Source: ABdhPJzQ9EXR+gf9iIkwlp78x4WbHMGqZe4v0DVeMJozHyn0p4ZHzHBEXY8PtS2De1fSuZbu5OgXGA== X-Received: by 2002:a1c:cc05:0:b0:39c:7477:3f24 with SMTP id h5-20020a1ccc05000000b0039c74773f24mr13564wmb.190.1654870892815; Fri, 10 Jun 2022 07:21:32 -0700 (PDT) Received: from 6wind.com ([2a01:e0a:5ac:6460:c065:401d:87eb:9b25]) by smtp.gmail.com with ESMTPSA id 131-20020a1c0289000000b0039c5fb1f592sm881579wmc.14.2022.06.10.07.21.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 10 Jun 2022 07:21:32 -0700 (PDT) Date: Fri, 10 Jun 2022 16:21:31 +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: 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 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 > > > --- > > > 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 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 > > > + > > > #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 > >