Re: [help-gengetopt] make cmd_line_list and cmd_line_list_tmp static?

[Andre Noll]

On 11:37, Lorenzo Bettini wrote:
> thanks for the file, I guess I have the problem clear now.
> 
> I think the problem is a sort of design problem of the getopt 
> implementation itself, or, at least, the implementation is correct but 
> under the (quite strong) condition that if you call one of the functions 
> twice you have to use the same arguments (I mean the same argv).

Exactly.  There are several workarounds available, but these are
all glibc-specific.

> I'd like to post a bug report at the glibc bugzilla about this, but 
> first I'd like to have a confirmation from you about the problem (by the 
> way could you please send me, in private, all the original sources of 
> glibc of getopt stuff you extracted this file from?).

The original sources are

        getopt.c, getopt.h, and getopt1.c

from the "posix" directory of the glibc source tree, version
2.3.6. I'll send them to you in private, but you may as well just
download the glibc tarball.

> >struct getopt_data {
> >     /*
> >      * These have exactly the same meaning as the corresponding global 
> >      variables,
> >      * except that they are used for the reentrant versions of getopt.
> >      */
> >     int optind;
> >     int opterr;
> >     int optopt;
> >     char *optarg;
> >
> >     /* True if the internal members have been initialized.  */
> >     int initialized;
> >
> >     /*
> >      * The next char to be scanned in the option-element in which the 
> >      last option
> >      * character we returned was found.  This allows us to pick up the 
> >      scan where
> >      * we left off.  If this is zero, or a null string, it means resume 
> >      the scan by
> >      * advancing to the next ARGV-element.
> >      */
> >     char *nextchar;
> >
> >     /*
> >      * Describe the part of ARGV that contains non-options that have 
> >      been skipped.
> >      * `first_nonopt' is the index in ARGV of the first of them; 
> >      `last_nonopt' is
> >      * the index after the last of them.
> >      */
> >     int first_nonopt;
> >     int last_nonopt;
> >};
> 
> so internally this struct is used, instead of the global variables, and 
> the problem, I think, is in the last three fields...

yes, in particular "nextchar" is problematic.

> > * 1003.2 says this must be 1 before any call.
> > */
> >int optind = 1;
> 
> this is the confirmation that the standard says that it must be 1, but 
> they use the 0 value for initializations...

Yeah, but it is still unclear to me if the application has to set this as
well. Unfortunately, the initialization feature is not mentioned in posix.

> >static void exchange(char **argv, struct getopt_data *d)
> >{
> >     int bottom = d->first_nonopt;
> >     int middle = d->last_nonopt;
> >     int top = d->optind;
> >     char *tem;
> >
> 
> <snip>
> this function exchanges elements of argv according to the values 
> d->first_nonopt and d->last_nonopt... so their values must be correct 
> otherwise we spoil memory...

Yes, it pretty much assumes that the argv array stays the same
between the individual calls.

> >static int getopt_internal_r(int argc, char *const *argv, const char 
> >*optstring,
> >             const struct option *longopts, int *longind,
> >             struct getopt_data *d)
> >{
> >     int ret, print_errors = d->opterr;
> >
> >     if (optstring[0] == ':')
> >             print_errors = 0;
> >     if (argc < 1)
> >             return -1;
> >     d->optarg = NULL;
> >     if (d->optind == 0 || !d->initialized) {
> >             if (d->optind == 0)
> >                     d->optind = 1;  /* Don't scan ARGV[0], the program 
> >                     name.  */
> >             getopt_initialize(d);
> >     }
> >     if (d->nextchar == NULL || *d->nextchar == '\0') {
> >             ret = shuffle_argv(argc, argv, longopts, d);
> >             if (ret)
> >                     return ret;
> >     }
> 
> this is the point where the struct is initialized, i.e., only when 
> d->optind == 0 (or !d->initialized)

And it is also the point where valgrind complains (I already posted
this part of the code in an earlier mail). The *d->nextchar == '\0'
is the "Invalid read of size 1"

> >static int getopt_internal(int argc, char *const *argv, const char 
> >*optstring,
> >     const struct option *longopts, int *longind)
> >{
> >     int result;
> >     /* Keep a global copy of all internal members of d */
> >     static struct getopt_data d;
> >
> >     d.optind = optind;
> >     d.opterr = opterr;
> >     result = getopt_internal_r(argc, argv, optstring, longopts,
> >             longind, &d);
> >     optind = d.optind;
> >     optarg = d.optarg;
> >     optopt = d.optopt;
> >     return result;
> >}
> 
> the struct that is passed to getopt_internal_r is static, so its fields 
> are initialized to 0 only once: the first time getopt_internal_r is 
> called the struct is correctly initialized, the other times it is not 
> (unless you set optind to 0, which is not standard)

Exactly.

> what do you think?

I fully agree with your analysis. But I'm afraid the glibc people will
argue that their implementation is posix conform and that there are
(glibc-only) workarounds for our problem available.

So I think the best way to circumvent this is to further clean
up the getopt.c I've posted and to include it in gengetopt.

This not only avoids future compatiblility issues with other
implementations of getopt, but also gives us the freedom to change
things at will rather than having to rely on a standard which simply
doesn't cover things like multiple parsers or a non-constant argv.

> I'd really like to hear from glibc people to know whether this is the 
> intended behavior (which is not documented anyway)

It's sure worth to ask them to comment on that. But the glibc people
can't change posix..

Regards
Andre
-- 
The only person who always got his work done by Friday was Robinson Crusoe