[PATCH 0/3] enhancements for dpdk-cmdline-gen script

[PATCH 0/3] enhancements for dpdk-cmdline-gen script

[Bruce Richardson]

This set contains some small enhancements for the cmdline generation
script introduced in the last release. Specifically:

* Add support for commands with an optional variable parameter. This
  is needed to support command pairs like testpmd's "start tx_first"
  and "start tx_first 128" (to send 128 packets rather than 32).

* Improve IP address handling support. We make the "IP" type correspond
  to the cmdline lib IP type which supports IPv4 and v6. Then we add
  explicit support for IPv4 addresses and IPv6 addresses only via
  new type names.


Bruce Richardson (3):
  buildtools/dpdk-cmdline-gen: support optional parameters
  buildtools/dpdk-cmdline-gen: fix IP address initializer
  buildtools/dpdk-cmdline-gen: add explicit IPv4 and v6 types

 buildtools/dpdk-cmdline-gen.py    | 21 ++++++++++++++++++++-
 doc/guides/prog_guide/cmdline.rst | 17 +++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)

--
2.40.1

[PATCH 1/3] buildtools/dpdk-cmdline-gen: support optional parameters

[Bruce Richardson]

Sometimes a user may want to have a command which takes an optional
parameter. For commands with an optional constant string, this is no
issue, as it can be configured as two separate commands, e.g.

start
start tx_first.

However, if we want to have a variable parameter on the command, we hit
issues, because variable tokens do not form part of the generated
command names used for function and result-struct naming. To avoid
duplicate name issues, we add a special syntax to allow the user to
indicate that a particular variable parameter should be included in the
name. Any leading variable names starting with a "__" will be included
in command naming.

This then allows us to have:

start
start tx_first
start tx_first <UINT16>__n

without hitting any naming conflicts.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 buildtools/dpdk-cmdline-gen.py    |  9 ++++++++-
 doc/guides/prog_guide/cmdline.rst | 13 +++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/buildtools/dpdk-cmdline-gen.py b/buildtools/dpdk-cmdline-gen.py
index bf1253d949..faee4ffca7 100755
--- a/buildtools/dpdk-cmdline-gen.py
+++ b/buildtools/dpdk-cmdline-gen.py
@@ -40,7 +40,12 @@ def process_command(lineno, tokens, comment):
     name_tokens = []
     for t in tokens:
         if t.startswith("<"):
-            break
+            # stop processing the name building at a variable token,
+            # UNLESS the token name starts with "__"
+            t_type, t_name = t[1:].split(">")
+            if not t_name.startswith("__"):
+                break
+            t = t_name[2:]   # strip off the leading '__'
         name_tokens.append(t)
     name = "_".join(name_tokens)
 
@@ -51,6 +56,8 @@ def process_command(lineno, tokens, comment):
         if t.startswith("<"):
             t_type, t_name = t[1:].split(">")
             t_val = "NULL"
+            if t_name.startswith("__"):
+                t_name = t_name[2:]
         else:
             t_type = "STRING"
             t_name = t
diff --git a/doc/guides/prog_guide/cmdline.rst b/doc/guides/prog_guide/cmdline.rst
index b804d7a328..fc32d727dc 100644
--- a/doc/guides/prog_guide/cmdline.rst
+++ b/doc/guides/prog_guide/cmdline.rst
@@ -155,6 +155,19 @@ To get at the results structure for each command above,
 the ``parsed_result`` parameter should be cast to ``struct cmd_quit_result``
 or ``struct cmd_show_port_stats_result`` respectively.
 
+.. note::
+
+  In some cases, the user may want to have an optional variable parameter at the end of a command.
+  Such a variable parameter would not normally be included in the ``<cmdname>`` string,
+  leading to duplicate definition errors.
+  To work around this,
+  any variable token with a name prefixed by ``'__'`` will be included in the cmdname string,
+  with the prefix removed.
+  Using this, it is possible to have commands, such as:
+  ``start tx_first`` and ``start tx_first <UINT16>__n``, without them conflicting.
+  The resulting code generated will expect functions called ``cmd_start_tx_first_parsed``
+  and ``cmd_start_tx_first_n_parsed`` respectively.
+
 Integrating with the Application
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-- 
2.40.1

[PATCH 2/3] buildtools/dpdk-cmdline-gen: fix IP address initializer

[Bruce Richardson]

The IP address type should be generic for both IPv4 and IPv6 and so use
the cmdline lib's TOKEN_IPADDR_INITIALIZER rather than
TOKEN_IPV4_INITIALIZER.

Fixes: 37666691e9ed ("buildtools: add a tool to generate cmdline boilerplate")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 buildtools/dpdk-cmdline-gen.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/buildtools/dpdk-cmdline-gen.py b/buildtools/dpdk-cmdline-gen.py
index faee4ffca7..8b4f22ca24 100755
--- a/buildtools/dpdk-cmdline-gen.py
+++ b/buildtools/dpdk-cmdline-gen.py
@@ -79,7 +79,7 @@ def process_command(lineno, tokens, comment):
             result_struct.append(f"\tcmdline_ipaddr_t {t_name};")
             initializers.append(
                 f"static cmdline_parse_token_ipaddr_t cmd_{name}_{t_name}_tok =\n"
-                f"\tTOKEN_IPV4_INITIALIZER(struct cmd_{name}_result, {t_name});"
+                f"\tTOKEN_IPADDR_INITIALIZER(struct cmd_{name}_result, {t_name});"
             )
         elif t_type.startswith("(") and t_type.endswith(")"):
             result_struct.append(f"\tcmdline_fixed_string_t {t_name};")
-- 
2.40.1

[PATCH 3/3] buildtools/dpdk-cmdline-gen: add explicit IPv4 and v6 types

[Bruce Richardson]

Add support for generating cmdline lib code to just match IPv4 addresses
or IPv6 addresses, rather than IP addresses in general.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 buildtools/dpdk-cmdline-gen.py    | 12 ++++++++++++
 doc/guides/prog_guide/cmdline.rst |  4 ++++
 2 files changed, 16 insertions(+)

diff --git a/buildtools/dpdk-cmdline-gen.py b/buildtools/dpdk-cmdline-gen.py
index 8b4f22ca24..7dadded783 100755
--- a/buildtools/dpdk-cmdline-gen.py
+++ b/buildtools/dpdk-cmdline-gen.py
@@ -81,6 +81,18 @@ def process_command(lineno, tokens, comment):
                 f"static cmdline_parse_token_ipaddr_t cmd_{name}_{t_name}_tok =\n"
                 f"\tTOKEN_IPADDR_INITIALIZER(struct cmd_{name}_result, {t_name});"
             )
+        elif t_type in ["IPV4", "IPv4", "IPV4_ADDR"]:
+            result_struct.append(f"\tcmdline_ipaddr_t {t_name};")
+            initializers.append(
+                f"static cmdline_parse_token_ipaddr_t cmd_{name}_{t_name}_tok =\n"
+                f"\tTOKEN_IPV4_INITIALIZER(struct cmd_{name}_result, {t_name});"
+            )
+        elif t_type in ["IPV6", "IPv6", "IPV6_ADDR"]:
+            result_struct.append(f"\tcmdline_ipaddr_t {t_name};")
+            initializers.append(
+                f"static cmdline_parse_token_ipaddr_t cmd_{name}_{t_name}_tok =\n"
+                f"\tTOKEN_IPV6_INITIALIZER(struct cmd_{name}_result, {t_name});"
+            )
         elif t_type.startswith("(") and t_type.endswith(")"):
             result_struct.append(f"\tcmdline_fixed_string_t {t_name};")
             t_val = f'"{t_type[1:-1].replace(",","#")}"'
diff --git a/doc/guides/prog_guide/cmdline.rst b/doc/guides/prog_guide/cmdline.rst
index fc32d727dc..f62f17f1aa 100644
--- a/doc/guides/prog_guide/cmdline.rst
+++ b/doc/guides/prog_guide/cmdline.rst
@@ -70,6 +70,10 @@ The format of the list file must be:
 
   * ``<IP>src_ip``
 
+  * ``<IPv4>dst_ip4``
+
+  * ``<IPv6>dst_ip6``
+
 * Variable fields, which take their values from a list of options,
   have the comma-separated option list placed in braces, rather than a the type name.
   For example,
-- 
2.40.1

RE: [EXT] [PATCH 1/3] buildtools/dpdk-cmdline-gen: support optional parameters

[Sunil Kumar Kori]

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Tuesday, December 5, 2023 8:21 PM
> To: dev@dpdk.org
> Cc: Sunil Kumar Kori <skori@marvell.com>; david.marchand@redhat.com;
> Bruce Richardson <bruce.richardson@intel.com>
> Subject: [EXT] [PATCH 1/3] buildtools/dpdk-cmdline-gen: support optional
> parameters
> 
> External Email
> 
> ----------------------------------------------------------------------
> Sometimes a user may want to have a command which takes an optional
> parameter. For commands with an optional constant string, this is no issue,
> as it can be configured as two separate commands, e.g.
> 
> start
> start tx_first.
> 
> However, if we want to have a variable parameter on the command, we hit
> issues, because variable tokens do not form part of the generated
> command names used for function and result-struct naming. To avoid
> duplicate name issues, we add a special syntax to allow the user to
> indicate that a particular variable parameter should be included in the
> name. Any leading variable names starting with a "__" will be included in
> command naming.
> 
> This then allows us to have:
> 
> start
> start tx_first
> start tx_first <UINT16>__n
> 
> without hitting any naming conflicts.
> 
It's a good option provided to user to choose name as per the need. I have another thought that how about providing flexibility to skip a token too along with implemented support.
Consider following example:
	1. ethdev <STRING>dev mtu  <UINT16>size
	2. ethdev <STRING>dev promiscuous <(on,off)>enable

So tokens and functions should be as cmd_ethdev_mtu_parsed() and cmd_ethdev_ promiscuous_parsed().
Here token <STRING>dev  should be skipped. If it make sense, then please add this support too. 

> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  buildtools/dpdk-cmdline-gen.py    |  9 ++++++++-
>  doc/guides/prog_guide/cmdline.rst | 13 +++++++++++++
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> --
> 2.40.1

RE: [EXT] [PATCH 2/3] buildtools/dpdk-cmdline-gen: fix IP address initializer

[Sunil Kumar Kori]

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Tuesday, December 5, 2023 8:21 PM
> To: dev@dpdk.org
> Cc: Sunil Kumar Kori <skori@marvell.com>; david.marchand@redhat.com;
> Bruce Richardson <bruce.richardson@intel.com>; stable@dpdk.org
> Subject: [EXT] [PATCH 2/3] buildtools/dpdk-cmdline-gen: fix IP address
> initializer
> 
> External Email
> 
> ----------------------------------------------------------------------
> The IP address type should be generic for both IPv4 and IPv6 and so use the
> cmdline lib's TOKEN_IPADDR_INITIALIZER rather than
> TOKEN_IPV4_INITIALIZER.
> 
> Fixes: 37666691e9ed ("buildtools: add a tool to generate cmdline
> boilerplate")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  buildtools/dpdk-cmdline-gen.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/buildtools/dpdk-cmdline-gen.py b/buildtools/dpdk-cmdline-
> gen.py index faee4ffca7..8b4f22ca24 100755
> --- a/buildtools/dpdk-cmdline-gen.py
> +++ b/buildtools/dpdk-cmdline-gen.py
> @@ -79,7 +79,7 @@ def process_command(lineno, tokens, comment):
>              result_struct.append(f"\tcmdline_ipaddr_t {t_name};")
>              initializers.append(
>                  f"static cmdline_parse_token_ipaddr_t cmd_{name}_{t_name}_tok
> =\n"
> -                f"\tTOKEN_IPV4_INITIALIZER(struct cmd_{name}_result,
> {t_name});"
> +                f"\tTOKEN_IPADDR_INITIALIZER(struct cmd_{name}_result,
> {t_name});"
>              )
>          elif t_type.startswith("(") and t_type.endswith(")"):
>              result_struct.append(f"\tcmdline_fixed_string_t {t_name};")

Acked-by: Sunil Kumar Kori <skori@marvell.com>
> --
> 2.40.1

RE: [EXT] [PATCH 3/3] buildtools/dpdk-cmdline-gen: add explicit IPv4 and v6 types

[Sunil Kumar Kori]

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Tuesday, December 5, 2023 8:21 PM
> To: dev@dpdk.org
> Cc: Sunil Kumar Kori <skori@marvell.com>; david.marchand@redhat.com;
> Bruce Richardson <bruce.richardson@intel.com>
> Subject: [EXT] [PATCH 3/3] buildtools/dpdk-cmdline-gen: add explicit IPv4
> and v6 types
> 
> External Email
> 
> ----------------------------------------------------------------------
> Add support for generating cmdline lib code to just match IPv4 addresses
> or IPv6 addresses, rather than IP addresses in general.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  buildtools/dpdk-cmdline-gen.py    | 12 ++++++++++++
>  doc/guides/prog_guide/cmdline.rst |  4 ++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/buildtools/dpdk-cmdline-gen.py b/buildtools/dpdk-cmdline-
> gen.py index 8b4f22ca24..7dadded783 100755
> --- a/buildtools/dpdk-cmdline-gen.py
> +++ b/buildtools/dpdk-cmdline-gen.py
> @@ -81,6 +81,18 @@ def process_command(lineno, tokens, comment):
>                  f"static cmdline_parse_token_ipaddr_t cmd_{name}_{t_name}_tok
> =\n"
>                  f"\tTOKEN_IPADDR_INITIALIZER(struct cmd_{name}_result,
> {t_name});"
>              )
> +        elif t_type in ["IPV4", "IPv4", "IPV4_ADDR"]:
> +            result_struct.append(f"\tcmdline_ipaddr_t {t_name};")
> +            initializers.append(
> +                f"static cmdline_parse_token_ipaddr_t
> cmd_{name}_{t_name}_tok =\n"
> +                f"\tTOKEN_IPV4_INITIALIZER(struct cmd_{name}_result,
> {t_name});"
> +            )
> +        elif t_type in ["IPV6", "IPv6", "IPV6_ADDR"]:
> +            result_struct.append(f"\tcmdline_ipaddr_t {t_name};")
> +            initializers.append(
> +                f"static cmdline_parse_token_ipaddr_t
> cmd_{name}_{t_name}_tok =\n"
> +                f"\tTOKEN_IPV6_INITIALIZER(struct cmd_{name}_result,
> {t_name});"
> +            )
>          elif t_type.startswith("(") and t_type.endswith(")"):
>              result_struct.append(f"\tcmdline_fixed_string_t {t_name};")
>              t_val = f'"{t_type[1:-1].replace(",","#")}"'
> diff --git a/doc/guides/prog_guide/cmdline.rst
> b/doc/guides/prog_guide/cmdline.rst
> index fc32d727dc..f62f17f1aa 100644
> --- a/doc/guides/prog_guide/cmdline.rst
> +++ b/doc/guides/prog_guide/cmdline.rst
> @@ -70,6 +70,10 @@ The format of the list file must be:
> 
>    * ``<IP>src_ip``
> 
> +  * ``<IPv4>dst_ip4``
> +
> +  * ``<IPv6>dst_ip6``
> +
>  * Variable fields, which take their values from a list of options,
>    have the comma-separated option list placed in braces, rather than a the
> type name.
>    For example,

Acked-by: Sunil Kumar Kori <skori@marvell.com>
> --
> 2.40.1

Re: [EXT] [PATCH 1/3] buildtools/dpdk-cmdline-gen: support optional parameters

[Bruce Richardson]

On Wed, Dec 06, 2023 at 07:23:51AM +0000, Sunil Kumar Kori wrote:
> > -----Original Message-----
> > From: Bruce Richardson <bruce.richardson@intel.com>
> > Sent: Tuesday, December 5, 2023 8:21 PM
> > To: dev@dpdk.org
> > Cc: Sunil Kumar Kori <skori@marvell.com>; david.marchand@redhat.com;
> > Bruce Richardson <bruce.richardson@intel.com>
> > Subject: [EXT] [PATCH 1/3] buildtools/dpdk-cmdline-gen: support optional
> > parameters
> > 
> > External Email
> > 
> > ----------------------------------------------------------------------
> > Sometimes a user may want to have a command which takes an optional
> > parameter. For commands with an optional constant string, this is no issue,
> > as it can be configured as two separate commands, e.g.
> > 
> > start
> > start tx_first.
> > 
> > However, if we want to have a variable parameter on the command, we hit
> > issues, because variable tokens do not form part of the generated
> > command names used for function and result-struct naming. To avoid
> > duplicate name issues, we add a special syntax to allow the user to
> > indicate that a particular variable parameter should be included in the
> > name. Any leading variable names starting with a "__" will be included in
> > command naming.
> > 
> > This then allows us to have:
> > 
> > start
> > start tx_first
> > start tx_first <UINT16>__n
> > 
> > without hitting any naming conflicts.
> > 
> It's a good option provided to user to choose name as per the need. I have another thought that how about providing flexibility to skip a token too along with implemented support.
> Consider following example:
> 	1. ethdev <STRING>dev mtu  <UINT16>size
> 	2. ethdev <STRING>dev promiscuous <(on,off)>enable
> 
> So tokens and functions should be as cmd_ethdev_mtu_parsed() and cmd_ethdev_ promiscuous_parsed().
> Here token <STRING>dev  should be skipped. If it make sense, then please add this support too. 
> 
It does and at the same time it doesn't. :-)

I understand how what you suggest would indeed lead to better function
names. However, given that these are functions for cmdline, and unlikely to
be ever called from regular code, how much does the function name matter,
so long as it doesn't conflict with anything else? It was the avoiding name
conflicts, more so than having better names, was the driving force for this
patch.

In the case you describe, if we take this patch, we can change "dev" to
"__dev" to give you the functions: cmd_ethdev_dev_mtu_parsed() and
cmd_ethdev_dev_promiscuous_parsed(). The extra "dev" is a little untidy,
but then again if app isn't ever calling this directly, does it really
matter?

If we do want to have precise control over what the functions are called,
rather than adding in lots of different tweak rules, I would instead look
to see if there is some syntax we could use to specify directly the name to
be used for the function and result structs. However, right now, I'd rather
not implement this, as I'm not sure how to do so in a clean way.

/Bruce

RE: [EXT] [PATCH 1/3] buildtools/dpdk-cmdline-gen: support optional parameters

[Sunil Kumar Kori]

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Wednesday, December 6, 2023 7:00 PM
> To: Sunil Kumar Kori <skori@marvell.com>
> Cc: dev@dpdk.org; david.marchand@redhat.com
> Subject: Re: [EXT] [PATCH 1/3] buildtools/dpdk-cmdline-gen: support
> optional parameters
> 
> On Wed, Dec 06, 2023 at 07:23:51AM +0000, Sunil Kumar Kori wrote:
> > > -----Original Message-----
> > > From: Bruce Richardson <bruce.richardson@intel.com>
> > > Sent: Tuesday, December 5, 2023 8:21 PM
> > > To: dev@dpdk.org
> > > Cc: Sunil Kumar Kori <skori@marvell.com>;
> david.marchand@redhat.com;
> > > Bruce Richardson <bruce.richardson@intel.com>
> > > Subject: [EXT] [PATCH 1/3] buildtools/dpdk-cmdline-gen: support
> > > optional parameters
> > >
> > > External Email
> > >
> > > --------------------------------------------------------------------
> > > -- Sometimes a user may want to have a command which takes an
> > > optional parameter. For commands with an optional constant string,
> > > this is no issue, as it can be configured as two separate commands,
> > > e.g.
> > >
> > > start
> > > start tx_first.
> > >
> > > However, if we want to have a variable parameter on the command, we
> > > hit issues, because variable tokens do not form part of the
> > > generated command names used for function and result-struct naming.
> > > To avoid duplicate name issues, we add a special syntax to allow the
> > > user to indicate that a particular variable parameter should be
> > > included in the name. Any leading variable names starting with a
> > > "__" will be included in command naming.
> > >
> > > This then allows us to have:
> > >
> > > start
> > > start tx_first
> > > start tx_first <UINT16>__n
> > >
> > > without hitting any naming conflicts.
> > >
> > It's a good option provided to user to choose name as per the need. I have
> another thought that how about providing flexibility to skip a token too along
> with implemented support.
> > Consider following example:
> > 	1. ethdev <STRING>dev mtu  <UINT16>size
> > 	2. ethdev <STRING>dev promiscuous <(on,off)>enable
> >
> > So tokens and functions should be as cmd_ethdev_mtu_parsed() and
> cmd_ethdev_ promiscuous_parsed().
> > Here token <STRING>dev  should be skipped. If it make sense, then
> please add this support too.
> >
> It does and at the same time it doesn't. :-)
> 
> I understand how what you suggest would indeed lead to better function
> names. However, given that these are functions for cmdline, and unlikely to
> be ever called from regular code, how much does the function name
> matter, so long as it doesn't conflict with anything else? It was the avoiding
> name conflicts, more so than having better names, was the driving force for
> this patch.
> 
> In the case you describe, if we take this patch, we can change "dev" to
> "__dev" to give you the functions: cmd_ethdev_dev_mtu_parsed() and
> cmd_ethdev_dev_promiscuous_parsed(). The extra "dev" is a little untidy,
> but then again if app isn't ever calling this directly, does it really matter?
> 
> If we do want to have precise control over what the functions are called,
> rather than adding in lots of different tweak rules, I would instead look to
> see if there is some syntax we could use to specify directly the name to be
> used for the function and result structs. However, right now, I'd rather not
> implement this, as I'm not sure how to do so in a clean way.
> 

Agreed that functions are not going to be called from application directly.
I mentioned this requirement as in name cmd_ethdev_dev_promiscuous_parsed()
"ethdev" is enough to describe it's role. Unnecessarily "dev" is added.
In future, If you get time to look into and it improves something then please have a look.

Rest looks okay. So giving ack. 
Acked-by: Sunil Kumar Kori <skori@marvell.com>

> /Bruce

Re: [PATCH 0/3] enhancements for dpdk-cmdline-gen script

[David Marchand]

On Tue, Dec 5, 2023 at 3:51 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> This set contains some small enhancements for the cmdline generation
> script introduced in the last release. Specifically:
>
> * Add support for commands with an optional variable parameter. This
>   is needed to support command pairs like testpmd's "start tx_first"
>   and "start tx_first 128" (to send 128 packets rather than 32).
>
> * Improve IP address handling support. We make the "IP" type correspond
>   to the cmdline lib IP type which supports IPv4 and v6. Then we add
>   explicit support for IPv4 addresses and IPv6 addresses only via
>   new type names.
>
>
> Bruce Richardson (3):
>   buildtools/dpdk-cmdline-gen: support optional parameters
>   buildtools/dpdk-cmdline-gen: fix IP address initializer
>   buildtools/dpdk-cmdline-gen: add explicit IPv4 and v6 types
>
>  buildtools/dpdk-cmdline-gen.py    | 21 ++++++++++++++++++++-
>  doc/guides/prog_guide/cmdline.rst | 17 +++++++++++++++++
>  2 files changed, 37 insertions(+), 1 deletion(-)

Series applied, thanks Bruce.


-- 
David Marchand