getopt: better handling of ambiguous options
authorZack Weinberg <zackw@panix.com>
Thu, 6 Apr 2017 18:14:14 +0000 (11:14 -0700)
committerPaul Eggert <eggert@cs.ucla.edu>
Thu, 6 Apr 2017 22:42:04 +0000 (15:42 -0700)
glibc's getopt uses alloca to construct a linked list of possibilities
for an "ambiguous" long option.  In gnulib, malloc should be used
instead.  Providing for both cases complicates things a fair bit.

This patch rewrites ambiguous-option handling to use a boolean vector
instead of a linked list.  There is then only one allocation that
might need freeing; in glibc it can honor __libc_use_alloca as usual,
and in gnulib we define __libc_use_alloca to always be false, so we
don't need ifdefs in the middle of the function.  This should also be
slightly more efficient in the normal case of long options being fully
spelled out -- I think most people aren't even aware they _can_
sometimes abbreviate long options.

One interesting consequence is that the list of possibilities is now
printed in exactly the order they appear in the list of long options,
instead of the first possibility being shuffled to the end.

(The patch looks bigger than it really is because there's a fair bit
of reindentation and code rearrangement.)

* lib/getopt.c: When used standalone, define __libc_use_alloca
as always false and alloca to abort if called.
(process_long_option): Rewrite handling of ambiguous long options
to use a single boolean vector, not a linked list; use
__libc_use_alloca to decide whether to allocate this using alloca.

ChangeLog
lib/getopt.c

index a1ad1bd6d813f958e99c14df8c7e4460866b6382..eb2f347a60c46efb089a2ffe85b2e3e6d4d3c8b0 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,34 @@
 2017-04-06  Zack Weinberg  <zackw@panix.com>
 
+       getopt: better handling of ambiguous options
+
+       glibc's getopt uses alloca to construct a linked list of possibilities
+       for an "ambiguous" long option.  In gnulib, malloc should be used
+       instead.  Providing for both cases complicates things a fair bit.
+
+       This patch rewrites ambiguous-option handling to use a boolean vector
+       instead of a linked list.  There is then only one allocation that
+       might need freeing; in glibc it can honor __libc_use_alloca as usual,
+       and in gnulib we define __libc_use_alloca to always be false, so we
+       don't need ifdefs in the middle of the function.  This should also be
+       slightly more efficient in the normal case of long options being fully
+       spelled out -- I think most people aren't even aware they _can_
+       sometimes abbreviate long options.
+
+       One interesting consequence is that the list of possibilities is now
+       printed in exactly the order they appear in the list of long options,
+       instead of the first possibility being shuffled to the end.
+
+       (The patch looks bigger than it really is because there's a fair bit
+       of reindentation and code rearrangement.)
+
+       * lib/getopt.c: When used standalone, define __libc_use_alloca
+       as always false and alloca to abort if called.
+       (process_long_option): Rewrite handling of ambiguous long options
+       to use a single boolean vector, not a linked list; use
+       __libc_use_alloca to decide whether to allocate this using alloca.
+
+
        getopt: refactor long-option handling
 
        There were two copies of the bulk of the code to handle long options.
index 51963746d95dca4ae89b3af788a0e5775b971181..6671787b6fc4d20bf9e94038c9e217b0c411abaf 100644 (file)
 #  define flockfile(fp) /* nop */
 #  define funlockfile(fp) /* nop */
 # endif
+/* When used standalone, do not attempt to use alloca.  */
+# define __libc_use_alloca(size) 0
+# undef alloca
+# define alloca(size) (abort (), (void *)0)
 #endif
 
 /* This implementation of 'getopt' has three modes for handling
@@ -197,150 +201,179 @@ process_long_option (int argc, char **argv, const char *optstring,
   size_t namelen;
   const struct option *p;
   const struct option *pfound = NULL;
-  struct option_list
-  {
-    const struct option *p;
-    struct option_list *next;
-  } *ambig_list = NULL;
-  int exact = 0;
-  int indfound = -1;
+  int n_options;
   int option_index;
 
   for (nameend = d->__nextchar; *nameend && *nameend != '='; nameend++)
     /* Do nothing.  */ ;
   namelen = nameend - d->__nextchar;
 
-  /* Test all long options for either exact match
-     or abbreviated matches.  */
-  for (p = longopts, option_index = 0; p->name; p++, option_index++)
-    if (!strncmp (p->name, d->__nextchar, namelen))
+  /* First look for an exact match, counting the options as a side
+     effect.  */
+  for (p = longopts, n_options = 0; p->name; p++, n_options++)
+    if (!strncmp (p->name, d->__nextchar, namelen)
+       && namelen == strlen (p->name))
       {
-        if (namelen == strlen (p->name))
-          {
-            /* Exact match found.  */
-            pfound = p;
-            indfound = option_index;
-            exact = 1;
-            break;
-          }
-        else if (pfound == NULL)
-          {
-            /* First nonexact match found.  */
-            pfound = p;
-            indfound = option_index;
-          }
-        else if (long_only
-                 || pfound->has_arg != p->has_arg
-                 || pfound->flag != p->flag
-                 || pfound->val != p->val)
-          {
-            /* Second or later nonexact match found.  */
-            struct option_list *newp = alloca (sizeof (*newp));
-            newp->p = p;
-            newp->next = ambig_list;
-            ambig_list = newp;
-          }
+       /* Exact match found.  */
+       pfound = p;
+       option_index = n_options;
+       break;
       }
 
-  if (ambig_list != NULL && !exact)
+  if (pfound == NULL)
     {
-      if (print_errors)
-        {
-          struct option_list first;
-          first.p = pfound;
-          first.next = ambig_list;
-          ambig_list = &first;
-
-          flockfile (stderr);
-
-          fprintf (stderr, _("%s: option '%s' is ambiguous; possibilities:"),
-                   argv[0], argv[d->optind]);
-          do
-            {
-              fprintf (stderr, " '--%s'", ambig_list->p->name);
-              ambig_list = ambig_list->next;
-            }
-          while (ambig_list != NULL);
-
-          /* This must use 'fprintf' even though it's only printing a
-             single character, so that it goes through __fxprintf_nocancel
-             when compiled as part of glibc.  */
-          fprintf (stderr, "\n");
-          funlockfile (stderr);
-        }
-      d->__nextchar += strlen (d->__nextchar);
-      d->optind++;
-      d->optopt = 0;
-      return '?';
+      /* Didn't find an exact match, so look for abbreviations.  */
+      unsigned char *ambig_set = NULL;
+      int ambig_malloced = 0;
+      int ambig_fallback = 0;
+      int indfound = -1;
+
+      for (p = longopts, option_index = 0; p->name; p++, option_index++)
+       if (!strncmp (p->name, d->__nextchar, namelen))
+         {
+           if (pfound == NULL)
+             {
+               /* First nonexact match found.  */
+               pfound = p;
+               indfound = option_index;
+             }
+           else if (long_only
+                    || pfound->has_arg != p->has_arg
+                    || pfound->flag != p->flag
+                    || pfound->val != p->val)
+             {
+               /* Second or later nonexact match found.  */
+               if (!ambig_fallback)
+                 {
+                   if (!print_errors)
+                     /* Don't waste effort tracking the ambig set if
+                        we're not going to print it anyway.  */
+                     ambig_fallback = 1;
+                   else if (!ambig_set)
+                     {
+                       if (__libc_use_alloca (n_options))
+                         ambig_set = alloca (n_options);
+                       else if ((ambig_set = malloc (n_options)) == NULL)
+                         /* Fall back to simpler error message.  */
+                         ambig_fallback = 1;
+                       else
+                         ambig_malloced = 1;
+
+                       if (ambig_set)
+                         {
+                           memset (ambig_set, 0, n_options);
+                           ambig_set[indfound] = 1;
+                         }
+                     }
+                   if (ambig_set)
+                     ambig_set[option_index] = 1;
+                 }
+             }
+         }
+
+      if (ambig_set || ambig_fallback)
+       {
+         if (print_errors)
+           {
+             if (ambig_fallback)
+               fprintf (stderr, _("%s: option '%s%s' is ambiguous\n"),
+                        argv[0], prefix, d->__nextchar);
+             else
+               {
+                 flockfile (stderr);
+                 fprintf (stderr,
+                          _("%s: option '%s%s' is ambiguous; possibilities:"),
+                          argv[0], prefix, d->__nextchar);
+
+                 for (option_index = 0; option_index < n_options; option_index++)
+                   if (ambig_set[option_index])
+                     fprintf (stderr, " '%s%s'",
+                              prefix, longopts[option_index].name);
+
+                 /* This must use 'fprintf' even though it's only
+                    printing a single character, so that it goes through
+                    __fxprintf_nocancel when compiled as part of glibc.  */
+                 fprintf (stderr, "\n");
+                 funlockfile (stderr);
+               }
+           }
+         if (ambig_malloced)
+           free (ambig_set);
+         d->__nextchar += strlen (d->__nextchar);
+         d->optind++;
+         d->optopt = 0;
+         return '?';
+       }
+
+      option_index = indfound;
     }
 
-  if (pfound != NULL)
+  if (pfound == NULL)
     {
-      option_index = indfound;
-      d->optind++;
-      if (*nameend)
-        {
-          /* Don't test has_arg with >, because some C compilers don't
-             allow it to be used on enums.  */
-          if (pfound->has_arg)
-            d->optarg = nameend + 1;
-          else
-            {
-              if (print_errors)
-                fprintf (stderr,
-                         _("%s: option '%s%s' doesn't allow an argument\n"),
-                         argv[0], prefix, pfound->name);
-
-              d->__nextchar += strlen (d->__nextchar);
-              d->optopt = pfound->val;
-              return '?';
-            }
-        }
-      else if (pfound->has_arg == 1)
-        {
-          if (d->optind < argc)
-            d->optarg = argv[d->optind++];
-          else
-            {
-              if (print_errors)
-                fprintf (stderr,
-                         _("%s: option '%s%s' requires an argument\n"),
-                         argv[0], prefix, pfound->name);
-
-              d->__nextchar += strlen (d->__nextchar);
-              d->optopt = pfound->val;
-              return optstring[0] == ':' ? ':' : '?';
-            }
-        }
-      d->__nextchar += strlen (d->__nextchar);
-      if (longind != NULL)
-        *longind = option_index;
-      if (pfound->flag)
-        {
-          *(pfound->flag) = pfound->val;
-          return 0;
-        }
-      return pfound->val;
+      /* Can't find it as a long option.  If this is not getopt_long_only,
+        or the option starts with '--' or is not a valid short option,
+        then it's an error.  */
+      if (!long_only || argv[d->optind][1] == '-'
+         || strchr (optstring, *d->__nextchar) == NULL)
+       {
+         if (print_errors)
+           fprintf (stderr, _("%s: unrecognized option '%s%s'\n"),
+                    argv[0], prefix, d->__nextchar);
+
+         d->__nextchar = NULL;
+         d->optind++;
+         d->optopt = 0;
+         return '?';
+       }
+
+      /* Otherwise interpret it as a short option.  */
+      return -1;
     }
 
-  /* Can't find it as a long option.  If this is not getopt_long_only,
-     or the option starts with '--' or is not a valid short option,
-     then it's an error.  */
-  if (!long_only || argv[d->optind][1] == '-'
-      || strchr (optstring, *d->__nextchar) == NULL)
+  /* We have found a matching long option.  Consume it.  */
+  d->optind++;
+  d->__nextchar = NULL;
+  if (*nameend)
+    {
+      /* Don't test has_arg with >, because some C compilers don't
+        allow it to be used on enums.  */
+      if (pfound->has_arg)
+       d->optarg = nameend + 1;
+      else
+       {
+         if (print_errors)
+           fprintf (stderr,
+                    _("%s: option '%s%s' doesn't allow an argument\n"),
+                    argv[0], prefix, pfound->name);
+
+         d->optopt = pfound->val;
+         return '?';
+       }
+    }
+  else if (pfound->has_arg == 1)
     {
-      if (print_errors)
-        fprintf (stderr, _("%s: unrecognized option '%s%s'\n"),
-                 argv[0], prefix, d->__nextchar);
-
-      d->__nextchar = NULL;
-      d->optind++;
-      d->optopt = 0;
-      return '?';
+      if (d->optind < argc)
+       d->optarg = argv[d->optind++];
+      else
+       {
+         if (print_errors)
+           fprintf (stderr,
+                    _("%s: option '%s%s' requires an argument\n"),
+                    argv[0], prefix, pfound->name);
+
+         d->optopt = pfound->val;
+         return optstring[0] == ':' ? ':' : '?';
+       }
     }
 
-  /* Otherwise interpret it as a short option.  */
-  return -1;
+  if (longind != NULL)
+    *longind = option_index;
+  if (pfound->flag)
+    {
+      *(pfound->flag) = pfound->val;
+      return 0;
+    }
+  return pfound->val;
 }
 
 /* Initialize internal data upon the first call to getopt.  */