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

[Sunil Kumar Kori]

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson at intel.com>
> Sent: Wednesday, December 6, 2023 7:00 PM
> To: Sunil Kumar Kori <skori at marvell.com>
> Cc: dev at dpdk.org; david.marchand at 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 at intel.com>
> > > Sent: Tuesday, December 5, 2023 8:21 PM
> > > To: dev at dpdk.org
> > > Cc: Sunil Kumar Kori <skori at marvell.com>;
> david.marchand at redhat.com;
> > > Bruce Richardson <bruce.richardson at 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 at marvell.com>

> /Bruce