[PATCH] getopt: add support for non-option arguments

[PATCH] getopt: add support for non-option arguments

[Gianluca Anzolin]

Currently getopt() doesn't handle the GNU getopt extension that allows
to parse non-option arguments when optstring starts with '-'.

This extensions is used by some common utilities, notably iptables, that
currently return with errors even with perfectly valid invocations, for
example:

$ iptables -A INPUT -p tcp ! --syn -m state --state NEW -j DROP

The patch add the code needed to implement this extension to getopt.c
and getopt_long.c

Signed-off-by: Gianluca Anzolin <gianluca@sottospazio.it>
---
 src/misc/getopt.c      | 17 ++++++++++++++++-
 src/misc/getopt_long.c |  6 +++++-
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/src/misc/getopt.c b/src/misc/getopt.c
index f94c4f7..a698c8d 100644
--- a/src/misc/getopt.c
+++ b/src/misc/getopt.c
@@ -24,8 +24,20 @@ int getopt(int argc, char * const argv[], const char *optstring)
 		optind = 1;
 	}
 
-	if (optind >= argc || !argv[optind] || argv[optind][0] != '-' || !argv[optind][1])
+	if (optind >= argc || !argv[optind])
 		return -1;
+
+	if (argv[optind][0] != '-') {
+		if (optstring[0] == '-') {
+			optarg = argv[optind++];
+			return 1;
+		}
+		return -1;
+	}
+
+	if (!argv[optind][1])
+		return -1;
+
 	if (argv[optind][1] == '-' && !argv[optind][2])
 		return optind++, -1;
 
@@ -43,6 +55,9 @@ int getopt(int argc, char * const argv[], const char *optstring)
 		optpos = 0;
 	}
 
+	if (optstring[0] == '-')
+		optstring++;
+
 	for (i=0; (l = mbtowc(&d, optstring+i, MB_LEN_MAX)) && d!=c; i+=l>0?l:1);
 
 	if (d != c) {
diff --git a/src/misc/getopt_long.c b/src/misc/getopt_long.c
index 4ef5a5c..d8b2b66 100644
--- a/src/misc/getopt_long.c
+++ b/src/misc/getopt_long.c
@@ -12,7 +12,11 @@ static int __getopt_long(int argc, char *const *argv, const char *optstring, con
 		__optpos = 0;
 		optind = 1;
 	}
-	if (optind >= argc || !argv[optind] || argv[optind][0] != '-') return -1;
+	if (optind >= argc || !argv[optind]) return -1;
+
+	if (argv[optind][0] != '-')
+		return getopt(argc, argv, optstring);
+
 	if ((longonly && argv[optind][1]) ||
 		(argv[optind][1] == '-' && argv[optind][2]))
 	{
-- 
2.1.3

Re: [PATCH] getopt: add support for non-option arguments

[Rich Felker]

On Mon, Nov 24, 2014 at 07:39:05PM +0100, Gianluca Anzolin wrote:
> Currently getopt() doesn't handle the GNU getopt extension that allows
> to parse non-option arguments when optstring starts with '-'.
> 
> This extensions is used by some common utilities, notably iptables, that
> currently return with errors even with perfectly valid invocations, for
> example:
> 
> $ iptables -A INPUT -p tcp ! --syn -m state --state NEW -j DROP
> 
> The patch add the code needed to implement this extension to getopt.c
> and getopt_long.c

This looks basically correct, and I like how clean and direct the
patch is (all changes are small and clearly motivated). Thanks!

Some comments below:

> Signed-off-by: Gianluca Anzolin <gianluca@sottospazio.it>
> ---
>  src/misc/getopt.c      | 17 ++++++++++++++++-
>  src/misc/getopt_long.c |  6 +++++-
>  2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/src/misc/getopt.c b/src/misc/getopt.c
> index f94c4f7..a698c8d 100644
> --- a/src/misc/getopt.c
> +++ b/src/misc/getopt.c
> @@ -24,8 +24,20 @@ int getopt(int argc, char * const argv[], const char *optstring)
>  		optind = 1;
>  	}
>  
> -	if (optind >= argc || !argv[optind] || argv[optind][0] != '-' || !argv[optind][1])
> +	if (optind >= argc || !argv[optind])
>  		return -1;
> +
> +	if (argv[optind][0] != '-') {
> +		if (optstring[0] == '-') {
> +			optarg = argv[optind++];
> +			return 1;
> +		}
> +		return -1;
> +	}
> +
> +	if (!argv[optind][1])
> +		return -1;
> +
>  	if (argv[optind][1] == '-' && !argv[optind][2])
>  		return optind++, -1;
>  
> @@ -43,6 +55,9 @@ int getopt(int argc, char * const argv[], const char *optstring)
>  		optpos = 0;
>  	}
>  
> +	if (optstring[0] == '-')
> +		optstring++;
> +
>  	for (i=0; (l = mbtowc(&d, optstring+i, MB_LEN_MAX)) && d!=c; i+=l>0?l:1);
>  
>  	if (d != c) {
> diff --git a/src/misc/getopt_long.c b/src/misc/getopt_long.c
> index 4ef5a5c..d8b2b66 100644
> --- a/src/misc/getopt_long.c
> +++ b/src/misc/getopt_long.c
> @@ -12,7 +12,11 @@ static int __getopt_long(int argc, char *const *argv, const char *optstring, con
>  		__optpos = 0;
>  		optind = 1;
>  	}
> -	if (optind >= argc || !argv[optind] || argv[optind][0] != '-') return -1;
> +	if (optind >= argc || !argv[optind]) return -1;
> +
> +	if (argv[optind][0] != '-')
> +		return getopt(argc, argv, optstring);
> +
>  	if ((longonly && argv[optind][1]) ||
>  		(argv[optind][1] == '-' && argv[optind][2]))
>  	{
> -- 
> 2.1.3

For getopt_long, this introduces a second code path to return
getopt(...). Wouldn't it be cleaner just to add an additional
condition of (argv[optind][0] == '-') to the if statement containing
the main body of the function?

I'm a little bit uncertain what's really best to do with getopt_long,
but since there's a pending patch for abbreviated long options and
there's demand to add GNU-style argv permutation to getopt_long (but
not plain getopt), this code is likely to need major changes anyway
in this release cycle, so I'm not too concerned that we do it the
"best possible way" right now anyway. So if it works we can probably
go with it.

I'll see if anyone else has comments and if not commit (possibly with
the above modification to getopt_long?) soon.

Rich

Re: [PATCH] getopt: add support for non-option arguments

[Gianluca Anzolin]

Hello,

On Mon, Nov 24, 2014 at 09:48:01PM -0500, Rich Felker wrote:
> On Mon, Nov 24, 2014 at 07:39:05PM +0100, Gianluca Anzolin wrote:
> > diff --git a/src/misc/getopt_long.c b/src/misc/getopt_long.c
> > index 4ef5a5c..d8b2b66 100644
> > --- a/src/misc/getopt_long.c
> > +++ b/src/misc/getopt_long.c
> > @@ -12,7 +12,11 @@ static int __getopt_long(int argc, char *const *argv, const char *optstring, con
> >  		__optpos = 0;
> >  		optind = 1;
> >  	}
> > -	if (optind >= argc || !argv[optind] || argv[optind][0] != '-') return -1;
> > +	if (optind >= argc || !argv[optind]) return -1;
> > +
> > +	if (argv[optind][0] != '-')
> > +		return getopt(argc, argv, optstring);
> > +
> >  	if ((longonly && argv[optind][1]) ||
> >  		(argv[optind][1] == '-' && argv[optind][2]))
> >  	{
> > -- 
> > 2.1.3
> 
> For getopt_long, this introduces a second code path to return
> getopt(...). Wouldn't it be cleaner just to add an additional
> condition of (argv[optind][0] == '-') to the if statement containing
> the main body of the function?

Yes, that seems better, I will do it.

> I'm a little bit uncertain what's really best to do with getopt_long,
> but since there's a pending patch for abbreviated long options and
> there's demand to add GNU-style argv permutation to getopt_long (but
> not plain getopt), this code is likely to need major changes anyway
> in this release cycle, so I'm not too concerned that we do it the
> "best possible way" right now anyway. So if it works we can probably
> go with it.
> 

I tested the changes and verified that they work correctly. However I found
something worth mentioning, not caused by the patch itself.

For a command line like the following (optstring="a:b:c:def")

$ ./test -a 1 -b 2 -c 3 -d -e -f

for the options d, e and f the variable optarg is "3". Maybe this is the
intended behaviour, after all those options aren't supposed to get an argument,
but glibc always sets optarg to NULL in these cases.

> I'll see if anyone else has comments and if not commit (possibly with
> the above modification to getopt_long?) soon.
> 
> Rich

I'll send a new patch soon,

Thanks

Gianluca

Re: [PATCH] getopt: add support for non-option arguments

[Rich Felker]

On Tue, Nov 25, 2014 at 08:33:50AM +0100, Gianluca Anzolin wrote:
> > I'm a little bit uncertain what's really best to do with getopt_long,
> > but since there's a pending patch for abbreviated long options and
> > there's demand to add GNU-style argv permutation to getopt_long (but
> > not plain getopt), this code is likely to need major changes anyway
> > in this release cycle, so I'm not too concerned that we do it the
> > "best possible way" right now anyway. So if it works we can probably
> > go with it.
> > 
> 
> I tested the changes and verified that they work correctly. However I found
> something worth mentioning, not caused by the patch itself.
> 
> For a command line like the following (optstring="a:b:c:def")
> 
> $ ./test -a 1 -b 2 -c 3 -d -e -f
> 
> for the options d, e and f the variable optarg is "3". Maybe this is the
> intended behaviour, after all those options aren't supposed to get an argument,
> but glibc always sets optarg to NULL in these cases.

This is a bug in glibc. getopt is specified to set optarg only in the
case where the option takes an argument.

Rich