PATCH: 3.1.6-pws-3: more getopts bugs

PATCH: 3.1.6-pws-3: more getopts bugs

[Peter Stephenson]

I haven't been using getopts in the zf* functions because people keep
reporting bugs in it.  Looks like I was right.  Here are bugs number 343
and 344.

1. The counter for how far we are into a single option argument doesn't get
rest when OPTIND goes back to 1 (i.e. when a new function is started), only
if OPTIND is set explicitly to 0.  (How can this possibly have escaped
notice for almost a decade?)  One way to see this is:

badfn() {
  local opt
  while getopts :abc opt; do
    [[ $opt = '?' ]] && print "Bad option $OPTARG" && return 1
  done
  return 0
}
badfn -X
badfn -X

The first time you get the error message as expected, the second time you
don't because getopts thinks it's already at the end of the option list.

2. The manual page claims the option variable gets set to '?' even if an
error message was printed by getopts itself (i.e. no initial colon).  Guess
what?  To see that, remove the : and watch the second line not get
executed.

--- Src/builtin.c.optc	Tue Sep  7 09:24:00 1999
+++ Src/builtin.c	Fri Sep 10 10:33:45 1999
@@ -2936,6 +2936,9 @@
     return ret;
 }
 
+/**/
+int optcind;
+
 /* getopts: automagical option handling for shell scripts */
 
 /**/
@@ -2945,7 +2948,6 @@
     int lenstr, lenoptstr, quiet, lenoptbuf;
     char *optstr = unmetafy(*argv++, &lenoptstr), *var = *argv++;
     char **args = (*argv) ? argv : pparams;
-    static int optcind = 0;
     char *str, optbuf[2] = " ", *p, opch;
 
     /* zoptind keeps count of the current argument number.  The *
@@ -2994,8 +2996,8 @@
 	p = "?";
 err:
 	zsfree(zoptarg);
+	setsparam(var, ztrdup(p));
 	if(quiet) {
-	    setsparam(var, ztrdup(p));
 	    zoptarg = metafy(optbuf, lenoptbuf, META_DUP);
 	} else {
 	    zerr(*p == '?' ? "bad option: -%c" :
--- Src/exec.c.optc	Wed Sep  1 14:32:50 1999
+++ Src/exec.c	Fri Sep 10 10:28:32 1999
@@ -2918,7 +2918,7 @@
  * was executed.                                            */
 {
     char **tab, **x, *oargv0 = NULL;
-    int oldzoptind, oldlastval;
+    int oldzoptind, oldlastval, oldoptcind;
     char saveopts[OPT_SIZE], *oldscriptname;
     int obreaks = breaks;
 
@@ -2935,6 +2935,8 @@
 	scriptname = dupstring(name);
 	oldzoptind = zoptind;
 	zoptind = 1;
+	oldoptcind = optcind;
+	optcind = 0;
 
 	/* We need to save the current options even if LOCALOPTIONS is *
 	 * not currently set.  That's because if it gets set in the    *
@@ -2974,6 +2976,7 @@
 	    zsfree(argzero);
 	    argzero = oargv0;
 	}
+	optcind = oldoptcind;
 	zoptind = oldzoptind;
 	scriptname = oldscriptname;
 	pparams = tab;

-- 
Peter Stephenson <pws@ibmth.df.unipi.it>       Tel: +39 050 844536
WWW:  http://www.ifh.de/~pws/
Dipartimento di Fisica, Via Buonarroti 2, 56127 Pisa, Italy

Re: PATCH: 3.1.6-pws-3: more getopts bugs

[Bart Schaefer]

On Sep 10, 11:21am, Peter Stephenson wrote:
} Subject: PATCH: 3.1.6-pws-3: more getopts bugs
}
} 1. The counter for how far we are into a single option argument doesn't get
} rest when OPTIND goes back to 1 (i.e. when a new function is started), only
} if OPTIND is set explicitly to 0.  (How can this possibly have escaped
} notice for almost a decade?)

It went unnoticed because it was introduced in January of 1998 when Zefram
"Rewrote getopts to remove its various bugs."  (Or so says ChangeLog.)  It
is not present in 3.0.6 (try your "badfn" test case).

} 2. The manual page claims the option variable gets set to '?' even if an
} error message was printed by getopts itself (i.e. no initial colon).  Guess
} what?  To see that, remove the : and watch the second line not get
} executed.

This also works as expected in 3.0.6.  I wonder what bugs Zefram removed?

Anyway, as this works in 3.0.6 with optcind static to bin_getopts(), I
wonder whether saving/restoring optcind in exec.c is in fact the right
thing.  The diffs in bin_getopts() from 3.0.6 to 3.1.6 are so extensive
that I'm not immediately able to discern what's happening differently.
It looks like maybe the 3.0 code resets optcind before returning end of
options, whereas the 3.1 code sets it on entry to the function iff the
user has set OPTIND=0.

Restarting on OPTIND=0 must be one of the bugs Zefram fixed, but maybe
he shouldn't have taken out the reset of optcind upon failure return?


-- 
Bart Schaefer                                 Brass Lantern Enterprises
http://www.well.com/user/barts              http://www.brasslantern.com

Re: PATCH: 3.1.6-pws-3: more getopts bugs

[Zefram]

Bart Schaefer wrote:
>This also works as expected in 3.0.6.  I wonder what bugs Zefram removed?

I don't remember exactly; ISTR there being a bunch of fencepost errors,
error handling errors, errors handling unusual cases and so on.  The code
was so messy that it was easier to start from scratch -- most patches
to that code were introducing new errors while fixing the old ones.
Of course, getopts is a particularly bad design, so the code can't help
being messy to some extent.

>Restarting on OPTIND=0 must be one of the bugs Zefram fixed, but maybe
>he shouldn't have taken out the reset of optcind upon failure return?

Probably.  I remember having some difficulty working out where optcind
should be reset -- it's all very implicit in the POSIX definition.

-zefram