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