[dpdk-dev] eal/bsd: reinitialize optind and optreset to 1

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

 	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
  

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

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

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

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

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

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

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