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 97B204318E; Tue, 17 Oct 2023 18:24:04 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 235BA40273; Tue, 17 Oct 2023 18:24:04 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id 8BBF040270 for ; Tue, 17 Oct 2023 18:24:02 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1697559841; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=h5rnqXvII/aV/Yyom4bj8CsCNg9u9n4jgNXp54GANEI=; b=Vlbhzl+WT2L0st+1quqLhFLy6xycMbknuR20CXjapk8FAC4rf39uMkzrWPenHRxZftM3/o 11dNI3JQXdrVc2hbGQqtloS+q+E5tbtaHv4MBUp+DKQIDIM73Mv7IoTAXFqYnXWIKDVKUW njKjK4ANBd56/5dtOaGWnJUFDm8MMuQ= Received: from mail-lj1-f200.google.com (mail-lj1-f200.google.com [209.85.208.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-264-lbgVLm2aNOe-p23tpRjNgg-1; Tue, 17 Oct 2023 12:24:00 -0400 X-MC-Unique: lbgVLm2aNOe-p23tpRjNgg-1 Received: by mail-lj1-f200.google.com with SMTP id 38308e7fff4ca-2c50257772bso50268421fa.3 for ; Tue, 17 Oct 2023 09:23:59 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697559839; x=1698164639; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=h5rnqXvII/aV/Yyom4bj8CsCNg9u9n4jgNXp54GANEI=; b=SJE71rw6Oe3MMJ3b7jLTYNx+yT4W1ogzZBzyE1iS8BkJQpGsv41pdYYasdi/M2Ldr4 qeexG2UiwosUY/cW8av8Hz09aLsiRj9aHLOb/C1uiyeoXpTshcvAocy2aHlB7I8YRge9 jYMYKMAgmCpyJufH7mWQ6ZMHOvKfLWNLLLzRTxchhvJOusuU3yl8Jt44pIh4+jJL9I3S zExUGE8ug++lkqn/BThFOMHq3I3ss6DKFqtVlat1ExIUI+PWv9eA87FMJNqgacR4xcXO rBy8vSqnVa88KXIz8UJggocVBrMHcxcNtF58CZjLXiTnptD+SBlrpRCJ+y9rdBuvc33b 5tTA== X-Gm-Message-State: AOJu0YwWDGyNEAymF8xVxwiJJdNvBV+OoYAU/4OFf9wsL6G349SVUzV0 1DiNcVWJ69YHHq/Pf48Jego+2vgcwRvNRsZ1G2qbC2PGtRUAft17FitQFX8OufXD4PQDEl7y5pv RHT0SeGduykXU5Aj0i+nikrKmBuI= X-Received: by 2002:a2e:9107:0:b0:2c5:1c9d:7f81 with SMTP id m7-20020a2e9107000000b002c51c9d7f81mr2060519ljg.32.1697559838818; Tue, 17 Oct 2023 09:23:58 -0700 (PDT) X-Google-Smtp-Source: AGHT+IH2CTdiK52BeMjRPucB2Vz6CTS5A75pIk0a56C4gqCep5AST8qTG4jIFGcQSruqQ+GGZsAbJuGZ490GJOrBSQw= X-Received: by 2002:a2e:9107:0:b0:2c5:1c9d:7f81 with SMTP id m7-20020a2e9107000000b002c51c9d7f81mr2060506ljg.32.1697559838359; Tue, 17 Oct 2023 09:23:58 -0700 (PDT) MIME-Version: 1.0 References: <20230802170052.955323-1-bruce.richardson@intel.com> <20231016140612.664853-1-bruce.richardson@intel.com> In-Reply-To: From: David Marchand Date: Tue, 17 Oct 2023 18:23:47 +0200 Message-ID: Subject: Re: [PATCH v4 0/7] document and simplify use of cmdline To: Bruce Richardson Cc: dev@dpdk.org, rjarry@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 Tue, Oct 17, 2023 at 10:29=E2=80=AFAM Bruce Richardson wrote: > > > > - testpmd accepts both "help" and "help
" commands. > > But the cmdline library does not provide a way (afair) for specifiying > > this "optional" aspect. > > > > And it can't be expressed with this series current syntax since > > generated symbols would conflict if we ask for two "help" commands. > > > > My quick hack was to introduce a way to select the prefix of the > > generated symbols. > > There may be a better way, like finding a better discriminant for > > naming symbols... > > But, on the other hand, the symbols prefix might be something a > > developer wants to control (instead of currently hardcoded cmd_ > > prefix). > > > > @@ -20,6 +20,12 @@ def process_command(tokens, cfile, comment): > > """Generate the structures and definitions for a single command.""= " > > name =3D [] > > > > + prefix, sep, cmd_name =3D tokens[0].partition(':') > > + if cmd_name: > > + tokens[0] =3D cmd_name > > + else: > > + prefix =3D 'cmd' > > + > > if tokens[0].startswith('<'): > > print('Error: each command must start with at least one > > literal string', file=3Dsys.stderr) > > sys.exit(1) > > > > (etc... I am not copying the rest of the diff) > > > > I then used as: > > > > cmd_brief:help # help: Show help > > help section # help: Show help > > > > Interesting. I actually had no plans to even consider moving something li= ke > testpmd over. However, this is an interesting one, though I'm not really Given the extensive use of the cmdline library in testpmd, it is a good way to identify the limits of this series :-). > sure I like it that much as a feature :-) I rather like having unique > prefixes for each command. I wasn't actually aware of the testpmd "help >
" command at all. I will have to look into it. Let me propose an alternative hack. I mentionned previously that we could have a better namespace / discriminant for those symbols, and it seems easier than I thought: @@ -25,8 +25,10 @@ def process_command(tokens, cfile, comment): sys.exit(1) for t in tokens: if t.startswith('<'): - break - name.append(t) + t_type, t_name =3D t[1:].split('>') + name.append(t_name) + else: + name.append(t) name =3D '_'.join(name) result_struct =3D [] With this, any command implementation symbol has the full chain of token names as a prefix which will ensure there is no conflict. WDYT? help # help: Show help help section # help: Show help Results in: cmd_help_parsed(void *parsed_result, struct cmdline *cl, void *data); struct cmd_help_result { static cmdline_parse_token_string_t cmd_help_help_tok =3D TOKEN_STRING_INITIALIZER(struct cmd_help_result, help, "help"); static cmdline_parse_inst_t cmd_help =3D { .f =3D cmd_help_parsed, (void *)&cmd_help_help_tok, And: cmd_help_section_parsed(void *parsed_result, struct cmdline *cl, void *data= ); struct cmd_help_section_result { static cmdline_parse_token_string_t cmd_help_section_help_tok =3D TOKEN_STRING_INITIALIZER(struct cmd_help_section_result, help, "help"); static cmdline_parse_token_string_t cmd_help_section_section_tok =3D TOKEN_STRING_INITIALIZER(struct cmd_help_section_result, section, NULL)= ; static cmdline_parse_inst_t cmd_help_section =3D { .f =3D cmd_help_section_parsed, (void *)&cmd_help_section_help_tok, (void *)&cmd_help_section_section_tok, > > > > > - Multi choice fixed strings is something that is often used in > > testpmd, like here, in the help
command. > > Here is my quick hack: > > > > diff --git a/buildtools/dpdk-cmdline-gen.py b/buildtools/dpdk-cmdline-g= en.py > > index 3b41fb0493..e8c9e079de 100755 > > --- a/buildtools/dpdk-cmdline-gen.py > > +++ b/buildtools/dpdk-cmdline-gen.py > > @@ -35,7 +35,11 @@ def process_command(tokens, cfile, comment): > > for t in tokens: > > if t.startswith('<'): > > t_type, t_name =3D t[1:].split('>') > > - t_val =3D 'NULL' > > + if len(t_type.split('(')) =3D=3D 2: > > + t_type, t_val =3D t_type.split('(') > > + t_val =3D '"' + t_val.split(')')[0] + '"' > > + else: > > + t_val =3D 'NULL' > > else: > > t_type =3D 'STRING' > > t_name =3D t > > @@ -113,7 +117,7 @@ def process_commands(infile, hfile, cfile, ctxname)= : > > continue > > if '#' not in line: > > line =3D line + '#' # ensure split always works, even if > > no help text > > - tokens, comment =3D line.split('#', 1) > > + tokens, comment =3D line.rsplit('#', 1) > > instances.append(process_command(tokens.strip().split(), > > cfile, comment.strip())) > > > > print(f'static __rte_used cmdline_parse_ctx_t {ctxname}[] =3D {{') > > > > > > Which translates as: > > cmd_brief:help # help: Show help > > help section # help: Show hel= p > > > > +1 > I was actualy thinking that adding support for multi-choice fixed strings > is something we should add. One thought that I had was that "#" is not a > particularly good choice of separator here. While, as you show, it can be > made to work; I think - since we are defining our own syntax here - that = it > would be both simpler for the script, and simpler for the user, to have "= |" > as the option separator. It should be familiar for everyone as an option > separator from regexes, unlike "#" which is more familar for comments. > > So what about: > help <|all|control|display|config|ports|>section I don't like using | as it gives the false impression regexp are supported.= .. > > By starting with the separator, we should avoid having to provide the > STRING type at all. ... and as a consequence, I find <|all confusing, it is like an empty value would be acceptable. About skipping the token type for such lists, I had considered it, but I thought other types took an optional list of allowed values... Now looking at the cmdline types, it is not the case. Maybe I mixed with some other cli framework I played with in the past... All of this to say, ok for me to omit the type. > > To my previous point on not liking to have a prefix-specifier, the > alternative to making testpmd work with the script is to tweak very > slightly the "help
". > > help # show general help > help on <|all|control|display|config|ports|>section > > By making the command "help on ports" rather than "help ports" we would > avoid the need for the prefix syntax. There are other cases where a "chain of command" returns the value of a parameter. And the same parameter may be set via "chain of command value". --=20 David Marchand