[Qemu-devel] [PATCH v2] qemu-io: Reinitialize optind to 1 (not 0) before parsing inner command.

[Qemu-devel] [PATCH v2] qemu-io: Reinitialize optind to 1 (not 0) before parsing inner command.

[Richard W.M. Jones]

v1 was here:
https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg06237.html
https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg00133.html

v2:

 - Simplify so we just set optind = 1.

 - Retest it on Linux and FreeBSD. 

Rich.

[Qemu-devel] [PATCH v2] qemu-io: Reinitialize optind to 1 (not 0) before parsing inner command.

[Richard W.M. Jones]

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(-)

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);
 }
 
-- 
2.19.2

Re: [Qemu-devel] [PATCH v2] qemu-io: Reinitialize optind to 1 (not 0) before parsing inner command.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2031 bytes --]

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>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2] qemu-io: Reinitialize optind to 1 (not 0) before parsing inner command.

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1630 bytes --]

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);
>  }
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2] qemu-io: Reinitialize optind to 1 (not 0) before parsing inner command.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2660 bytes --]

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)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2] qemu-io: Reinitialize optind to 1 (not 0) before parsing inner command.

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3029 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2] qemu-io: Reinitialize optind to 1 (not 0) before parsing inner command.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2448 bytes --]

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).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2] qemu-io: Reinitialize optind to 1 (not 0) before parsing inner command.

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3723 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2] qemu-io: Reinitialize optind to 1 (not 0) before parsing inner command.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 4385 bytes --]

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?").


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2] qemu-io: Reinitialize optind to 1 (not 0) before parsing inner command.

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 5397 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2] qemu-io: Reinitialize optind to 1 (not 0) before parsing inner command.

[Richard W.M. Jones]

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.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [Qemu-devel] [PATCH v2] qemu-io: Reinitialize optind to 1 (not 0) before parsing inner command.

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH v2] qemu-io: Reinitialize optind to 1 (not 0) before parsing inner command.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1271 bytes --]

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2] qemu-io: Reinitialize optind to 1 (not 0) before parsing inner command.

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1735 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [PATCH v2] qemu-io: Reinitialize optind to 1 (not 0) before parsing inner command.

[Richard W.M. Jones]

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

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [Qemu-devel] [PATCH v2] qemu-io: Reinitialize optind to 1 (not 0) before parsing inner command.

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1480 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]