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

[Bruce Richardson <bruce.richardson@intel.com>]

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