From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
Subject: Re: [PATCH v2] eal: add option --master-lcore
Date: Wed, 05 Nov 2014 17:52:46 +0100
Message-ID: <2670906.3ddkbcCbba@xps13>
References: <1404808110-16314-1-git-send-email-simon.kuenzer@neclab.eu>
 <1415137233-6364-1-git-send-email-thomas.monjalon@6wind.com>
 <2601191342CEEE43887BDE71AB977258213A2877@IRSMSX105.ger.corp.intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7Bit
Cc: dev-VfR2kkLFssw@public.gmane.org
To: "Ananyev, Konstantin" <konstantin.ananyev-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Return-path: <dev-bounces-VfR2kkLFssw@public.gmane.org>
In-Reply-To: <2601191342CEEE43887BDE71AB977258213A2877-kPTMFJFq+rEu0RiL9chJVbfspsVTdybXVpNB7YpNyf8@public.gmane.org>
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev-VfR2kkLFssw@public.gmane.org>
List-Help: <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=subscribe>
Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org
Sender: "dev" <dev-bounces-VfR2kkLFssw@public.gmane.org>

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