| Message ID | 20190103094728.31747-2-rjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] qemu-io: Reinitialize optind to 1 (not 0) before parsing inner command. | expand |
On 1/3/19 3:47 AM, Richard W.M. Jones wrote: > On FreeBSD 11.2: > > $ nbdkit memory size=1M --run './qemu-io -f raw -c "aio_write 0 512" $nbd' > Parsing error: non-numeric argument, or extraneous/unrecognized suffix -- aio_write > > After main option parsing, we reinitialize optind so we can parse each > command. However reinitializing optind to 0 does not work on FreeBSD. > What happens when you do this is optind remains 0 after the option > parsing loop, and the result is we try to parse argv[optind] == > argv[0] == "aio_write" as if it was the first parameter. > > The FreeBSD manual page says: > > In order to use getopt() to evaluate multiple sets of arguments, or to > evaluate a single set of arguments multiple times, the variable optreset > must be set to 1 before the second and each additional set of calls to > getopt(), and the variable optind must be reinitialized. > > (From the rest of the man page it is clear that optind must be > reinitialized to 1). > > The glibc man page says: > > A program that scans multiple argument vectors, or rescans the same > vector more than once, and wants to make use of GNU extensions such as > '+' and '-' at the start of optstring, or changes the value of > POSIXLY_CORRECT between scans, must reinitialize getopt() by resetting > optind to 0, rather than the traditional value of 1. (Resetting to 0 > forces the invocation of an internal initialization routine that > rechecks POSIXLY_CORRECT and checks for GNU extensions in optstring.) > > Note I didn't set optreset. It's not present in glibc and the "hard > reset" is not necessary in this context. > > Signed-off-by: Richard W.M. Jones <rjones@redhat.com> > --- > qemu-io-cmds.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Eric Blake <eblake@redhat.com>
On 03.01.19 10:47, Richard W.M. Jones wrote: > On FreeBSD 11.2: > > $ nbdkit memory size=1M --run './qemu-io -f raw -c "aio_write 0 512" $nbd' > Parsing error: non-numeric argument, or extraneous/unrecognized suffix -- aio_write > > After main option parsing, we reinitialize optind so we can parse each > command. However reinitializing optind to 0 does not work on FreeBSD. > What happens when you do this is optind remains 0 after the option > parsing loop, and the result is we try to parse argv[optind] == > argv[0] == "aio_write" as if it was the first parameter. > > The FreeBSD manual page says: > > In order to use getopt() to evaluate multiple sets of arguments, or to > evaluate a single set of arguments multiple times, the variable optreset > must be set to 1 before the second and each additional set of calls to > getopt(), and the variable optind must be reinitialized. [...] > Note I didn't set optreset. It's not present in glibc and the "hard > reset" is not necessary in this context. But it sure sounds like FreeBSD requires you to set it, doesn't it? Max > Signed-off-by: Richard W.M. Jones <rjones@redhat.com> > --- > qemu-io-cmds.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c > index 2c39124036..aa10ed5a20 100644 > --- a/qemu-io-cmds.c > +++ b/qemu-io-cmds.c > @@ -114,7 +114,7 @@ static int command(BlockBackend *blk, const cmdinfo_t *ct, int argc, > } > } > > - optind = 0; > + optind = 1; > return ct->cfunc(blk, argc, argv); > } > >
On 1/7/19 11:17 AM, Max Reitz wrote: > On 03.01.19 10:47, Richard W.M. Jones wrote: >> On FreeBSD 11.2: >> >> $ nbdkit memory size=1M --run './qemu-io -f raw -c "aio_write 0 512" $nbd' >> Parsing error: non-numeric argument, or extraneous/unrecognized suffix -- aio_write >> >> After main option parsing, we reinitialize optind so we can parse each >> command. However reinitializing optind to 0 does not work on FreeBSD. >> What happens when you do this is optind remains 0 after the option >> parsing loop, and the result is we try to parse argv[optind] == >> argv[0] == "aio_write" as if it was the first parameter. >> >> The FreeBSD manual page says: >> >> In order to use getopt() to evaluate multiple sets of arguments, or to >> evaluate a single set of arguments multiple times, the variable optreset >> must be set to 1 before the second and each additional set of calls to >> getopt(), and the variable optind must be reinitialized. > > [...] > >> Note I didn't set optreset. It's not present in glibc and the "hard >> reset" is not necessary in this context. > > But it sure sounds like FreeBSD requires you to set it, doesn't it? The reason BSD and glibc have a hard reset path is because of hidden state - both BSD and glibc track state that remembers if the options began with '+' or '-' (both of those are extensions beyond POSIX), and whether POSIXLY_CORRECT was set. Beyond that hidden state is a corner case of one more piece of state that you can trigger using only POSIX: if the user passes './prog -ab' while you had code: swich (getopt(argc, argv, "ab")) { case 'a': optind = 1; ... then things fall apart for both BSD and glibc, because getopt() has to track invisible state in order to remember that the next call will process the -b portion of the merged short-option in argv[optind==1] rather than repeating the -a half and before moving on to optind==2. But this latter corner case can only happen when getopt() did not return -1. At the end of the day, both GNU optind=0 and BSD optreset=1 are sufficient to force a hard reset of all hidden state. But if you don't use POSIX extensions, and always run getopt() until a -1 return, then setting optind=1 is a portable soft reset, regardless of how the hidden state is implemented, and regardless of how (or even if) libc offers a hard reset, even though POSIX itself is currently lacking that mention. (I should probably file a POSIX defect to get that wording listed in POSIX)
On 07.01.19 18:46, Eric Blake wrote: > On 1/7/19 11:17 AM, Max Reitz wrote: >> On 03.01.19 10:47, Richard W.M. Jones wrote: >>> On FreeBSD 11.2: >>> >>> $ nbdkit memory size=1M --run './qemu-io -f raw -c "aio_write 0 512" $nbd' >>> Parsing error: non-numeric argument, or extraneous/unrecognized suffix -- aio_write >>> >>> After main option parsing, we reinitialize optind so we can parse each >>> command. However reinitializing optind to 0 does not work on FreeBSD. >>> What happens when you do this is optind remains 0 after the option >>> parsing loop, and the result is we try to parse argv[optind] == >>> argv[0] == "aio_write" as if it was the first parameter. >>> >>> The FreeBSD manual page says: >>> >>> In order to use getopt() to evaluate multiple sets of arguments, or to >>> evaluate a single set of arguments multiple times, the variable optreset >>> must be set to 1 before the second and each additional set of calls to >>> getopt(), and the variable optind must be reinitialized. >> >> [...] >> >>> Note I didn't set optreset. It's not present in glibc and the "hard >>> reset" is not necessary in this context. >> >> But it sure sounds like FreeBSD requires you to set it, doesn't it? > > The reason BSD and glibc have a hard reset path is because of hidden > state - both BSD and glibc track state that remembers if the options > began with '+' or '-' (both of those are extensions beyond POSIX), and > whether POSIXLY_CORRECT was set. Beyond that hidden state is a corner > case of one more piece of state that you can trigger using only POSIX: > if the user passes './prog -ab' while you had code: > > swich (getopt(argc, argv, "ab")) { > case 'a': optind = 1; ... > > then things fall apart for both BSD and glibc, because getopt() has to > track invisible state in order to remember that the next call will > process the -b portion of the merged short-option in argv[optind==1] > rather than repeating the -a half and before moving on to optind==2. > But this latter corner case can only happen when getopt() did not return -1. > > At the end of the day, both GNU optind=0 and BSD optreset=1 are > sufficient to force a hard reset of all hidden state. But if you don't > use POSIX extensions, and always run getopt() until a -1 return, then > setting optind=1 is a portable soft reset, regardless of how the hidden > state is implemented, and regardless of how (or even if) libc offers a > hard reset, even though POSIX itself is currently lacking that mention. > (I should probably file a POSIX defect to get that wording listed in POSIX) Hm, OK? Is there any guarantee for that behavior for FreeBSD, or is that just how it is? Because the man page is very clear on it: "optreset must be set to 1". It doesn't talk about soft or hard resets like the glibc man page does. And if optreset not being available for glibc is the only issue, I'd say adding it as a weak global variable would work without #ifdefs. Max
On 1/7/19 11:50 AM, Max Reitz wrote: >>>> Note I didn't set optreset. It's not present in glibc and the "hard >>>> reset" is not necessary in this context. >>> >>> But it sure sounds like FreeBSD requires you to set it, doesn't it? No. Quoting https://www.freebsd.org/cgi/man.cgi?getopt(3) The variables opterr and optind are both initialized to 1. The optind variable may be set to another value before a set of calls to getopt() in order to skip over more or less argv entries. so resetting it to 1 as a soft reset is no different to setting it to 2 to skip argv[1]. It just means that you didn't get the hard reset of internal state (there is definitely internal state if argv[1] was merged short options - but that state is cleared if you run getopt() until it returns -1; there may also be internal state if you used extensions, but when you don't use extensions, such internal state is irrelevant). I think the BSD man page needs updating, and that will probably happen if I file my promised POSIX defect. >> At the end of the day, both GNU optind=0 and BSD optreset=1 are >> sufficient to force a hard reset of all hidden state. But if you don't >> use POSIX extensions, and always run getopt() until a -1 return, then >> setting optind=1 is a portable soft reset, regardless of how the hidden >> state is implemented, and regardless of how (or even if) libc offers a >> hard reset, even though POSIX itself is currently lacking that mention. >> (I should probably file a POSIX defect to get that wording listed in POSIX) > > Hm, OK? Is there any guarantee for that behavior for FreeBSD, or is > that just how it is? Because the man page is very clear on it: > "optreset must be set to 1". It doesn't talk about soft or hard resets > like the glibc man page does. > > And if optreset not being available for glibc is the only issue, I'd say > adding it as a weak global variable would work without #ifdefs. I don't see the point - Richard has already tested that optind = 1 worked on BSD machines for our purposes, so we don't have to worry about the hard reset aspect of optreset=1. (But yes, it would also be nice if BSD and glibc folks could agree on how to do hard resets, instead of having two different incompatible ways).
On 07.01.19 18:59, Eric Blake wrote: > On 1/7/19 11:50 AM, Max Reitz wrote: > >>>>> Note I didn't set optreset. It's not present in glibc and the "hard >>>>> reset" is not necessary in this context. >>>> >>>> But it sure sounds like FreeBSD requires you to set it, doesn't it? > > No. Quoting https://www.freebsd.org/cgi/man.cgi?getopt(3) > > The variables opterr and optind are both initialized to 1. The optind > variable may be set to another value before a set of calls to > getopt() in > order to skip over more or less argv entries. > > so resetting it to 1 as a soft reset is no different to setting it to 2 > to skip argv[1]. In theory it is very much different because the text clearly says "in order to skip", not "in order to re-parse or use a different argv". Especially the fact that we use different argvs is something that implementations may not expect. > It just means that you didn't get the hard reset of > internal state (there is definitely internal state if argv[1] was merged > short options - but that state is cleared if you run getopt() until it > returns -1; While I agree that this is probably the case in practice, I see no reason why this should be the case if it isn't guaranteed. Why should the state be cleared once you reach the end? > there may also be internal state if you used extensions, but > when you don't use extensions, such internal state is irrelevant). > > I think the BSD man page needs updating, and that will probably happen > if I file my promised POSIX defect. Sure. But as it is, it doesn't tell me that resetting optind to 1 is sufficient to be able to parse a new argv. >>> At the end of the day, both GNU optind=0 and BSD optreset=1 are >>> sufficient to force a hard reset of all hidden state. But if you don't >>> use POSIX extensions, and always run getopt() until a -1 return, then >>> setting optind=1 is a portable soft reset, regardless of how the hidden >>> state is implemented, and regardless of how (or even if) libc offers a >>> hard reset, even though POSIX itself is currently lacking that mention. >>> (I should probably file a POSIX defect to get that wording listed in POSIX) >> >> Hm, OK? Is there any guarantee for that behavior for FreeBSD, or is >> that just how it is? Because the man page is very clear on it: >> "optreset must be set to 1". It doesn't talk about soft or hard resets >> like the glibc man page does. >> >> And if optreset not being available for glibc is the only issue, I'd say >> adding it as a weak global variable would work without #ifdefs. > > I don't see the point - Richard has already tested that optind = 1 > worked on BSD machines for our purposes, so we don't have to worry about > the hard reset aspect of optreset=1. Well, and as far as I remember glibc's memcpy() at one point only copied in one direction and things broke badly once they reversed it at some point for some CPUs. Just because it works now doesn't mean it will work always if the specification allows for different behavior. > (But yes, it would also be nice if > BSD and glibc folks could agree on how to do hard resets, instead of > having two different incompatible ways) I don't see why we should have a general code path if there is no standard way of resetting getopt() other than "This seems to work". What's so bad about a weak optreset or an "#ifdef __FreeBSD__; optreset = 1; #endif"? Sure, if you can get POSIX to define the fact that optind = 1 after getopt() == -1 will be sufficient to start parsing a new argv, that'd be great. But there is no such standard yet (other than "Why would that not work?"). Max
On Mon, Jan 07, 2019 at 06:50:53PM +0100, Max Reitz wrote: [...] I don't particularly care how we fix this, but it breaks the nbdkit tests on FreeBSD so I am keen to fix it one way or another. > And if optreset not being available for glibc is the only issue, I'd say > adding it as a weak global variable would work without #ifdefs. The weak global variable doesn't make the code "#ifdef free". I tried a patch like this: +int optreset __attribute__((weak)); ... static int command(...) { ... optind = 0; + optreset = 1; ... } but that still doesn't work on FreeBSD. You have to set optind=1 apparently. So if we want to set optreset=1 we still end up with #ifdef __FreeBSD__. The final patch will end up looking something like: static int command(...) { ... +#ifdef __FreeBSD__ + optind = 1; + optreset = 1; +#else optind = 0; +#endif ... } If you want me to submit a formal patch like this let me know. Rich.
On 1/7/19 12:14 PM, Max Reitz wrote: > On 07.01.19 18:59, Eric Blake wrote: >> On 1/7/19 11:50 AM, Max Reitz wrote: >> >>>>>> Note I didn't set optreset. It's not present in glibc and the "hard >>>>>> reset" is not necessary in this context. >>>>> >>>>> But it sure sounds like FreeBSD requires you to set it, doesn't it? >> >> No. Quoting https://www.freebsd.org/cgi/man.cgi?getopt(3) >> >> The variables opterr and optind are both initialized to 1. The optind >> variable may be set to another value before a set of calls to >> getopt() in >> order to skip over more or less argv entries. >> >> so resetting it to 1 as a soft reset is no different to setting it to 2 >> to skip argv[1]. > > In theory it is very much different because the text clearly says "in > order to skip", not "in order to re-parse or use a different argv". > Especially the fact that we use different argvs is something that > implementations may not expect. Consider the following input: ./prog -ab -cd -ef against while ((opt = getopt(argc, argv, "abcdef")) != -1) { switch (opt) { case 'a': case 'b': case 'f': break; case 'c': optind = 3; break; case 'd': case 'e': abort(); } } What does that do on BSD? On glibc, after the third call, optind is still 2 (but I hard-set it to 3), then the fourth call returns 'd' and increments optind to 4, before abort()ing, never reaching 'e' or 'f'. But if BSD goes from 'c' to 'f' and skips 'd' and 'e', it is because BSD tracks internal state differently from glibc. Either way, the fact that setting optind = 3 does NOT make glibc return 'e' or 'f' means that I did NOT skip ahead to argument 3 (glibc still returned 'd' then skipped to argument 4; either BSD does the same, or BSD skips to 'f'), and thus I can argue that the BSD man page is incomplete, and SHOULD be corrected to mention that assigning to optind to skip to a future argument is safe ONLY when the hidden state is not affected by being mid-parse of merged short options. But how do you get out of the hidden state of merged short options? By parsing until getopt() returns -1. And once you've reached that point, then hidden state is clear, and skipping backwards is just as reasonable as skipping forwards. >> I think the BSD man page needs updating, and that will probably happen >> if I file my promised POSIX defect. > > Sure. But as it is, it doesn't tell me that resetting optind to 1 is > sufficient to be able to parse a new argv. But arguing that something that worked for Richard's testing is wrong, without reading the BSD source code, isn't going to help us either. >> I don't see the point - Richard has already tested that optind = 1 >> worked on BSD machines for our purposes, so we don't have to worry about >> the hard reset aspect of optreset=1. > > Well, and as far as I remember glibc's memcpy() at one point only copied > in one direction and things broke badly once they reversed it at some > point for some CPUs. That was because of buggy software that didn't read the function contracts, and should have been using memmove( insta > > Just because it works now doesn't mean it will work always if the > specification allows for different behavior. Yes, but that's why I need to file a POSIX defect, so that BSD won't change their current behavior because POSIX will require the soft reset behavior. Here's the current thread on the POSIX list: https://www.mail-archive.com/austin-group-l@opengroup.org/msg03210.html which I hope to turn into a formal defect soon. > >> (But yes, it would also be nice if >> BSD and glibc folks could agree on how to do hard resets, instead of >> having two different incompatible ways) > I don't see why we should have a general code path if there is no > standard way of resetting getopt() other than "This seems to work". > What's so bad about a weak optreset or an > "#ifdef __FreeBSD__; optreset = 1; #endif"? > > Sure, if you can get POSIX to define the fact that optind = 1 after > getopt() == -1 will be sufficient to start parsing a new argv, that'd be > great. But there is no such standard yet (other than "Why would that > not work?").
Am 07.01.2019 um 19:40 hat Richard W.M. Jones geschrieben: > On Mon, Jan 07, 2019 at 06:50:53PM +0100, Max Reitz wrote: > [...] > > I don't particularly care how we fix this, but it breaks the nbdkit > tests on FreeBSD so I am keen to fix it one way or another. > > > And if optreset not being available for glibc is the only issue, I'd say > > adding it as a weak global variable would work without #ifdefs. > > The weak global variable doesn't make the code "#ifdef free". I tried > a patch like this: > > +int optreset __attribute__((weak)); > > ... > > static int command(...) > { > ... > optind = 0; > + optreset = 1; > ... > } > > but that still doesn't work on FreeBSD. > > You have to set optind=1 apparently. So if we want to set optreset=1 > we still end up with #ifdef __FreeBSD__. The final patch will > end up looking something like: > > static int command(...) > { > ... > +#ifdef __FreeBSD__ > + optind = 1; > + optreset = 1; > +#else > optind = 0; > +#endif > ... > } Unconditionally setting optind = 1 looks fine. I would, however, quote a different part of the glibc man page (in addition or instead of the paragraph you already quoted): The variable optind is the index of the next element to be processed in argv. The system initializes this value to 1. The caller can reset it to 1 to restart scanning of the same argv, or when scanning a new argument vector. This makes it pretty clear that optind = 1 is fine for our case with glibc. The FreeBSD man page still suggests that we need optreset = 1, so I suppose we'd end up with something like: ... optind = 1; #ifdef __FreeBSD__ optreset = 1; #endif ... I think I slightly prefer the #ifdef to a weak variable, but that's another option. Kevin
On 1/8/19 6:16 AM, Kevin Wolf wrote: > Unconditionally setting optind = 1 looks fine. I would, however, quote a > different part of the glibc man page (in addition or instead of the > paragraph you already quoted): > > The variable optind is the index of the next element to be > processed in argv. The system initializes this value to 1. The > caller can reset it to 1 to restart scanning of the same argv, or > when scanning a new argument vector. > > This makes it pretty clear that optind = 1 is fine for our case with > glibc. The FreeBSD man page still suggests that we need optreset = 1, so > I suppose we'd end up with something like: > > ... > optind = 1; > #ifdef __FreeBSD__ > optreset = 1; > #endif If you really want to set optreset for BSD systems, I'd do a configure probe for whether optreset exists, and if so set it for ALL platforms that have optreset, not just for __FreeBSD__. (That, and checkpatch.pl will gripe if you don't do it that way). But I'm leaning towards not bothering with optreset UNLESS someone proves they have a case where it actually matters.
Am 08.01.2019 um 15:51 hat Eric Blake geschrieben: > On 1/8/19 6:16 AM, Kevin Wolf wrote: > > Unconditionally setting optind = 1 looks fine. I would, however, quote a > > different part of the glibc man page (in addition or instead of the > > paragraph you already quoted): > > > > The variable optind is the index of the next element to be > > processed in argv. The system initializes this value to 1. The > > caller can reset it to 1 to restart scanning of the same argv, or > > when scanning a new argument vector. > > > > This makes it pretty clear that optind = 1 is fine for our case with > > glibc. The FreeBSD man page still suggests that we need optreset = 1, so > > I suppose we'd end up with something like: > > > > ... > > optind = 1; > > #ifdef __FreeBSD__ > > optreset = 1; > > #endif > > If you really want to set optreset for BSD systems, I'd do a configure > probe for whether optreset exists, and if so set it for ALL platforms > that have optreset, not just for __FreeBSD__. (That, and checkpatch.pl > will gripe if you don't do it that way). > > But I'm leaning towards not bothering with optreset UNLESS someone > proves they have a case where it actually matters. I don't mind either way as long as it works. Using optreset would be following the spec by the letter. In fact, I had already applied this patch because it's correct even if possibly incomplete, depending on your interpretation, but I decided to reply instead because the commit message didn't really describe that optreset = 1 is correct for glibc, but that optreset = 0 is necessary in some other case (which is irrelevant here). So the commit message was my main point. Kevin
On Tue, Jan 08, 2019 at 04:13:09PM +0100, Kevin Wolf wrote: > Am 08.01.2019 um 15:51 hat Eric Blake geschrieben: > > On 1/8/19 6:16 AM, Kevin Wolf wrote: > > > Unconditionally setting optind = 1 looks fine. I would, however, quote a > > > different part of the glibc man page (in addition or instead of the > > > paragraph you already quoted): > > > > > > The variable optind is the index of the next element to be > > > processed in argv. The system initializes this value to 1. The > > > caller can reset it to 1 to restart scanning of the same argv, or > > > when scanning a new argument vector. > > > > > > This makes it pretty clear that optind = 1 is fine for our case with > > > glibc. The FreeBSD man page still suggests that we need optreset = 1, so > > > I suppose we'd end up with something like: > > > > > > ... > > > optind = 1; > > > #ifdef __FreeBSD__ > > > optreset = 1; > > > #endif > > > > If you really want to set optreset for BSD systems, I'd do a configure > > probe for whether optreset exists, and if so set it for ALL platforms > > that have optreset, not just for __FreeBSD__. (That, and checkpatch.pl > > will gripe if you don't do it that way). > > > > But I'm leaning towards not bothering with optreset UNLESS someone > > proves they have a case where it actually matters. > > I don't mind either way as long as it works. Using optreset would be > following the spec by the letter. > > In fact, I had already applied this patch because it's correct even if > possibly incomplete, depending on your interpretation, but I decided to > reply instead because the commit message didn't really describe that > optreset = 1 is correct for glibc, but that optreset = 0 is necessary in > some other case (which is irrelevant here). So the commit message was my > main point. I have tested it[1] on: Linux, glibc 2.28 FreeBSD 11.2-RELEASE OpenBSD 6.4 Interestingly OpenBSD getopt works in this respect the same way as Linux, so it doesn't need any fix. It's only FreeBSD which needs the fix. Rich. [1] "it" being v2 here: https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg00189.html
On 07.01.19 19:45, Eric Blake wrote: > On 1/7/19 12:14 PM, Max Reitz wrote: >> On 07.01.19 18:59, Eric Blake wrote: >>> On 1/7/19 11:50 AM, Max Reitz wrote: >>> >>>>>>> Note I didn't set optreset. It's not present in glibc and the "hard >>>>>>> reset" is not necessary in this context. >>>>>> >>>>>> But it sure sounds like FreeBSD requires you to set it, doesn't it? >>> >>> No. Quoting https://www.freebsd.org/cgi/man.cgi?getopt(3) >>> >>> The variables opterr and optind are both initialized to 1. The optind >>> variable may be set to another value before a set of calls to >>> getopt() in >>> order to skip over more or less argv entries. >>> >>> so resetting it to 1 as a soft reset is no different to setting it to 2 >>> to skip argv[1]. >> >> In theory it is very much different because the text clearly says "in >> order to skip", not "in order to re-parse or use a different argv". >> Especially the fact that we use different argvs is something that >> implementations may not expect. > > Consider the following input: > > ./prog -ab -cd -ef > > against > > while ((opt = getopt(argc, argv, "abcdef")) != -1) { > switch (opt) { > case 'a': case 'b': case 'f': break; > case 'c': optind = 3; break; > case 'd': case 'e': abort(); > } > } > > What does that do on BSD? On glibc, after the third call, optind is > still 2 (but I hard-set it to 3), then the fourth call returns 'd' and > increments optind to 4, before abort()ing, never reaching 'e' or 'f'. > But if BSD goes from 'c' to 'f' and skips 'd' and 'e', it is because BSD > tracks internal state differently from glibc. Either way, the fact that > setting optind = 3 does NOT make glibc return 'e' or 'f' means that I > did NOT skip ahead to argument 3 (glibc still returned 'd' then skipped > to argument 4; either BSD does the same, or BSD skips to 'f'), and thus > I can argue that the BSD man page is incomplete, and SHOULD be corrected > to mention that assigning to optind to skip to a future argument is safe > ONLY when the hidden state is not affected by being mid-parse of merged > short options. But how do you get out of the hidden state of merged > short options? By parsing until getopt() returns -1. And once you've > reached that point, then hidden state is clear, and skipping backwards > is just as reasonable as skipping forwards. Is it? Why wouldn't FreeBSD just set optind to -1 at that point and just have an "if (optind < 0) { return -1; }" at the beginning of getopt(), without having cleared the internal state? I agree that the variable _sp you define below should be reset, because that is the sane thing to do whenever you jump to a new index in argv[] (i.e. a new optind). But that doesn't guarantee to me that there isn't additional state that may not be cleared. No, I cannot imagine why there should be such additional state. I'm just arguing based on the interface description, i.e. the man page. >>> I think the BSD man page needs updating, and that will probably happen >>> if I file my promised POSIX defect. >> >> Sure. But as it is, it doesn't tell me that resetting optind to 1 is >> sufficient to be able to parse a new argv. > > But arguing that something that worked for Richard's testing is wrong, > without reading the BSD source code, isn't going to help us either. Come on, it's not like I'm demanding something impossible when I'm asking for a weak global optreset. ;-) >>> I don't see the point - Richard has already tested that optind = 1 >>> worked on BSD machines for our purposes, so we don't have to worry about >>> the hard reset aspect of optreset=1. >> >> Well, and as far as I remember glibc's memcpy() at one point only copied >> in one direction and things broke badly once they reversed it at some >> point for some CPUs. > > That was because of buggy software that didn't read the function > contracts, and should have been using memmove( insta Well, and right now this here is a patch that does not conform to the function contract and that should use optreset = 1 in addition. You're arguing with "Why would it not work?". And I just feel like you could have argued the same for memcpy(), "Why would it copy backwards?". Please don't take this as an accusation, but I feel like you are trying to make a point of how the standard should be. I can fully understand that, but I personally don't feel like qemu is the place to make such a point (until the standard truly has been fixed). Sure, this is a minor issue and I don't mind if someone else (i.e. Kevin) merges it. But maybe I want to make a point, too, which is to take interface descriptions as they are, not as they should be. Making the interface more nice for all user software is distinct from making our user software just assume it is nice already. >> Just because it works now doesn't mean it will work always if the >> specification allows for different behavior. > > Yes, but that's why I need to file a POSIX defect, so that BSD won't > change their current behavior because POSIX will require the soft reset > behavior. > > Here's the current thread on the POSIX list: > https://www.mail-archive.com/austin-group-l@opengroup.org/msg03210.html > > which I hope to turn into a formal defect soon. Thanks! Max
On 08.01.19 15:51, Eric Blake wrote: > On 1/8/19 6:16 AM, Kevin Wolf wrote: > > >> Unconditionally setting optind = 1 looks fine. I would, however, quote a >> different part of the glibc man page (in addition or instead of the >> paragraph you already quoted): >> >> The variable optind is the index of the next element to be >> processed in argv. The system initializes this value to 1. The >> caller can reset it to 1 to restart scanning of the same argv, or >> when scanning a new argument vector. >> >> This makes it pretty clear that optind = 1 is fine for our case with >> glibc. The FreeBSD man page still suggests that we need optreset = 1, so >> I suppose we'd end up with something like: >> >> ... >> optind = 1; >> #ifdef __FreeBSD__ >> optreset = 1; >> #endif > > If you really want to set optreset for BSD systems, I'd do a configure > probe for whether optreset exists, and if so set it for ALL platforms > that have optreset, not just for __FreeBSD__. (That, and checkpatch.pl > will gripe if you don't do it that way). ...or you just make it a weak variable... > But I'm leaning towards not bothering with optreset UNLESS someone > proves they have a case where it actually matters. I don't care in this case, but this is not a good argument. As I said before, in general, if an interface description says to do X for Y you cannot rely on doing Y without X just because it works right now. Max
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c index 2c39124036..aa10ed5a20 100644 --- a/qemu-io-cmds.c +++ b/qemu-io-cmds.c @@ -114,7 +114,7 @@ static int command(BlockBackend *blk, const cmdinfo_t *ct, int argc, } } - optind = 0; + optind = 1; return ct->cfunc(blk, argc, argv); }
On FreeBSD 11.2: $ nbdkit memory size=1M --run './qemu-io -f raw -c "aio_write 0 512" $nbd' Parsing error: non-numeric argument, or extraneous/unrecognized suffix -- aio_write After main option parsing, we reinitialize optind so we can parse each command. However reinitializing optind to 0 does not work on FreeBSD. What happens when you do this is optind remains 0 after the option parsing loop, and the result is we try to parse argv[optind] == argv[0] == "aio_write" as if it was the first parameter. The FreeBSD manual page says: In order to use getopt() to evaluate multiple sets of arguments, or to evaluate a single set of arguments multiple times, the variable optreset must be set to 1 before the second and each additional set of calls to getopt(), and the variable optind must be reinitialized. (From the rest of the man page it is clear that optind must be reinitialized to 1). The glibc man page says: A program that scans multiple argument vectors, or rescans the same vector more than once, and wants to make use of GNU extensions such as '+' and '-' at the start of optstring, or changes the value of POSIXLY_CORRECT between scans, must reinitialize getopt() by resetting optind to 0, rather than the traditional value of 1. (Resetting to 0 forces the invocation of an internal initialization routine that rechecks POSIXLY_CORRECT and checks for GNU extensions in optstring.) Note I didn't set optreset. It's not present in glibc and the "hard reset" is not necessary in this context. Signed-off-by: Richard W.M. Jones <rjones@redhat.com> --- qemu-io-cmds.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)