[Qemu-devel] [PATCH v4] qemu-io: Add generic function for reinitializing optind.

[Qemu-devel] [PATCH v4] qemu-io: Add generic function for reinitializing optind.

[Richard W.M. Jones]

v3 was posted here:
https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg01536.html

In v4:

 - Detect optreset at ./configure time and use ifdef HAVE_OPTRESET.

 - Adjusted comment style.

 - Checkpatch is now clean.

 - Retested on Linux, FreeBSD 11.2 and OpenBSD 6.4.

Rich.

[Qemu-devel] [PATCH v4] qemu-io: Add generic function for reinitializing optind.

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

This commit introduces an OS-portability function called
qemu_reset_optind which provides a way of resetting optind that works
on FreeBSD and platforms that use optreset, while keeping it the same
as now on other platforms.

Note that the qemu codebase sets optind in many other places, but in
those other places it's setting a local variable and not using getopt.
This change is only needed in places where we are using getopt and the
associated global variable optind.

Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
---
 configure            | 14 ++++++++++++++
 include/qemu/osdep.h | 16 ++++++++++++++++
 qemu-img.c           |  2 +-
 qemu-io-cmds.c       |  2 +-
 4 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/configure b/configure
index 3eee3fcf70..3d46df1517 100755
--- a/configure
+++ b/configure
@@ -4215,6 +4215,17 @@ if compile_prog "" "" ; then
   signalfd=yes
 fi
 
+# check if optreset global is declared by <getopt.h>
+optreset="no"
+cat > $TMPC << EOF
+#include <getopt.h>
+int main(void) { return optreset; }
+EOF
+
+if compile_prog "" "" ; then
+  optreset=yes
+fi
+
 # check if eventfd is supported
 eventfd=no
 cat > $TMPC << EOF
@@ -6577,6 +6588,9 @@ fi
 if test "$signalfd" = "yes" ; then
   echo "CONFIG_SIGNALFD=y" >> $config_host_mak
 fi
+if test "$optreset" = "yes" ; then
+  echo "HAVE_OPTRESET=y" >> $config_host_mak
+fi
 if test "$tcg" = "yes"; then
   echo "CONFIG_TCG=y" >> $config_host_mak
   if test "$tcg_interpreter" = "yes" ; then
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 80df7253db..840af09cb0 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -109,6 +109,7 @@ extern int daemon(int, int);
 #include <ctype.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <getopt.h>
 #include <sys/stat.h>
 #include <sys/time.h>
 #include <assert.h>
@@ -604,4 +605,19 @@ extern int qemu_icache_linesize_log;
 extern int qemu_dcache_linesize;
 extern int qemu_dcache_linesize_log;
 
+/*
+ * After using getopt or getopt_long, if you need to parse another set
+ * of options, then you must reset optind.  Unfortunately the way to
+ * do this varies between implementations of getopt.
+ */
+static inline void qemu_reset_optind(void)
+{
+#ifdef HAVE_OPTRESET
+    optind = 1;
+    optreset = 1;
+#else
+    optind = 0;
+#endif
+}
+
 #endif
diff --git a/qemu-img.c b/qemu-img.c
index ad04f59565..25288c4d18 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4962,7 +4962,7 @@ int main(int argc, char **argv)
         return 0;
     }
     argv += optind;
-    optind = 0;
+    qemu_reset_optind();
 
     if (!trace_init_backends()) {
         exit(1);
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 2c39124036..ee8f56e46a 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;
+    qemu_reset_optind();
     return ct->cfunc(blk, argc, argv);
 }
 
-- 
2.20.1

Re: [Qemu-devel] [PATCH v4] qemu-io: Add generic function for reinitializing optind.

[Daniel P. Berrangé]

On Fri, Jan 18, 2019 at 10:11:14AM +0000, 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.)
> 
> This commit introduces an OS-portability function called
> qemu_reset_optind which provides a way of resetting optind that works
> on FreeBSD and platforms that use optreset, while keeping it the same
> as now on other platforms.
> 
> Note that the qemu codebase sets optind in many other places, but in
> those other places it's setting a local variable and not using getopt.
> This change is only needed in places where we are using getopt and the
> associated global variable optind.
> 
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> ---
>  configure            | 14 ++++++++++++++
>  include/qemu/osdep.h | 16 ++++++++++++++++
>  qemu-img.c           |  2 +-
>  qemu-io-cmds.c       |  2 +-
>  4 files changed, 32 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v4] qemu-io: Add generic function for reinitializing optind.

[Eric Blake]

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

On 1/18/19 4:11 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

> This commit introduces an OS-portability function called
> qemu_reset_optind which provides a way of resetting optind that works
> on FreeBSD and platforms that use optreset, while keeping it the same
> as now on other platforms.
> 
> Note that the qemu codebase sets optind in many other places, but in
> those other places it's setting a local variable and not using getopt.
> This change is only needed in places where we are using getopt and the
> associated global variable optind.
> 
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> ---
>  configure            | 14 ++++++++++++++
>  include/qemu/osdep.h | 16 ++++++++++++++++
>  qemu-img.c           |  2 +-
>  qemu-io-cmds.c       |  2 +-
>  4 files changed, 32 insertions(+), 2 deletions(-)

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 v4] qemu-io: Add generic function for reinitializing optind.

[Max Reitz]

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

On 18.01.19 11:11, 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.)
> 
> This commit introduces an OS-portability function called
> qemu_reset_optind which provides a way of resetting optind that works
> on FreeBSD and platforms that use optreset, while keeping it the same
> as now on other platforms.
> 
> Note that the qemu codebase sets optind in many other places, but in
> those other places it's setting a local variable and not using getopt.
> This change is only needed in places where we are using getopt and the
> associated global variable optind.
> 
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> ---
>  configure            | 14 ++++++++++++++
>  include/qemu/osdep.h | 16 ++++++++++++++++
>  qemu-img.c           |  2 +-
>  qemu-io-cmds.c       |  2 +-
>  4 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/configure b/configure
> index 3eee3fcf70..3d46df1517 100755

(I'm not quite sure where along the way just using a weak optreset was
discarded, but, oh well, this does make the code less ugly.)

[...]

> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 80df7253db..840af09cb0 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -109,6 +109,7 @@ extern int daemon(int, int);
>  #include <ctype.h>
>  #include <errno.h>
>  #include <fcntl.h>
> +#include <getopt.h>
>  #include <sys/stat.h>
>  #include <sys/time.h>
>  #include <assert.h>
> @@ -604,4 +605,19 @@ extern int qemu_icache_linesize_log;
>  extern int qemu_dcache_linesize;
>  extern int qemu_dcache_linesize_log;
>  
> +/*
> + * After using getopt or getopt_long, if you need to parse another set
> + * of options, then you must reset optind.  Unfortunately the way to
> + * do this varies between implementations of getopt.
> + */
> +static inline void qemu_reset_optind(void)
> +{
> +#ifdef HAVE_OPTRESET
> +    optind = 1;
> +    optreset = 1;
> +#else
> +    optind = 0;

So I take it this is supposed to always do a hard reset -- because if it
isn't, it might have been better to just always set optind = 1 as Eric
suggested.  But googling suggests OpenBSD and NetBSD both have optreset
as well, and newlib accepts optind = 0 (and apparently did not accept
optind = 1 in the past?), so I think this is good for that purpose.


So thanks a lot (and sorry about me being so stupid about
everything...), I've applied the patch to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max

> +#endif
> +}
> +
>  #endif


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