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

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

[Richard W.M. Jones]

How about this one?  Add a generic osdep function for reinitializing
optind, which does optreset on FreeBSD (but is identical on all other
OSes).  Use it from qemu-io and qemu-img.

I have tested this on Linux, FreeBSD and OpenBSD.

checkpatch complains:

WARNING: Block comments use a leading /* on a separate line
#69: FILE: include/qemu/osdep.h:591:
+/**

WARNING: architecture specific defines should be avoided
#78: FILE: include/qemu/osdep.h:600:
+#ifdef __FreeBSD__

total: 0 errors, 2 warnings, 44 lines checked

but as far as I'm aware I've followed the correct formatting and
guidelines.

Rich.

[Qemu-devel] [PATCH v3] 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, while keeping it the same on other OSes.

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>
---
 include/qemu/osdep.h | 18 ++++++++++++++++++
 qemu-img.c           |  2 +-
 qemu-io-cmds.c       |  2 +-
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 3bf48bcdec..ea4ce4db9e 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -92,6 +92,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>
@@ -587,4 +588,21 @@ extern int qemu_icache_linesize_log;
 extern int qemu_dcache_linesize;
 extern int qemu_dcache_linesize_log;
 
+/**
+ * qemu_reset_optind:
+ *
+ * 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 __FreeBSD__
+    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 v3] qemu-io: Add generic function for reinitializing optind.

[Daniel P. Berrangé]

On Wed, Jan 09, 2019 at 01:23:01PM +0000, Richard W.M. Jones wrote:
> How about this one?  Add a generic osdep function for reinitializing
> optind, which does optreset on FreeBSD (but is identical on all other
> OSes).  Use it from qemu-io and qemu-img.
> 
> I have tested this on Linux, FreeBSD and OpenBSD.
> 
> checkpatch complains:
> 
> WARNING: Block comments use a leading /* on a separate line
> #69: FILE: include/qemu/osdep.h:591:
> +/**

I think it just doesn't like your '/**' and wants '/*' instead.

> WARNING: architecture specific defines should be avoided
> #78: FILE: include/qemu/osdep.h:600:
> +#ifdef __FreeBSD__

Normally we'd suggest doing a configure test to for the platform
feature and then using a feature based ifdef test. In this case
though that would be difficult and/or overly complex.

This does make me wonder about the other *BSDs, OS-X and Mingw
though ?  Should they all be using the #else codepath, or should
the other BSDs / OS-X use the __FreeBSD__ codepath.


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

[Richard W.M. Jones]

On Wed, Jan 09, 2019 at 01:44:48PM +0000, Daniel P. Berrangé wrote:
> On Wed, Jan 09, 2019 at 01:23:01PM +0000, Richard W.M. Jones wrote:
> > How about this one?  Add a generic osdep function for reinitializing
> > optind, which does optreset on FreeBSD (but is identical on all other
> > OSes).  Use it from qemu-io and qemu-img.
> > 
> > I have tested this on Linux, FreeBSD and OpenBSD.
> > 
> > checkpatch complains:
> > 
> > WARNING: Block comments use a leading /* on a separate line
> > #69: FILE: include/qemu/osdep.h:591:
> > +/**
> 
> I think it just doesn't like your '/**' and wants '/*' instead.

The existing comments in the same file are a mix of three styles.
I chose the style used closest to the new comment I was adding :-)

> > WARNING: architecture specific defines should be avoided
> > #78: FILE: include/qemu/osdep.h:600:
> > +#ifdef __FreeBSD__
> 
> Normally we'd suggest doing a configure test to for the platform
> feature and then using a feature based ifdef test. In this case
> though that would be difficult and/or overly complex.
> 
> This does make me wonder about the other *BSDs, OS-X and Mingw

OpenBSD is known fine with optind = 0.  I can't test OS-X.  I can test
mingw (on Linux) later.

Rich.

> though ?  Should they all be using the #else codepath, or should
> the other BSDs / OS-X use the __FreeBSD__ codepath.
> 
> 
> 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 :|

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

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

[Daniel P. Berrangé]

On Wed, Jan 09, 2019 at 02:14:46PM +0000, Richard W.M. Jones wrote:
> On Wed, Jan 09, 2019 at 01:44:48PM +0000, Daniel P. Berrangé wrote:
> > On Wed, Jan 09, 2019 at 01:23:01PM +0000, Richard W.M. Jones wrote:
> > > How about this one?  Add a generic osdep function for reinitializing
> > > optind, which does optreset on FreeBSD (but is identical on all other
> > > OSes).  Use it from qemu-io and qemu-img.
> > > 
> > > I have tested this on Linux, FreeBSD and OpenBSD.
> > > 
> > > checkpatch complains:
> > > 
> > > WARNING: Block comments use a leading /* on a separate line
> > > #69: FILE: include/qemu/osdep.h:591:
> > > +/**
> > 
> > I think it just doesn't like your '/**' and wants '/*' instead.
> 
> The existing comments in the same file are a mix of three styles.
> I chose the style used closest to the new comment I was adding :-)

Yeah, pre-existing code is a mess often not passing current style
rules. I wish we'd clean up existing code, but failing that, it can
be valid to ignore style warnings to be more consistent with existing
code.

> 
> > > WARNING: architecture specific defines should be avoided
> > > #78: FILE: include/qemu/osdep.h:600:
> > > +#ifdef __FreeBSD__
> > 
> > Normally we'd suggest doing a configure test to for the platform
> > feature and then using a feature based ifdef test. In this case
> > though that would be difficult and/or overly complex.
> > 
> > This does make me wonder about the other *BSDs, OS-X and Mingw
> 
> OpenBSD is known fine with optind = 0.  I can't test OS-X.  I can test
> mingw (on Linux) later.
> 
> 
> > though ?  Should they all be using the #else codepath, or should
> > the other BSDs / OS-X use the __FreeBSD__ codepath.

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

[Eric Blake]

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

On 1/9/19 7:44 AM, Daniel P. Berrangé wrote:
> On Wed, Jan 09, 2019 at 01:23:01PM +0000, Richard W.M. Jones wrote:
>> How about this one?  Add a generic osdep function for reinitializing
>> optind, which does optreset on FreeBSD (but is identical on all other
>> OSes).  Use it from qemu-io and qemu-img.
>>
>> I have tested this on Linux, FreeBSD and OpenBSD.
>>

> 
>> WARNING: architecture specific defines should be avoided
>> #78: FILE: include/qemu/osdep.h:600:
>> +#ifdef __FreeBSD__
> 
> Normally we'd suggest doing a configure test to for the platform
> feature and then using a feature based ifdef test. In this case
> though that would be difficult and/or overly complex.
> 
> This does make me wonder about the other *BSDs, OS-X and Mingw
> though ?  Should they all be using the #else codepath, or should
> the other BSDs / OS-X use the __FreeBSD__ codepath.

Indeed, and I already suggested a configure-time probe  on the v2
review.  My preference, if we want to go with this helper function for a
hard reset, would be:

#if HAVE_OPTRESET
  optind = 1;
  optreset = 1;
#else
  optind = 0;
#endif

where HAVE_OPTRESET is based on a configure-time probe.

Basically, any platform that HAS optreset will probably honor it; any
platform that lacks optreset is hopefully okay with the
POSIX-unspecified behavior of optind = 0.

But I do like the idea presented in this version of trying to isolate
the reset into a common helper function, so that if we have to make
later changes, we only have to touch the one function rather than all
callsites that reset getopt parsing.

-- 
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 --]