From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/6616 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] getopt: add support for non-option arguments Date: Mon, 24 Nov 2014 21:48:01 -0500 Message-ID: <20141125024801.GP29621@brightrain.aerifal.cx> References: <1416854345-5252-1-git-send-email-gianluca@sottospazio.it> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1416883737 3201 80.91.229.3 (25 Nov 2014 02:48:57 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Tue, 25 Nov 2014 02:48:57 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-6629-gllmg-musl=m.gmane.org@lists.openwall.com Tue Nov 25 03:48:49 2014 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1Xt6BL-0006OH-9F for gllmg-musl@m.gmane.org; Tue, 25 Nov 2014 03:48:47 +0100 Original-Received: (qmail 11635 invoked by uid 550); 25 Nov 2014 02:48:14 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 11594 invoked from network); 25 Nov 2014 02:48:13 -0000 Content-Disposition: inline In-Reply-To: <1416854345-5252-1-git-send-email-gianluca@sottospazio.it> User-Agent: Mutt/1.5.21 (2010-09-15) Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:6616 Archived-At: 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 > --- > 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