Re: [PATCH 3/3] app/testpmd: changed example to parse options from cfg file

["Kozak, KubaX" <kubax.kozak@intel.com>]

Comments inline.

Best regards,
Kuba Kozak

> -----Original Message-----
> From: Wu, Jingjing
> Sent: Tuesday, June 20, 2017 04:14
> To: Piasecki, JacekX <jacekx.piasecki@intel.com>; dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richardson@intel.com>; Jain, Deepak K <deepak.k.jain@intel.com>;
> Jastrzebski, MichalX K <michalx.k.jastrzebski@intel.com>; Kozak, KubaX <kubax.kozak@intel.com>
> Subject: RE: [dpdk-dev] [PATCH 3/3] app/testpmd: changed example to parse options from cfg file
> 
> > +
> > +#ifdef RTE_LIBRTE_CFGFILE
> > +/* Load config file path from command line */ static char *
> > +cfgfile_load_path(int argc, char **argv) {
> > +	int i;
> > +
> > +	for (i = 0; i < argc; i++) {
> > +		if (!strcmp("--cfgfile-path", argv[i])) {
> > +			if (i < argc)
> > +				return strdup(argv[i+1]);
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +#endif
> > +
> It is a little confused. Is the cfgfile-path is application's argument or EAL's?
> Where is it supposed to be located?

It is an application argument.

> 
> >  int
> >  main(int argc, char** argv)
> >  {
> >  	int  diag;
> >  	uint8_t port_id;
> > +	struct rte_cfgfile *cfg = NULL;
> > +	char *config_file = NULL;
> >
> >  	signal(SIGINT, signal_handler);
> >  	signal(SIGTERM, signal_handler);
> >
> > +#ifdef RTE_LIBRTE_CFGFILE
> > +	/* load --cfgfile-path argument from argv */
> > +	config_file = cfgfile_load_path(argc, argv);
> > +
> > +	if (config_file) {
> > +		printf("Info: found cfgfile-path \"%s\"\n", config_file);
> > +	} else {
> > +		printf("Info: not found cfgfile-path parameter "
> > +				"(searching for cfgfile "
> > +				"in default directory)\n");
> > +		config_file = strdup("config.ini");
> > +	}
> > +	cfg = rte_cfgfile_load(config_file, CFG_FLAG_EMPTY_VALUES);
> > +	if (cfg == NULL)
> > +		printf("Info: Valid cfgfile not found\n");
> > +	rte_eal_configure(cfg);
> > +#endif
> > +
> Does it mean if RTE_LIBRTE_CFGFILE is defined, then the arguments are passed by Cfgfile, if no cfgfile
> will use config.ini by default? How about the legacy command lines?
> I think even cfgfile is good, we still need to keep the command line way.

If RTE_LIBRTE_CFGFILE is defined then arguments are passed both by cfgfile and command line. Command line arguments have priority over cfgfile arguments and they override their values.

> 
> >  	diag = rte_eal_init(argc, argv);
> >  	if (diag < 0)
> >  		rte_panic("Cannot init EAL\n");
> > @@ -2289,7 +2329,16 @@ uint8_t port_is_bonding_slave(portid_t slave_pid)
> >  	argc -= diag;
> >  	argv += diag;
> >  	if (argc > 1)
> > -		launch_args_parse(argc, argv);
> > +		launch_args_parse(argc, argv, cfg);
> > +
> 
> The argc and argv have been overwritten by cfgfile, right? Does it make sense to Pass cfg to
> launch_args_parse?

This function translate argv arguments into configuration file form and append/override current cfgfile.
Finally it parse application arguments from cfgfile. 
Intention was to parse arguments from single source (configuration file form).

> 
> 
> And you also need to update the testpmd doc to describe this new use.
> 
> Thanks
> Jingjing