From: Neil Horman <nhorman@tuxdriver.com>
To: "Wiles, Keith" <keith.wiles@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [RFC PATCH] eal:Add new API for parsing args at rte_eal_init time
Date: Thu, 4 Jun 2015 09:55:42 -0400 [thread overview]
Message-ID: <20150604135542.GC24585@hmsreliant.think-freely.org> (raw)
In-Reply-To: <D195A551.21883%keith.wiles@intel.com>
On Thu, Jun 04, 2015 at 11:50:33AM +0000, Wiles, Keith wrote:
> Hi Stephen
>
> On 6/3/15, 7:12 PM, "Stephen Hemminger" <stephen@networkplumber.org> wrote:
>
> >On Wed, 3 Jun 2015 13:49:53 -0500
> >Keith Wiles <keith.wiles@intel.com> wrote:
> >
> >> +/* Launch threads, called at application init() and parse app args. */
> >> +int
> >> +rte_eal_init_parse(int argc, char **argv,
> >> + int (*parse)(int, char **))
> >> +{
> >> + int ret;
> >> +
> >> + ret = rte_eal_init(argc, argv);
> >> + if ((ret >= 0) && (parse != NULL)) {
> >> + argc -= ret;
> >> + argv += ret;
> >
> >This won't work C is call by value.
>
> I tested this routine with Pktgen (again), which has a number of
> application options and it appears to work correctly. Can you explain why
> this will not work?
>
> Regards,
> ++Keith
> >
>
>
Stephen was thinking that your intent was to have argc, and argv modified at the
call site of this function (i.e. if you called rte_eal_init_parse from main(),
then after the call to rte_ela_init_parse, argc would be reduced by ret and argv
would point forward in memory ret bytes in the main function, but they wont. It
doesn't matter though, because you return ret, so the caller can do that
movement themselves. As you note, it works.
Note that if it was your intention to have argc and argv modified at the call
site, then Stephen is right and this is broken, you need to modify the prototype
to be:
int rte_eal_init_parse(int *argc, char ***argv)
and do a dereference when modifying the parameters so the change is seen at the
call site.
That said, I'm not sure theres much value in adding this to the API. For one,
it implies that dpdk arguments need to come first on the command line. While
all the example applications do that, theres no requirement that they do so, and
this function silently implies that they have to, so any existing applications
in the wild that violate that assumption are enjoined from using this
It also doesn't really save any code. If we pick an example app (I'll us
l2fwd-jobstats), We currently have this:
/* init EAL */
ret = rte_eal_init(argc, argv);
if (ret < 0)
rte_exit(EXIT_FAILURE, "Invalid EAL arguments\n");
argc -= ret;
argv += ret;
/* parse application arguments (after the EAL ones) */
ret = l2fwd_parse_args(argc, argv);
if (ret < 0)
rte_exit(EXIT_FAILURE, "Invalid L2FWD arguments\n");
With your new API we would get this:
ret = rte_eal_init_parse(argc, argv, l2fwd_parse_args)
if (ret < 0)
rte_exit(EXIT_FAILURE, "Invalid arguments - not sure what\n");
Its definately 5 fewer lines of source, but it doesn't save any execution
instructions, and for the effort of that, you loose the ability to determine if
it was a DPDK argument or an application argument that failed.
Its not a bad addition, I'm just not sure its worth having to take on the
additional API surface to include. I'd be more supportive if you could enhance
the function to allow the previously mentioned before/after flexibiilty. Then
we could just deprecate rte_eal_init as an API call entirely, and use this
instead.
Neil
next prev parent reply other threads:[~2015-06-04 13:55 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-03 18:49 Keith Wiles
2015-06-03 19:24 ` Thomas Monjalon
2015-06-03 19:43 ` Wiles, Keith
2015-06-04 0:12 ` Stephen Hemminger
2015-06-04 11:50 ` Wiles, Keith
2015-06-04 13:55 ` Neil Horman [this message]
2015-06-04 14:27 ` Wiles, Keith
2015-06-04 14:43 ` David Marchand
2015-06-04 14:51 ` Wiles, Keith
2015-06-04 14:55 ` Wiles, Keith
2015-06-04 14:47 ` Stephen Hemminger
2015-06-04 16:51 ` Thomas F Herbert
2015-06-04 21:27 ` Chilikin, Andrey
2015-06-05 6:01 ` Simon Kågström
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=20150604135542.GC24585@hmsreliant.think-freely.org \
--to=nhorman@tuxdriver.com \
--cc=dev@dpdk.org \
--cc=keith.wiles@intel.com \
/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).