[dpdk-dev] [RFC PATCH] eal:Add new API for parsing args at rte_eal_init time

Neil Horman nhorman at tuxdriver.com
Thu Jun 4 15:55:42 CEST 2015


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 at networkplumber.org> wrote:
> 
> >On Wed,  3 Jun 2015 13:49:53 -0500
> >Keith Wiles <keith.wiles at 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



More information about the dev mailing list