Re: [dpdk-dev] [PATCH v2] eal: add option --master-lcore

[Thomas Monjalon <thomas.monjalon@6wind.com>]

2014-11-05 11:54, Ananyev, Konstantin:
> From: Thomas Monjalon
> > +	long master_lcore;
> > +	char *parsing_end;
> > +	struct rte_config *cfg = rte_eal_get_configuration();
> > +
> > +	errno = 0;
> > +	master_lcore = strtol(arg, &parsing_end, 0);
> > +	if (errno || parsing_end == arg)
> > +		return -1;
> 
> Why not: "errno || parsing_end[0] != 0"
> ?
> Otherwise something like "1blah" would be considered as valid input.

Good point.

> > +	if (!(master_lcore >= 0 && master_lcore < RTE_MAX_LCORE))
> > +		return -1;
> 
> If negative values are not allowed, then why not:
> 
> unsigned long master_lcore;
> ...
> master_lcore = strtoul(...)
> ...
> if(master_clore > RTE_MAX_LCORE)
>     return -1;   

Matter of taste. Your code is less explicit.
But it should be
	if(master_clore >= RTE_MAX_LCORE)
Anyone else to vote for 1 solution or the other?

> > +		if (opt == OPT_MASTER_LCORE_NUM && !coremask_ok) {
> > +			RTE_LOG(ERR, EAL, "please specify the master lcore id"
> > +				"after specifying the coremask\n");
> > +			eal_usage(prgname);
> > +			return -1;
> > +		}
> > +
> 
> I don't really like an idea of introducing strict order between -c and  "--master-lcore..

Me too. And Aaron too :)

> Can we move check for coremask_ok/ and assignment of cfg->master_lcore out of  
> while (getopt_long(...)) loop?
> 
> >  		ret = eal_parse_common_option(opt, optarg, &internal_config);
> >  		/* common parser is not happy */
> >  		if (ret < 0) {

Yes we should move the check outside of the loop.
First we should migrate all flags check in a common function for BSD and Linux.

Simon made the v1. I made the v2. Any volunteer for the v3?

-- 
Thomas