DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] Found a bug related to getopt() in eal/bsd module
@ 2015-10-13  8:54 Tiwei Bie
  2015-10-13  8:54 ` [dpdk-dev] [PATCH] eal/bsd: reinitialize optind and optreset to 1 Tiwei Bie
  0 siblings, 1 reply; 9+ messages in thread
From: Tiwei Bie @ 2015-10-13  8:54 UTC (permalink / raw)
  To: dev

I found a bug when trying to make my DPDK application work on FreeBSD.
The variable optind must be reinitialized to 1 on FreeBSD to skip over
argv[0]. Because getopt() on FreeBSD will return -1 when it meets an
argument which doesn't start with '-'. This behaviour is implemented
by the 13-17 lines:

01 /*
02  * getopt --
03  *	Parse argc/argv argument vector.
04  */
05 int
06 getopt(int nargc, char * const nargv[], const char *ostr)
07 {
08 	static char *place = EMSG;		/* option letter processing */
09 	char *oli;				/* option letter list index */
10
11 	if (optreset || *place == 0) {		/* update scanning pointer */
12 		optreset = 0;
13 		place = nargv[optind];
14 		if (optind >= nargc || *place++ != '-') {
15 			/* Argument is absent or is not an option */
16 			place = EMSG;
17 			return (-1);
18 		}
19 		......
20 	}
21 	......
22 }

The variable optreset is also provided on FreeBSD to indicate the
additional set of calls to getopt(). So, also reinitialize it to 1.

References:

1. https://svnweb.freebsd.org/base/head/lib/libc/stdlib/getopt.c?view=markup#l70
2. https://www.freebsd.org/cgi/man.cgi?query=getopt&apropos=0&sektion=3&manpath=FreeBSD+11-current&arch=default&format=html

Tiwei Bie (1):
  eal/bsd: reinitialize optind and optreset to 1

 lib/librte_eal/bsdapp/eal/eal.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

-- 
2.6.0

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [dpdk-dev] [PATCH] eal/bsd: reinitialize optind and optreset to 1
  2015-10-13  8:54 [dpdk-dev] [PATCH] Found a bug related to getopt() in eal/bsd module Tiwei Bie
@ 2015-10-13  8:54 ` Tiwei Bie
  2015-10-13 14:58   ` Bruce Richardson
  2015-10-13 17:14   ` Don Provan
  0 siblings, 2 replies; 9+ messages in thread
From: Tiwei Bie @ 2015-10-13  8:54 UTC (permalink / raw)
  To: dev

The variable optind must be reinitialized to 1 in order to skip over
argv[0] on FreeBSD. Because getopt() on FreeBSD will return -1 when
it meets an argument which doesn't start with '-'.

The variable optreset is provided on FreeBSD to indicate the additional
set of calls to getopt(). So, also reinitialize it to 1.

Signed-off-by: Tiwei Bie <btw@mail.ustc.edu.cn>
---
 lib/librte_eal/bsdapp/eal/eal.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index 1b6f705..35feaee 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -334,7 +334,8 @@ eal_log_level_parse(int argc, char **argv)
 			break;
 	}
 
-	optind = 0; /* reset getopt lib */
+	optind = 1; /* reset getopt lib */
+	optreset = 1;
 }
 
 /* Parse the argument given in the command line of the application */
@@ -403,7 +404,8 @@ eal_parse_args(int argc, char **argv)
 	if (optind >= 0)
 		argv[optind-1] = prgname;
 	ret = optind-1;
-	optind = 0; /* reset getopt lib */
+	optind = 1; /* reset getopt lib */
+	optreset = 1;
 	return ret;
 }
 
-- 
2.6.0

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] eal/bsd: reinitialize optind and optreset to 1
  2015-10-13  8:54 ` [dpdk-dev] [PATCH] eal/bsd: reinitialize optind and optreset to 1 Tiwei Bie
@ 2015-10-13 14:58   ` Bruce Richardson
  2015-10-13 17:14   ` Don Provan
  1 sibling, 0 replies; 9+ messages in thread
From: Bruce Richardson @ 2015-10-13 14:58 UTC (permalink / raw)
  To: Tiwei Bie; +Cc: dev

On Tue, Oct 13, 2015 at 04:54:06PM +0800, Tiwei Bie wrote:
> The variable optind must be reinitialized to 1 in order to skip over
> argv[0] on FreeBSD. Because getopt() on FreeBSD will return -1 when
> it meets an argument which doesn't start with '-'.
> 
> The variable optreset is provided on FreeBSD to indicate the additional
> set of calls to getopt(). So, also reinitialize it to 1.
> 
> Signed-off-by: Tiwei Bie <btw@mail.ustc.edu.cn>

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] eal/bsd: reinitialize optind and optreset to 1
  2015-10-13  8:54 ` [dpdk-dev] [PATCH] eal/bsd: reinitialize optind and optreset to 1 Tiwei Bie
  2015-10-13 14:58   ` Bruce Richardson
@ 2015-10-13 17:14   ` Don Provan
       [not found]     ` <20151014022843.GA26774@dell>
  1 sibling, 1 reply; 9+ messages in thread
From: Don Provan @ 2015-10-13 17:14 UTC (permalink / raw)
  To: Tiwei Bie, dev

Actually, this is a good opportunity to fix a bug that's been in this code forever: it shouldn't be resetting optind to some arbitrary value: it should be saving optind (and optarg and optopt) at the beginning, initializing optind to 1 before calling getopt_long(), then restoring all the values after. (And, from what you're saying, optreset should be handled the same as optind.)

This avoids broken behavior if rte_eal_init() is called by code that's in the middle of using getopt() to parse its own unrelated argc/argv parameters. 

-don provan
dprovan@bivio.net

-----Original Message-----
From: Tiwei Bie [mailto:btw@mail.ustc.edu.cn] 
Sent: Tuesday, October 13, 2015 1:54 AM
To: dev@dpdk.org
Subject: [dpdk-dev] [PATCH] eal/bsd: reinitialize optind and optreset to 1

The variable optind must be reinitialized to 1 in order to skip over argv[0] on FreeBSD. Because getopt() on FreeBSD will return -1 when it meets an argument which doesn't start with '-'.

The variable optreset is provided on FreeBSD to indicate the additional set of calls to getopt(). So, also reinitialize it to 1.

Signed-off-by: Tiwei Bie <btw@mail.ustc.edu.cn>
---
 lib/librte_eal/bsdapp/eal/eal.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c index 1b6f705..35feaee 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -334,7 +334,8 @@ eal_log_level_parse(int argc, char **argv)
 			break;
 	}
 
-	optind = 0; /* reset getopt lib */
+	optind = 1; /* reset getopt lib */
+	optreset = 1;
 }
 
 /* Parse the argument given in the command line of the application */ @@ -403,7 +404,8 @@ eal_parse_args(int argc, char **argv)
 	if (optind >= 0)
 		argv[optind-1] = prgname;
 	ret = optind-1;
-	optind = 0; /* reset getopt lib */
+	optind = 1; /* reset getopt lib */
+	optreset = 1;
 	return ret;
 }
 
--
2.6.0

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] eal/bsd: reinitialize optind and optreset to 1
       [not found]     ` <20151014022843.GA26774@dell>
@ 2015-10-14  9:31       ` Bruce Richardson
  2015-10-14 10:19         ` Tiwei Bie
  0 siblings, 1 reply; 9+ messages in thread
From: Bruce Richardson @ 2015-10-14  9:31 UTC (permalink / raw)
  To: Tiwei Bie; +Cc: dev, Don Provan

On Wed, Oct 14, 2015 at 10:28:44AM +0800, Tiwei Bie wrote:
> On Tue, Oct 13, 2015 at 05:14:38PM +0000, Don Provan wrote:
> > Actually, this is a good opportunity to fix a bug that's been in this code forever: it shouldn't be resetting optind to some arbitrary value: it should be saving optind (and optarg and optopt) at the beginning, initializing optind to 1 before calling getopt_long(), then restoring all the values after. (And, from what you're saying, optreset should be handled the same as optind.)
> > 
> 
> It is designed to have DPDK's parameters specified in the front of the
> cmd line and terminated by '--'. Or at least, you should put DPDK's
> parameters together and terminate them by '--'. And 1 or 0 are not some
> arbitrary values. They are used to put the index back to the beginning
> of the new argv[] array.
> 
> > This avoids broken behavior if rte_eal_init() is called by code that's in the middle of using getopt() to parse its own unrelated argc/argv parameters. 
> > 
> 
> We shouldn't mix up DPDK's parameters and application's parameters.
> And we should group them using '--'.
> 
> Best,
> Tiwei Bie

While true, that does not prevent us from implementing Don's suggestion, as it
should fix the bug you are looking at with your original patch, and also allow
additional use-cases for applications at no extra cost.

/Bruce

> 
> > -don provan
> > dprovan@bivio.net
> > 
> > -----Original Message-----
> > From: Tiwei Bie [mailto:btw@mail.ustc.edu.cn] 
> > Sent: Tuesday, October 13, 2015 1:54 AM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH] eal/bsd: reinitialize optind and optreset to 1
> > 
> > The variable optind must be reinitialized to 1 in order to skip over argv[0] on FreeBSD. Because getopt() on FreeBSD will return -1 when it meets an argument which doesn't start with '-'.
> > 
> > The variable optreset is provided on FreeBSD to indicate the additional set of calls to getopt(). So, also reinitialize it to 1.
> > 
> > Signed-off-by: Tiwei Bie <btw@mail.ustc.edu.cn>
> > ---
> >  lib/librte_eal/bsdapp/eal/eal.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c index 1b6f705..35feaee 100644
> > --- a/lib/librte_eal/bsdapp/eal/eal.c
> > +++ b/lib/librte_eal/bsdapp/eal/eal.c
> > @@ -334,7 +334,8 @@ eal_log_level_parse(int argc, char **argv)
> >  			break;
> >  	}
> >  
> > -	optind = 0; /* reset getopt lib */
> > +	optind = 1; /* reset getopt lib */
> > +	optreset = 1;
> >  }
> >  
> >  /* Parse the argument given in the command line of the application */ @@ -403,7 +404,8 @@ eal_parse_args(int argc, char **argv)
> >  	if (optind >= 0)
> >  		argv[optind-1] = prgname;
> >  	ret = optind-1;
> > -	optind = 0; /* reset getopt lib */
> > +	optind = 1; /* reset getopt lib */
> > +	optreset = 1;
> >  	return ret;
> >  }
> >  
> > --
> > 2.6.0
> > 
> > 
> > 
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] eal/bsd: reinitialize optind and optreset to 1
  2015-10-14  9:31       ` Bruce Richardson
@ 2015-10-14 10:19         ` Tiwei Bie
  2015-10-14 11:28           ` Tiwei Bie
  0 siblings, 1 reply; 9+ messages in thread
From: Tiwei Bie @ 2015-10-14 10:19 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev, Don Provan

On Wed, Oct 14, 2015 at 10:31:28AM +0100, Bruce Richardson wrote:
> On Wed, Oct 14, 2015 at 10:28:44AM +0800, Tiwei Bie wrote:
> > On Tue, Oct 13, 2015 at 05:14:38PM +0000, Don Provan wrote:
> > > Actually, this is a good opportunity to fix a bug that's been in this code forever: it shouldn't be resetting optind to some arbitrary value: it should be saving optind (and optarg and optopt) at the beginning, initializing optind to 1 before calling getopt_long(), then restoring all the values after. (And, from what you're saying, optreset should be handled the same as optind.)
> > > 
> > 
> > It is designed to have DPDK's parameters specified in the front of the
> > cmd line and terminated by '--'. Or at least, you should put DPDK's
> > parameters together and terminate them by '--'. And 1 or 0 are not some
> > arbitrary values. They are used to put the index back to the beginning
> > of the new argv[] array.
> > 
> > > This avoids broken behavior if rte_eal_init() is called by code that's in the middle of using getopt() to parse its own unrelated argc/argv parameters. 
> > > 
> > 
> > We shouldn't mix up DPDK's parameters and application's parameters.
> > And we should group them using '--'.
> > 
> > Best,
> > Tiwei Bie
> 
> While true, that does not prevent us from implementing Don's suggestion, as it
> should fix the bug you are looking at with your original patch, and also allow
> additional use-cases for applications at no extra cost.
> 

As I understand it, what Don wants is something like this:

int main(int argc, char **argv)
{
	int ret;
	int ch;

	while ((ch = getopt(argc, argv, "whateveroptions:d")) != -1) {
		switch (ch) {
		case 'd':
			/* rte_eal_init() will be called */
			ret = dpdk_init(argc, argv);
			argc -= ret;
			argv += ret;
			break;
		case 'w':
			......
		}
	}

	argc -= optind;
	argv += optind;

	......
}

static int
dpdk_init(int argc, char **argv)
{
	int ret;

	ret = rte_eal_init(argc, argv);
	if (ret < 0)
		FATAL_ERROR("Could not initialise EAL (%d)", ret);

	......

	return (ret);
}

And the current code should work correctly if DPDK's parameters are
put together and terminated by '--':

$ ./demo -whatever -d -c f -n 2 -- -option -s hello

The only limitation is that the return value of rte_eal_init() must
be returned to the code which is using getopt() to parse argc/argv.

And I'm very willing to rework my patch to get rid of this limitation.

Best regards,
Tiwei Bie

> /Bruce
> 
> > -----Original Message-----
> > From: Tiwei Bie [mailto:btw@mail.ustc.edu.cn]
> > Sent: Wednesday, October 14, 2015 6:48 AM
> > To: Zhu, Heqing
> > Cc: Richardson, Bruce; O'Driscoll, Tim; Ananyev, Konstantin
> > Subject: Re: [dpdk-dev] [PATCH] eal/bsd: reinitialize optind and optreset
> > to 1
> > 
> > On Wed, Oct 14, 2015 at 05:26:17AM +0000, Zhu, Heqing wrote:
> > >
> > > Bruce, thank you. Tiwei will join DPDK team in 2016. :)
> > >
> > > @Tiwei, Bruce/Konstantin are the DPDK tech leads, both work and live in
> > Ireland.
> > >
> > 
> > Cool! It's my great honor to join DPDK team! Thank you very much! :-)
> > 
> > Best regards,
> > Tiwei Bie
> > 
> > >
> > >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> > > Sent: Tuesday, October 13, 2015 10:59 PM
> > > To: Tiwei Bie
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH] eal/bsd: reinitialize optind and
> > > optreset to 1
> > >
> > > On Tue, Oct 13, 2015 at 04:54:06PM +0800, Tiwei Bie wrote:
> > > > The variable optind must be reinitialized to 1 in order to skip over
> > > > argv[0] on FreeBSD. Because getopt() on FreeBSD will return -1 when
> > > > it meets an argument which doesn't start with '-'.
> > > >
> > > > The variable optreset is provided on FreeBSD to indicate the
> > > > additional set of calls to getopt(). So, also reinitialize it to 1.
> > > >
> > > > Signed-off-by: Tiwei Bie <btw@mail.ustc.edu.cn>
> > >
> > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > >
> 


> /Bruce
> 
> > 
> > > -don provan
> > > dprovan@bivio.net
> > > 
> > > -----Original Message-----
> > > From: Tiwei Bie [mailto:btw@mail.ustc.edu.cn] 
> > > Sent: Tuesday, October 13, 2015 1:54 AM
> > > To: dev@dpdk.org
> > > Subject: [dpdk-dev] [PATCH] eal/bsd: reinitialize optind and optreset to 1
> > > 
> > > The variable optind must be reinitialized to 1 in order to skip over argv[0] on FreeBSD. Because getopt() on FreeBSD will return -1 when it meets an argument which doesn't start with '-'.
> > > 
> > > The variable optreset is provided on FreeBSD to indicate the additional set of calls to getopt(). So, also reinitialize it to 1.
> > > 
> > > Signed-off-by: Tiwei Bie <btw@mail.ustc.edu.cn>
> > > ---
> > >  lib/librte_eal/bsdapp/eal/eal.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c index 1b6f705..35feaee 100644
> > > --- a/lib/librte_eal/bsdapp/eal/eal.c
> > > +++ b/lib/librte_eal/bsdapp/eal/eal.c
> > > @@ -334,7 +334,8 @@ eal_log_level_parse(int argc, char **argv)
> > >  			break;
> > >  	}
> > >  
> > > -	optind = 0; /* reset getopt lib */
> > > +	optind = 1; /* reset getopt lib */
> > > +	optreset = 1;
> > >  }
> > >  
> > >  /* Parse the argument given in the command line of the application */ @@ -403,7 +404,8 @@ eal_parse_args(int argc, char **argv)
> > >  	if (optind >= 0)
> > >  		argv[optind-1] = prgname;
> > >  	ret = optind-1;
> > > -	optind = 0; /* reset getopt lib */
> > > +	optind = 1; /* reset getopt lib */
> > > +	optreset = 1;
> > >  	return ret;
> > >  }
> > >  
> > > --
> > > 2.6.0
> > > 
> > > 
> > > 
> > 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] eal/bsd: reinitialize optind and optreset to 1
  2015-10-14 10:19         ` Tiwei Bie
@ 2015-10-14 11:28           ` Tiwei Bie
  2015-10-14 17:54             ` Don Provan
  0 siblings, 1 reply; 9+ messages in thread
From: Tiwei Bie @ 2015-10-14 11:28 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev, Don Provan

On Wed, Oct 14, 2015 at 06:19:33PM +0800, Tiwei Bie wrote:
> On Wed, Oct 14, 2015 at 10:31:28AM +0100, Bruce Richardson wrote:
> > On Wed, Oct 14, 2015 at 10:28:44AM +0800, Tiwei Bie wrote:
> > > On Tue, Oct 13, 2015 at 05:14:38PM +0000, Don Provan wrote:
> > > > Actually, this is a good opportunity to fix a bug that's been in this code forever: it shouldn't be resetting optind to some arbitrary value: it should be saving optind (and optarg and optopt) at the beginning, initializing optind to 1 before calling getopt_long(), then restoring all the values after. (And, from what you're saying, optreset should be handled the same as optind.)
> > > > 
> > > 
> > > It is designed to have DPDK's parameters specified in the front of the
> > > cmd line and terminated by '--'. Or at least, you should put DPDK's
> > > parameters together and terminate them by '--'. And 1 or 0 are not some
> > > arbitrary values. They are used to put the index back to the beginning
> > > of the new argv[] array.
> > > 
> > > > This avoids broken behavior if rte_eal_init() is called by code that's in the middle of using getopt() to parse its own unrelated argc/argv parameters. 
> > > > 
> > > 
> > > We shouldn't mix up DPDK's parameters and application's parameters.
> > > And we should group them using '--'.
> > > 
> > > Best,
> > > Tiwei Bie
> > 
> > While true, that does not prevent us from implementing Don's suggestion, as it
> > should fix the bug you are looking at with your original patch, and also allow
> > additional use-cases for applications at no extra cost.
> > 
> 
> As I understand it, what Don wants is something like this:
> 
> int main(int argc, char **argv)
> {
> 	int ret;
> 	int ch;
> 
> 	while ((ch = getopt(argc, argv, "whateveroptions:d")) != -1) {
> 		switch (ch) {
> 		case 'd':
> 			/* rte_eal_init() will be called */
> 			ret = dpdk_init(argc, argv);
> 			argc -= ret;
> 			argv += ret;
> 			break;
> 		case 'w':
> 			......
> 		}
> 	}
> 
> 	argc -= optind;
> 	argv += optind;
> 
> 	......
> }
> 
> static int
> dpdk_init(int argc, char **argv)
> {
> 	int ret;
> 
> 	ret = rte_eal_init(argc, argv);
> 	if (ret < 0)
> 		FATAL_ERROR("Could not initialise EAL (%d)", ret);
> 
> 	......
> 
> 	return (ret);
> }
> 
> And the current code should work correctly if DPDK's parameters are
> put together and terminated by '--':
> 
> $ ./demo -whatever -d -c f -n 2 -- -option -s hello
> 

eal_log_level_parse() needs to be reworked to make the code work correctly.

> The only limitation is that the return value of rte_eal_init() must
> be returned to the code which is using getopt() to parse argc/argv.
> 
> And I'm very willing to rework my patch to get rid of this limitation.
> 

I'm considering updating optind to make it point to the option after
'--' before eal_parse_args() returns, and eal_parse_args() will return
0 to avoid breaking the current applications which use the return value
of rte_eal_init() to update argc/argv.

Any comments? Thanks! :-)

Best regards,
Tiwei Bie

> Best regards,
> Tiwei Bie
> 
> > /Bruce
> > 
> > > 
> > > > -don provan
> > > > dprovan@bivio.net
> > > > 
> > > > -----Original Message-----
> > > > From: Tiwei Bie [mailto:btw@mail.ustc.edu.cn] 
> > > > Sent: Tuesday, October 13, 2015 1:54 AM
> > > > To: dev@dpdk.org
> > > > Subject: [dpdk-dev] [PATCH] eal/bsd: reinitialize optind and optreset to 1
> > > > 
> > > > The variable optind must be reinitialized to 1 in order to skip over argv[0] on FreeBSD. Because getopt() on FreeBSD will return -1 when it meets an argument which doesn't start with '-'.
> > > > 
> > > > The variable optreset is provided on FreeBSD to indicate the additional set of calls to getopt(). So, also reinitialize it to 1.
> > > > 
> > > > Signed-off-by: Tiwei Bie <btw@mail.ustc.edu.cn>
> > > > ---
> > > >  lib/librte_eal/bsdapp/eal/eal.c | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c index 1b6f705..35feaee 100644
> > > > --- a/lib/librte_eal/bsdapp/eal/eal.c
> > > > +++ b/lib/librte_eal/bsdapp/eal/eal.c
> > > > @@ -334,7 +334,8 @@ eal_log_level_parse(int argc, char **argv)
> > > >  			break;
> > > >  	}
> > > >  
> > > > -	optind = 0; /* reset getopt lib */
> > > > +	optind = 1; /* reset getopt lib */
> > > > +	optreset = 1;
> > > >  }
> > > >  
> > > >  /* Parse the argument given in the command line of the application */ @@ -403,7 +404,8 @@ eal_parse_args(int argc, char **argv)
> > > >  	if (optind >= 0)
> > > >  		argv[optind-1] = prgname;
> > > >  	ret = optind-1;
> > > > -	optind = 0; /* reset getopt lib */
> > > > +	optind = 1; /* reset getopt lib */
> > > > +	optreset = 1;
> > > >  	return ret;
> > > >  }
> > > >  
> > > > --
> > > > 2.6.0
> > > > 
> > > > 
> > > > 
> > > 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] eal/bsd: reinitialize optind and optreset to 1
  2015-10-14 11:28           ` Tiwei Bie
@ 2015-10-14 17:54             ` Don Provan
  2015-10-15  1:40               ` Tiwei Bie
  0 siblings, 1 reply; 9+ messages in thread
From: Don Provan @ 2015-10-14 17:54 UTC (permalink / raw)
  To: Tiwei Bie, Bruce Richardson; +Cc: dev

> > On Wed, Oct 14, 2015 at 10:28:44AM +0800, Tiwei Bie wrote:
> > > It is designed to have DPDK's parameters specified in the front of the
> > > cmd line and terminated by '--'.

In other words, it is designed assuming the DPDK library can dictate
the application's command line. This is an incorrect assumption
that should be eliminated.

I don't think I'm the only one that has figured out that layering a
service on top of DPDK requires creating a fake argc/argv array.
The original plan of having rte_eal_init() parse the actual application
command line organized as dictated by DPDK is not reasonable.

> > > And 1 or 0 are not some
> > > arbitrary values. They are used to put the index back to the beginning
> > > of the new argv[] array.

They're arbitrary values from the point of view of the calling
application. If the caller is using getopt() for its own purposes,
it has every reason to expect optind to have the same value
after the call to rte_eal_init() that it had before, not some
other value that the DPDK library happened to think was
useful.

> > > We shouldn't mix up DPDK's parameters and application's parameters.
> > > And we should group them using '--'.

Exactly! Yet we do confuse the two by using getopt() without considering
that the application's parameters might not be in the argc/argv list that's
passed to rte_eal_init().

> I'm considering updating optind to make it point to the option after
> '--' before eal_parse_args() returns, and eal_parse_args() will return
> 0 to avoid breaking the current applications which use the return value
> of rte_eal_init() to update argc/argv.
>
> Any comments? Thanks! :-)

Don't do it. Last time I looked, optind wasn't even mentioned in the
documentation. Even if I thought it was reasonable to change it,
I would still argue against using it as an undocumented return
value, particularly when the documented return value works fine.

-don provan
dprovan@bivio.net

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] eal/bsd: reinitialize optind and optreset to 1
  2015-10-14 17:54             ` Don Provan
@ 2015-10-15  1:40               ` Tiwei Bie
  0 siblings, 0 replies; 9+ messages in thread
From: Tiwei Bie @ 2015-10-15  1:40 UTC (permalink / raw)
  To: Don Provan; +Cc: dev

Hi Don!

I'm truly sorry for my misunderstanding. :-(
Thank you so much for your detailed comments!

I will update my patch!

Thanks again!

Best wishes,
Tiwei Bie

On Wed, Oct 14, 2015 at 05:54:14PM +0000, Don Provan wrote:
> > > On Wed, Oct 14, 2015 at 10:28:44AM +0800, Tiwei Bie wrote:
> > > > It is designed to have DPDK's parameters specified in the front of the
> > > > cmd line and terminated by '--'.
> 
> In other words, it is designed assuming the DPDK library can dictate
> the application's command line. This is an incorrect assumption
> that should be eliminated.
> 
> I don't think I'm the only one that has figured out that layering a
> service on top of DPDK requires creating a fake argc/argv array.
> The original plan of having rte_eal_init() parse the actual application
> command line organized as dictated by DPDK is not reasonable.
> 
> > > > And 1 or 0 are not some
> > > > arbitrary values. They are used to put the index back to the beginning
> > > > of the new argv[] array.
> 
> They're arbitrary values from the point of view of the calling
> application. If the caller is using getopt() for its own purposes,
> it has every reason to expect optind to have the same value
> after the call to rte_eal_init() that it had before, not some
> other value that the DPDK library happened to think was
> useful.
> 
> > > > We shouldn't mix up DPDK's parameters and application's parameters.
> > > > And we should group them using '--'.
> 
> Exactly! Yet we do confuse the two by using getopt() without considering
> that the application's parameters might not be in the argc/argv list that's
> passed to rte_eal_init().
> 
> > I'm considering updating optind to make it point to the option after
> > '--' before eal_parse_args() returns, and eal_parse_args() will return
> > 0 to avoid breaking the current applications which use the return value
> > of rte_eal_init() to update argc/argv.
> >
> > Any comments? Thanks! :-)
> 
> Don't do it. Last time I looked, optind wasn't even mentioned in the
> documentation. Even if I thought it was reasonable to change it,
> I would still argue against using it as an undocumented return
> value, particularly when the documented return value works fine.
> 
> -don provan
> dprovan@bivio.net

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2015-10-15  1:41 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-13  8:54 [dpdk-dev] [PATCH] Found a bug related to getopt() in eal/bsd module Tiwei Bie
2015-10-13  8:54 ` [dpdk-dev] [PATCH] eal/bsd: reinitialize optind and optreset to 1 Tiwei Bie
2015-10-13 14:58   ` Bruce Richardson
2015-10-13 17:14   ` Don Provan
     [not found]     ` <20151014022843.GA26774@dell>
2015-10-14  9:31       ` Bruce Richardson
2015-10-14 10:19         ` Tiwei Bie
2015-10-14 11:28           ` Tiwei Bie
2015-10-14 17:54             ` Don Provan
2015-10-15  1:40               ` Tiwei Bie

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).