Re: [Genparse-users] Simplifying command line parsing with Genparse

[<li...@mi...>]

On Thu, Jun 07, 2007 at 05:06:33PM +0200, Michael Geng wrote:
> On Thu, Jun 07, 2007 at 02:44:38PM +0200, Jim Meyering wrote:
> >   - one of the generated .c files I looked at calls strdup but doesn't
> >       check for a NULL return value or (less important) attempt to avoid
> >       the leak when the corresponding --backup=S option appears twice or more.
> 
> I guess Jim looked into the mv example 
> (http://genparse.sourceforge.net/examples/mv_clp.c). 
> 
> I propose not to allocate any memory dynamically in Genparse at all and use 
> the stack instead. malloc() and strdup() might not be fast enough in all 
> applications. For instance in the mv command I can hardly imagine that it 
> is acceptable to have a call to malloc() for allocating the arg_t struct 
> which would have to be executed with every call to mv no matter what 
> command line switches or arguments were specified. By the way, the result
> of this call to malloc() is also not checked for failure.
> 
> In principle the Coreutils implementation of the mv command does it
> similarly: The optind pointer which is returned by getopt_long() is first 
> stored in a variable on the heap and later the content of the string is
> copied to a string with strdup() which allocates from the heap. In order 
> to leave the freedom to do so to the user of Genparse it is required that 
> Genparse uses only the stack.

The problem with not using malloc() and strdup() is that this would break 
compatibility of Genparse. For this reason I suggest to add a new command
line switch -s/--noheap to Genparse. If this switch is not set (the default)
then Genparse will stay compatible to previous versions. If it is set then 
the parser interface will change:

-----------------------------

The compatible version of the generated parser of the mycopy3 example 
(--noheap not set) will look as follows. I marked the lines which 
have changed with '>'. 

struct arg_t *Cmdline(int argc, char *argv[])
{
  extern char *optarg;
  extern int optind;
  int c;
  struct arg_t *my_args;
  int errflg = 0;

  static struct option long_options[] =
  {
    {"iterations", 1, 0, 'i'},
    {"outfile", 1, 0, 'o'},
    {"longopt", 0, 0, 256},
    {"help", 0, 0, 'h'},
    {"version", 0, 0, 'v'},
    {0, 0, 0, 0}
  };

  my_args = (struct arg_t *) malloc (sizeof(struct arg_t));
> if (my_args == NULL)
>   {
>     fprintf(stderr, "error: out of memory\n");
>     return NULL;
>   }

  my_args->i = 1;
  my_args->o = NULL;
  my_args->longopt = false;
  my_args->h = false;
  my_args->v = false;

  optind = 0;
  while ((c = getopt_long(argc, argv, "i:o:hv", long_options, &optind)) != EOF)
    {
      switch(c)
        {
        case 'i': 
          my_args->i = atoi(optarg);
          if (my_args->i < 0)
            {
              fprintf(stderr, "parameter range error: i must be >= 0\n");
              errflg++;
            }
          break;

        case 'o': 
>         if (my_args->o != NULL)
>           free(my_args->o);
>         if ((my_args->o = strdup(optarg)) == NULL)
>           {
>             fprintf(stderr, "error: out of memory\n");
>             errflg++;
>           }
          break;

        case 256: 
          my_args->longopt = true;
          break;

        case 'h': 
          my_args->h = true;
          usage(argv[0]);
          break;

        case 'v': 
          my_args->v = true;
          break;

        default:
          usage(argv[0]);

        }
    } /* while */

  if (errflg)
    usage(argv[0]);

  if (optind >= argc)
    my_args->optind = 0;
  else
    my_args->optind = optind;
  return my_args;
}

-----------------------------

The new version which doesn't use malloc() and strdup() (--noheap set) 
will look as follows. 

void Cmdline(struct arg_t *my_args, int argc, char *argv[])
{
  extern char *optarg;
  extern int optind;
  int c;
  int errflg = 0;

  static struct option long_options[] =
  {
    {"iterations", 1, 0, 'i'},
    {"outfile", 1, 0, 'o'},
    {"longopt", 0, 0, 256},
    {"help", 0, 0, 'h'},
    {"version", 0, 0, 'v'},
    {0, 0, 0, 0}
  };

  my_args->i = 1;
  my_args->o = NULL;
  my_args->longopt = false;
  my_args->h = false;
  my_args->v = false;

  optind = 0;
  while ((c = getopt_long(argc, argv, "i:o:hv", long_options, &optind)) != EOF)
    {
      switch(c)
        {
        case 'i': 
          my_args->i = atoi(optarg);
          if (my_args->i < 0)
            {
              fprintf(stderr, "parameter range error: i must be >= 0\n");
              errflg++;
            }
          break;

        case 'o': 
>         my_args->o = optarg;
          break;

        case 256: 
          my_args->longopt = true;
          break;

        case 'h': 
          my_args->h = true;
          usage(argv[0]);
          break;

        case 'v': 
          my_args->v = true;
          break;

        default:
          usage(argv[0]);

        }
    } /* while */

  if (errflg)
    usage(argv[0]);

  if (optind >= argc)
    my_args->optind = 0;
  else
    my_args->optind = optind;
  return my_args;
}

-----------------------------

The latter version is clearly the most simple one. For this reason
I would recommend to always set the --noheap switch. I would also 
like to update all the examples including those in the info pages 
to use --noheap.

I found another issue with the present version of Genparse:
If a string has a default value then setting this parameter will always 
cause a memory leak because it is initialized with a strdup() result 
and when the parameter is actually processed it is overwritten with 
another strdup() result without freeing the old value. The same happens 
if you specify the same string parameter twice on the command line as 
Jim mentioned. Both cases will be solved in the code above no matter 
if Genparse is called with or without --noheap.

Michael