[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20141125024801.GP29621@brightrain.aerifal.cx>
Date: Mon, 24 Nov 2014 21:48:01 -0500
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] getopt: add support for non-option arguments
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@...tospazio.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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
