[v4,0/7] document and simplify use of cmdline

  The DPDK commandline library is widely used by apps and examples within
DPDK, but it is not documented in our programmers guide and it requires
a lot of boilerplate code definitions in order to used. We can improve
this situation by creating a simple python script to automatically
generate the boilerplate from a list of commands.

This patchset contains a new documentation chapter on cmdline library,
going through step-by-step how to add commands and create the necessary
token lists and parse contexts.

Following that initial doc patch, the set then contains a
boilerplate-generating script, as well as a set of four patches showing
its use, by converting four examples to use the script instead of
having the hard-coded boilerplate. Once the script is used, adding a new
command becomes as simple as adding the desired command to the .list
file, and then writing the required function which will be called for
that command. No other boilerplate coding is necessary.

Script obviously does not cover the full range of capabilities of the
commandline lib, but does cover the most used parts. The code-saving to
each of the examples by auto-generating the boilerplate is significant,
and probably more examples with commandlines can be converted over in
future.

The "cmdline" example itself, is not converted over, as it should
probably remain as a simple example of direct library use without the
script.

V4:
* Reworked script following feedback from Robin J.
   - formatted code with black
   - used multi-line strings
   - replaced sys.exit(1) with proper error handling
   - per-command function returns lists rather than printing directly
   - other small cleanups
* Added change to CI script so the cmdline script is in PATH
* Converted VDPA example to script, saving another 100 LOC

V3:
* Added lots of documentation
* Added support for help text for each command
* Cleaned up script a little so it passes pycodestyle and most flake8
  checks, when line-length is set to max 100.
* Removed RFC tag, as I consider this patchset stable enough for
  consideration in a release.

V2-RFC:
* Add support for IP addresses in commands
* Move to buildtools directory and make installable
* Convert 3 examples to use script, and eliminate their boilerplate


Bruce Richardson (7):
  doc/prog_guide: new chapter on cmdline library
  buildtools: script to generate cmdline boilerplate
  ci: allow use of DPDK tools when building examples
  examples/simple_mp: auto-generate cmdline boilerplate
  examples/hotplug_mp: auto-generate cmdline boilerplate
  examples/bond: auto-generate cmdline boilerplate
  examples/vdpa: auto-generate cmdline boilerplate

 .ci/linux-build.sh                            |   1 +
 app/test/commands.c                           |   2 +
 buildtools/dpdk-cmdline-gen.py                | 190 +++++++
 buildtools/meson.build                        |   7 +
 doc/guides/prog_guide/cmdline.rst             | 466 ++++++++++++++++++
 doc/guides/prog_guide/index.rst               |   1 +
 examples/bond/Makefile                        |  12 +-
 examples/bond/commands.list                   |   6 +
 examples/bond/main.c                          | 161 +-----
 examples/bond/main.h                          |  10 -
 examples/bond/meson.build                     |   8 +
 examples/multi_process/hotplug_mp/Makefile    |  12 +-
 examples/multi_process/hotplug_mp/commands.c  | 147 +-----
 examples/multi_process/hotplug_mp/commands.h  |  10 -
 .../multi_process/hotplug_mp/commands.list    |   5 +
 examples/multi_process/hotplug_mp/meson.build |   9 +
 examples/multi_process/simple_mp/Makefile     |  12 +-
 examples/multi_process/simple_mp/meson.build  |   9 +
 .../multi_process/simple_mp/mp_commands.c     | 106 +---
 .../multi_process/simple_mp/mp_commands.h     |  14 -
 .../multi_process/simple_mp/mp_commands.list  |   3 +
 examples/vdpa/Makefile                        |  12 +-
 examples/vdpa/commands.list                   |   5 +
 examples/vdpa/main.c                          | 131 +----
 examples/vdpa/meson.build                     |   7 +
 25 files changed, 796 insertions(+), 550 deletions(-)
 create mode 100755 buildtools/dpdk-cmdline-gen.py
 create mode 100644 doc/guides/prog_guide/cmdline.rst
 create mode 100644 examples/bond/commands.list
 delete mode 100644 examples/bond/main.h
 delete mode 100644 examples/multi_process/hotplug_mp/commands.h
 create mode 100644 examples/multi_process/hotplug_mp/commands.list
 delete mode 100644 examples/multi_process/simple_mp/mp_commands.h
 create mode 100644 examples/multi_process/simple_mp/mp_commands.list
 create mode 100644 examples/vdpa/commands.list

--
2.39.2
  

Hi Bruce,

On Mon, Oct 16, 2023 at 4:06 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> The DPDK commandline library is widely used by apps and examples within
> DPDK, but it is not documented in our programmers guide and it requires
> a lot of boilerplate code definitions in order to used. We can improve
> this situation by creating a simple python script to automatically
> generate the boilerplate from a list of commands.
>
> This patchset contains a new documentation chapter on cmdline library,
> going through step-by-step how to add commands and create the necessary
> token lists and parse contexts.
>
> Following that initial doc patch, the set then contains a
> boilerplate-generating script, as well as a set of four patches showing
> its use, by converting four examples to use the script instead of
> having the hard-coded boilerplate. Once the script is used, adding a new
> command becomes as simple as adding the desired command to the .list
> file, and then writing the required function which will be called for
> that command. No other boilerplate coding is necessary.
>
> Script obviously does not cover the full range of capabilities of the
> commandline lib, but does cover the most used parts. The code-saving to
> each of the examples by auto-generating the boilerplate is significant,
> and probably more examples with commandlines can be converted over in
> future.

This is not something blocking for the series, but I had a quick try
with testpmd to see where this series stands.
I hit, right off the bat, some of those limitations, but I think there
are not that difficult to deal with.


- testpmd accepts both "help" and "help <section>" 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 = []

+    prefix, sep, cmd_name = tokens[0].partition(':')
+    if cmd_name:
+        tokens[0] = cmd_name
+    else:
+        prefix = 'cmd'
+
     if tokens[0].startswith('<'):
         print('Error: each command must start with at least one
literal string', file=sys.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 <STRING>section # help: Show help


- Multi choice fixed strings is something that is often used in
testpmd, like here, in the help <section> command.
Here is my quick hack:

diff --git a/buildtools/dpdk-cmdline-gen.py b/buildtools/dpdk-cmdline-gen.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 = t[1:].split('>')
-            t_val = 'NULL'
+            if len(t_type.split('(')) == 2:
+                t_type, t_val = t_type.split('(')
+                t_val = '"' + t_val.split(')')[0] + '"'
+            else:
+                t_val = 'NULL'
         else:
             t_type = 'STRING'
             t_name = t
@@ -113,7 +117,7 @@ def process_commands(infile, hfile, cfile, ctxname):
             continue
         if '#' not in line:
             line = line + '#'  # ensure split always works, even if
no help text
-        tokens, comment = line.split('#', 1)
+        tokens, comment = line.rsplit('#', 1)
         instances.append(process_command(tokens.strip().split(),
cfile, comment.strip()))

     print(f'static __rte_used cmdline_parse_ctx_t {ctxname}[] = {{')


Which translates as:
cmd_brief:help # help: Show help
help <STRING(all#control#display#config#ports)>section # help: Show help


>
> The "cmdline" example itself, is not converted over, as it should
> probably remain as a simple example of direct library use without the
> script.

+1
  

On Tue, Oct 17, 2023 at 09:10:51AM +0200, David Marchand wrote:
> Hi Bruce,
> 
> On Mon, Oct 16, 2023 at 4:06 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > The DPDK commandline library is widely used by apps and examples within
> > DPDK, but it is not documented in our programmers guide and it requires
> > a lot of boilerplate code definitions in order to used. We can improve
> > this situation by creating a simple python script to automatically
> > generate the boilerplate from a list of commands.
> >
> > This patchset contains a new documentation chapter on cmdline library,
> > going through step-by-step how to add commands and create the necessary
> > token lists and parse contexts.
> >
> > Following that initial doc patch, the set then contains a
> > boilerplate-generating script, as well as a set of four patches showing
> > its use, by converting four examples to use the script instead of
> > having the hard-coded boilerplate. Once the script is used, adding a new
> > command becomes as simple as adding the desired command to the .list
> > file, and then writing the required function which will be called for
> > that command. No other boilerplate coding is necessary.
> >
> > Script obviously does not cover the full range of capabilities of the
> > commandline lib, but does cover the most used parts. The code-saving to
> > each of the examples by auto-generating the boilerplate is significant,
> > and probably more examples with commandlines can be converted over in
> > future.
> 
> This is not something blocking for the series, but I had a quick try
> with testpmd to see where this series stands.
> I hit, right off the bat, some of those limitations, but I think there
> are not that difficult to deal with.
> 

Thanks for investigating. Item #2 below is something I'd already been
thinking of, but had not yet implemented. The hope was to get the basics in
first and iterate thereafter, rather than trying to get it all in in one
go. However, if I have the chance, and depending on how long before this
set gets merged, I might have a try at a respin with an additional feature.

> 
> - testpmd accepts both "help" and "help <section>" 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 = []
> 
> +    prefix, sep, cmd_name = tokens[0].partition(':')
> +    if cmd_name:
> +        tokens[0] = cmd_name
> +    else:
> +        prefix = 'cmd'
> +
>      if tokens[0].startswith('<'):
>          print('Error: each command must start with at least one
> literal string', file=sys.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 <STRING>section # help: Show help
> 

Interesting. I actually had no plans to even consider moving something like
testpmd over. However, this is an interesting one, though I'm not really
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
<section>" command at all. I will have to look into it.

> 
> - Multi choice fixed strings is something that is often used in
> testpmd, like here, in the help <section> command.
> Here is my quick hack:
> 
> diff --git a/buildtools/dpdk-cmdline-gen.py b/buildtools/dpdk-cmdline-gen.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 = t[1:].split('>')
> -            t_val = 'NULL'
> +            if len(t_type.split('(')) == 2:
> +                t_type, t_val = t_type.split('(')
> +                t_val = '"' + t_val.split(')')[0] + '"'
> +            else:
> +                t_val = 'NULL'
>          else:
>              t_type = 'STRING'
>              t_name = t
> @@ -113,7 +117,7 @@ def process_commands(infile, hfile, cfile, ctxname):
>              continue
>          if '#' not in line:
>              line = line + '#'  # ensure split always works, even if
> no help text
> -        tokens, comment = line.split('#', 1)
> +        tokens, comment = line.rsplit('#', 1)
>          instances.append(process_command(tokens.strip().split(),
> cfile, comment.strip()))
> 
>      print(f'static __rte_used cmdline_parse_ctx_t {ctxname}[] = {{')
> 
> 
> Which translates as:
> cmd_brief:help # help: Show help
> help <STRING(all#control#display#config#ports)>section # help: Show help
> 

+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

By starting with the separator, we should avoid having to provide the
STRING type at all.

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 <section>".

  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.

WDYT?

/Bruce
  

On Tue, Oct 17, 2023 at 09:29:34AM +0100, Bruce Richardson wrote:
> On Tue, Oct 17, 2023 at 09:10:51AM +0200, David Marchand wrote:

<snip>

> > 
> > - Multi choice fixed strings is something that is often used in
> > testpmd, like here, in the help <section> command.  Here is my quick
> > hack:
> > 
> > diff --git a/buildtools/dpdk-cmdline-gen.py
> > b/buildtools/dpdk-cmdline-gen.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 = t[1:].split('>') -            t_val
> > = 'NULL' +            if len(t_type.split('(')) == 2: +
> > t_type, t_val = t_type.split('(') +                t_val = '"' +
> > t_val.split(')')[0] + '"' +            else: +                t_val =
> > 'NULL' else: t_type = 'STRING' t_name = t @@ -113,7 +117,7 @@ def
> > process_commands(infile, hfile, cfile, ctxname): continue if '#' not in
> > line: line = line + '#'  # ensure split always works, even if no help
> > text -        tokens, comment = line.split('#', 1) +        tokens,
> > comment = line.rsplit('#', 1)
> > instances.append(process_command(tokens.strip().split(), cfile,
> > comment.strip()))
> > 
> >      print(f'static __rte_used cmdline_parse_ctx_t {ctxname}[] = {{')
> > 
> > 
> > Which translates as: cmd_brief:help # help: Show help help
> > <STRING(all#control#display#config#ports)>section # help: Show help
> > 
> 
> +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
> 
> By starting with the separator, we should avoid having to provide the
> STRING type at all.
> 

This (my suggestion) is now prototyped in V5. I've kept it as a separate
patch so it's easily to review and discuss without affecting the rest of
the set. To test it out, I've converted over the ntb example app which uses
option lists.

/Bruce
  

On Tue, Oct 17, 2023 at 10:29 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> >
> > - testpmd accepts both "help" and "help <section>" 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 = []
> >
> > +    prefix, sep, cmd_name = tokens[0].partition(':')
> > +    if cmd_name:
> > +        tokens[0] = cmd_name
> > +    else:
> > +        prefix = 'cmd'
> > +
> >      if tokens[0].startswith('<'):
> >          print('Error: each command must start with at least one
> > literal string', file=sys.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 <STRING>section # help: Show help
> >
>
> Interesting. I actually had no plans to even consider moving something like
> 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
> <section>" 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 = t[1:].split('>')
+            name.append(t_name)
+        else:
+            name.append(t)
     name = '_'.join(name)

     result_struct = []

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 <STRING>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 =
    TOKEN_STRING_INITIALIZER(struct cmd_help_result, help, "help");
static cmdline_parse_inst_t cmd_help = {
    .f = 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 =
    TOKEN_STRING_INITIALIZER(struct cmd_help_section_result, help, "help");
static cmdline_parse_token_string_t cmd_help_section_section_tok =
    TOKEN_STRING_INITIALIZER(struct cmd_help_section_result, section, NULL);
static cmdline_parse_inst_t cmd_help_section = {
    .f = 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 <section> command.
> > Here is my quick hack:
> >
> > diff --git a/buildtools/dpdk-cmdline-gen.py b/buildtools/dpdk-cmdline-gen.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 = t[1:].split('>')
> > -            t_val = 'NULL'
> > +            if len(t_type.split('(')) == 2:
> > +                t_type, t_val = t_type.split('(')
> > +                t_val = '"' + t_val.split(')')[0] + '"'
> > +            else:
> > +                t_val = 'NULL'
> >          else:
> >              t_type = 'STRING'
> >              t_name = t
> > @@ -113,7 +117,7 @@ def process_commands(infile, hfile, cfile, ctxname):
> >              continue
> >          if '#' not in line:
> >              line = line + '#'  # ensure split always works, even if
> > no help text
> > -        tokens, comment = line.split('#', 1)
> > +        tokens, comment = line.rsplit('#', 1)
> >          instances.append(process_command(tokens.strip().split(),
> > cfile, comment.strip()))
> >
> >      print(f'static __rte_used cmdline_parse_ctx_t {ctxname}[] = {{')
> >
> >
> > Which translates as:
> > cmd_brief:help # help: Show help
> > help <STRING(all#control#display#config#ports)>section # help: Show help
> >
>
> +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 <section>".
>
>   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 <UINT>value".
  

On Tue, Oct 17, 2023 at 06:23:47PM +0200, David Marchand wrote:
> On Tue, Oct 17, 2023 at 10:29 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> > >
> > > - testpmd accepts both "help" and "help <section>" 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 = []
> > >
> > > +    prefix, sep, cmd_name = tokens[0].partition(':')
> > > +    if cmd_name:
> > > +        tokens[0] = cmd_name
> > > +    else:
> > > +        prefix = 'cmd'
> > > +
> > >      if tokens[0].startswith('<'):
> > >          print('Error: each command must start with at least one
> > > literal string', file=sys.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 <STRING>section # help: Show help
> > >
> >
> > Interesting. I actually had no plans to even consider moving something like
> > 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 :-).
> 

It is indeed. That was never meant to be the target of this series, rather
it was to make writing new apps simpler. However, if we have a path to
getting there, then I suppose we might as well continue.

> 
> > 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
> > <section>" 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 = t[1:].split('>')
> +            name.append(t_name)
> +        else:
> +            name.append(t)
>      name = '_'.join(name)
> 
>      result_struct = []
> 
> With this, any command implementation symbol has the full chain of
> token names as a prefix which will ensure there is no conflict.
> WDYT?
> 

I actually think I like this less than the previous proposal. :-(

The main reason for that is that we would need to decide immediately
whether to go with this or not, as it would break compatibility with what
is already proposed - because the function and struct names would get
longer. The other scheme you proposed had the advantage that it could at
least be adopted later. It also had the advantage that it was largely
invisble except for the non-trivial cases that needed it.


> help # help: Show help
> help <STRING>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 =
>     TOKEN_STRING_INITIALIZER(struct cmd_help_result, help, "help");
> static cmdline_parse_inst_t cmd_help = {
>     .f = 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 =
>     TOKEN_STRING_INITIALIZER(struct cmd_help_section_result, help, "help");
> static cmdline_parse_token_string_t cmd_help_section_section_tok =
>     TOKEN_STRING_INITIALIZER(struct cmd_help_section_result, section, NULL);
> static cmdline_parse_inst_t cmd_help_section = {
>     .f = 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 <section> command.
> > > Here is my quick hack:
> > >
> > > diff --git a/buildtools/dpdk-cmdline-gen.py b/buildtools/dpdk-cmdline-gen.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 = t[1:].split('>')
> > > -            t_val = 'NULL'
> > > +            if len(t_type.split('(')) == 2:
> > > +                t_type, t_val = t_type.split('(')
> > > +                t_val = '"' + t_val.split(')')[0] + '"'
> > > +            else:
> > > +                t_val = 'NULL'
> > >          else:
> > >              t_type = 'STRING'
> > >              t_name = t
> > > @@ -113,7 +117,7 @@ def process_commands(infile, hfile, cfile, ctxname):
> > >              continue
> > >          if '#' not in line:
> > >              line = line + '#'  # ensure split always works, even if
> > > no help text
> > > -        tokens, comment = line.split('#', 1)
> > > +        tokens, comment = line.rsplit('#', 1)
> > >          instances.append(process_command(tokens.strip().split(),
> > > cfile, comment.strip()))
> > >
> > >      print(f'static __rte_used cmdline_parse_ctx_t {ctxname}[] = {{')
> > >
> > >
> > > Which translates as:
> > > cmd_brief:help # help: Show help
> > > help <STRING(all#control#display#config#ports)>section # help: Show help
> > >
> >
> > +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...
> 
I'm not sure it does particularly. The bit that struck me was that if you
look at the help texts for the commands that take multiple options, they
all give the options separated with "|". Therefore, it seems logical to
also use that in command specifiers too.

> 
> >
> > 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.
> 

I'm ok to drop the initial and terminating |, if that is what the issue is
- or else I'm misunderstanding things. For example:

help <all|control|display|config|ports>section


> 
> 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.
>

Good. However, it only works if we come up with a suitable separator
character to detect to split on. I don't like "#" as separator as it is
used so many places as a comment character. What about bracketed,
comma-separated lists?

help <(all,control,display,config,ports)>section

> 
> >
> > 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 <section>".
> >
> >   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 <UINT>value".
> 
Ok, let me think on it a bit....

/Bruce
  

On Tue, Oct 17, 2023 at 06:23:47PM +0200, David Marchand wrote:
> On Tue, Oct 17, 2023 at 10:29 AM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> > >
> > > - testpmd accepts both "help" and "help <section>" 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 = []
> > >
> > > +    prefix, sep, cmd_name = tokens[0].partition(':')
> > > +    if cmd_name:
> > > +        tokens[0] = cmd_name
> > > +    else:
> > > +        prefix = 'cmd'
> > > +
> > >      if tokens[0].startswith('<'):
> > >          print('Error: each command must start with at least one
> > > literal string', file=sys.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 <STRING>section # help: Show help
> > >
> >
> > Interesting. I actually had no plans to even consider moving something like
> > 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
> > <section>" 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 = t[1:].split('>')
> +            name.append(t_name)
> +        else:
> +            name.append(t)
>      name = '_'.join(name)
> 
>      result_struct = []
> 
> With this, any command implementation symbol has the full chain of
> token names as a prefix which will ensure there is no conflict.
> WDYT?
> 
Having thought a little more about it, I still don't like having the full
command in all cases, but I can see it being useful for cases of
overlapping prefixes.

How about making it optional - setting a flag in the typename, or in the
parameter name to indicate that it should be included in the overall
command name. For example, if we prefix the variable name with "_" or "__",
it could indicate that we can choose to include this.

show port <UINT16>n  --> void cmd_show_port_parsed(...)
show port <UINT16>_n --> void cmd_show_port_n_parsed(...)

Prefixes on strings beyond initial tokens could just be silently stripped.
[Obviously open to other ideas on what form tagging could take]

/Bruce
  

Hello Bruce,


On Tue, Oct 17, 2023 at 7:08 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> > > 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
> > > <section>" 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 = t[1:].split('>')
> > +            name.append(t_name)
> > +        else:
> > +            name.append(t)
> >      name = '_'.join(name)
> >
> >      result_struct = []
> >
> > With this, any command implementation symbol has the full chain of
> > token names as a prefix which will ensure there is no conflict.
> > WDYT?
> >
> Having thought a little more about it, I still don't like having the full
> command in all cases, but I can see it being useful for cases of
> overlapping prefixes.
>
> How about making it optional - setting a flag in the typename, or in the
> parameter name to indicate that it should be included in the overall
> command name. For example, if we prefix the variable name with "_" or "__",
> it could indicate that we can choose to include this.
>
> show port <UINT16>n  --> void cmd_show_port_parsed(...)
> show port <UINT16>_n --> void cmd_show_port_n_parsed(...)
>

I think I get what you mean, and it seems acceptable.


> Prefixes on strings beyond initial tokens could just be silently stripped.

By initial tokens, do you mean fixed strings token before a <> typed token ?
  

On Wed, Oct 18, 2023 at 01:21:40PM +0200, David Marchand wrote:
> Hello Bruce,
> 
> 
> On Tue, Oct 17, 2023 at 7:08 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> > > > 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
> > > > <section>" 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 = t[1:].split('>')
> > > +            name.append(t_name)
> > > +        else:
> > > +            name.append(t)
> > >      name = '_'.join(name)
> > >
> > >      result_struct = []
> > >
> > > With this, any command implementation symbol has the full chain of
> > > token names as a prefix which will ensure there is no conflict.
> > > WDYT?
> > >
> > Having thought a little more about it, I still don't like having the full
> > command in all cases, but I can see it being useful for cases of
> > overlapping prefixes.
> >
> > How about making it optional - setting a flag in the typename, or in the
> > parameter name to indicate that it should be included in the overall
> > command name. For example, if we prefix the variable name with "_" or "__",
> > it could indicate that we can choose to include this.
> >
> > show port <UINT16>n  --> void cmd_show_port_parsed(...)
> > show port <UINT16>_n --> void cmd_show_port_n_parsed(...)
> >
> 
> I think I get what you mean, and it seems acceptable.
> 

Cool. Any suggestions for a preferred prefix to indicate inclusion in the
cmd name? "_", "__" or something else? I'm trending towards single "_" as
above.

> 
> > Prefixes on strings beyond initial tokens could just be silently stripped.
> 
> By initial tokens, do you mean fixed strings token before a <> typed token ?
>
Yes.

So:

add <UINT16>x             --> cmd_add_parsed
add <UINT16>_x            --> cmd_add_x_parsed
add <UINT16>_x <UINT16>_y --> cmd_add_x_y_parsed
add <UINT16>x <UINT16>_y  --> cmd_add_parsed, strip "_" off y silently

/Bruce