From: Sunil Kumar Kori <skori@marvell.com>
To: Bruce Richardson <bruce.richardson@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"david.marchand@redhat.com" <david.marchand@redhat.com>
Subject: RE: [EXT] [PATCH 1/3] buildtools/dpdk-cmdline-gen: support optional parameters
Date: Wed, 6 Dec 2023 16:19:24 +0000 [thread overview]
Message-ID: <CO6PR18MB38604DBE44AD86093DE02449B484A@CO6PR18MB3860.namprd18.prod.outlook.com> (raw)
In-Reply-To: <ZXB3YCUtFLS4-b1C@bricha3-MOBL.ger.corp.intel.com>
> -----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
next prev parent reply other threads:[~2023-12-06 16:19 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-05 14:51 [PATCH 0/3] enhancements for dpdk-cmdline-gen script Bruce Richardson
2023-12-05 14:51 ` [PATCH 1/3] buildtools/dpdk-cmdline-gen: support optional parameters Bruce Richardson
2023-12-06 7:23 ` [EXT] " Sunil Kumar Kori
2023-12-06 13:30 ` Bruce Richardson
2023-12-06 16:19 ` Sunil Kumar Kori [this message]
2023-12-05 14:51 ` [PATCH 2/3] buildtools/dpdk-cmdline-gen: fix IP address initializer Bruce Richardson
2023-12-06 7:24 ` [EXT] " Sunil Kumar Kori
2023-12-05 14:51 ` [PATCH 3/3] buildtools/dpdk-cmdline-gen: add explicit IPv4 and v6 types Bruce Richardson
2023-12-06 7:42 ` [EXT] " Sunil Kumar Kori
2023-12-12 10:25 ` [PATCH 0/3] enhancements for dpdk-cmdline-gen script David Marchand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CO6PR18MB38604DBE44AD86093DE02449B484A@CO6PR18MB3860.namprd18.prod.outlook.com \
--to=skori@marvell.com \
--cc=bruce.richardson@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).