[1/3] buildtools/dpdk-cmdline-gen: support optional parameters
Checks
Commit Message
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(-)
Comments
> -----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
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
> -----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
@@ -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
@@ -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
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~