[Qemu-devel] [PATCH v3 0/3] Fix QemuOpts regression on bogus keys

[Qemu-devel] [PATCH v3 0/3] Fix QemuOpts regression on bogus keys

[Eric Blake]

Reported to me off-list by Laurent Vivier, who found the
problem while working on https://bugzilla.redhat.com/1433193;
now split into https://bugzilla.redhat.com/show_bug.cgi?id=1434666.
Broken since 2.7, but the fix is a one-liner (pointing out my
embarrassing mistake of mis-converting a pre-decrement operator);
as a bug fix, it still qualifies for 2.9 in spite of hard freeze,
on the other hand, as the regression was not introduced in 2.9,
I also understand if it is postponed.

v1 was here:
https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg04072.html
v2 was here:
https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg04275.html

Since then:
- more commit message tweaks
- drop a spurious hunk from the test in commit 3

Eric Blake (3):
  tests: Expose regression in QemuOpts visitor
  qom: Avoid unvisited 'id'/'qom-type' in user_creatable_add_opts
  qapi: Fix QemuOpts visitor regression on unvisited input

 qapi/opts-visitor.c       |  6 +++---
 qom/object_interfaces.c   |  7 ++++---
 tests/test-opts-visitor.c | 27 +++++++++++++++++++++++++--
 3 files changed, 32 insertions(+), 8 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v3 1/3] tests: Expose regression in QemuOpts visitor

[Eric Blake]

Commit 15c2f669e broke the ability of the QemuOpts visitor to
flag extra input parameters, but the regression went unnoticed
because of missing testsuite coverage.  Add a test to cover this;
take the approach already used in 9cb8ef3 of adding a test that
passes (to avoid breaking bisection) but marks with BUG the
behavior that we don't like, so that the actual impact of the
fix in a later patch is easier to see.

CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

---
v2: s/FIXME/BUG/ in comment, enhance commit message
---
 tests/test-opts-visitor.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c
index 2238f8e..8e0dda5 100644
--- a/tests/test-opts-visitor.c
+++ b/tests/test-opts-visitor.c
@@ -247,6 +247,24 @@ test_opts_range_beyond(void)
     qemu_opts_del(opts);
 }

+static void
+test_opts_dict_unvisited(void)
+{
+    QemuOpts *opts;
+    Visitor *v;
+    UserDefOptions *userdef;
+
+    opts = qemu_opts_parse(qemu_find_opts("userdef"), "i64x=0,bogus=1", false,
+                           &error_abort);
+
+    v = opts_visitor_new(opts);
+    /* BUG: bogus should be diagnosed */
+    visit_type_UserDefOptions(v, NULL, &userdef, &error_abort);
+    visit_free(v);
+    qemu_opts_del(opts);
+    qapi_free_UserDefOptions(userdef);
+}
+
 int
 main(int argc, char **argv)
 {
@@ -343,6 +361,8 @@ main(int argc, char **argv)
     g_test_add_func("/visitor/opts/range/beyond",
                     test_opts_range_beyond);

+    g_test_add_func("/visitor/opts/dict/unvisited", test_opts_dict_unvisited);
+
     g_test_run();
     return 0;
 }
-- 
2.9.3

[Qemu-devel] [PATCH v3 2/3] qom: Avoid unvisited 'id'/'qom-type' in user_creatable_add_opts

[Eric Blake]

A regression in commit 15c2f669e caused us to silently ignore
excess input to the QemuOpts visitor.  Later, commit ea4641
accidentally abused that situation, by removing "qom-type" and
"id" from the corresponding QDict but leaving them defined in
the QemuOpts, when using the pair of containers to create a
user-defined object. Note that since we are already traversing
two separate items (a QDict and a QemuOpts), we are already
able to flag bogus arguments, as in:

$ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio -object memory-backend-ram,id=mem1,size=4k,bogus=huh
qemu-system-x86_64: -object memory-backend-ram,id=mem1,size=4k,bogus=huh: Property '.bogus' not found

So the only real concern is that when we re-enable strict checking
in the QemuOpts visitor, we do not want to start flagging the two
leftover keys as unvisited.  Rearrange the code to clean out the
QemuOpts listing in advance, rather than removing items from the
QDict.  Since "qom-type" is usually an automatic implicit default,
we don't have to restore it (this does mean that once instantiated,
QemuOpts is not necessarily an accurate representation of the
original command line - but this is not the first place to do that);
however "id" has to be put back (requiring us to cast away a const).

[As a side note, hmp_object_add() turns a QDict into a QemuOpts,
then calls user_creatable_add_opts() which converts QemuOpts into
a new QDict. There are probably a lot of wasteful conversions like
this, but cleaning them up is a much bigger task than the immediate
regression fix.]

CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>

---
v3: enhance commit message
v2: new patch
---
 qom/object_interfaces.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 03a95c3..cc9a694 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -114,7 +114,7 @@ Object *user_creatable_add_opts(QemuOpts *opts, Error **errp)
     QDict *pdict;
     Object *obj;
     const char *id = qemu_opts_id(opts);
-    const char *type = qemu_opt_get(opts, "qom-type");
+    char *type = qemu_opt_get_del(opts, "qom-type");

     if (!type) {
         error_setg(errp, QERR_MISSING_PARAMETER, "qom-type");
@@ -125,14 +125,15 @@ Object *user_creatable_add_opts(QemuOpts *opts, Error **errp)
         return NULL;
     }

+    qemu_opts_set_id(opts, NULL);
     pdict = qemu_opts_to_qdict(opts, NULL);
-    qdict_del(pdict, "qom-type");
-    qdict_del(pdict, "id");

     v = opts_visitor_new(opts);
     obj = user_creatable_add_type(type, id, pdict, v, errp);
     visit_free(v);

+    qemu_opts_set_id(opts, (char *) id);
+    g_free(type);
     QDECREF(pdict);
     return obj;
 }
-- 
2.9.3

Re: [Qemu-devel] [PATCH v3 2/3] qom: Avoid unvisited 'id'/'qom-type' in user_creatable_add_opts

[Laurent Vivier]

On 22/03/2017 15:45, Eric Blake wrote:
> A regression in commit 15c2f669e caused us to silently ignore
> excess input to the QemuOpts visitor.  Later, commit ea4641
> accidentally abused that situation, by removing "qom-type" and
> "id" from the corresponding QDict but leaving them defined in
> the QemuOpts, when using the pair of containers to create a
> user-defined object. Note that since we are already traversing
> two separate items (a QDict and a QemuOpts), we are already
> able to flag bogus arguments, as in:
> 
> $ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio -object memory-backend-ram,id=mem1,size=4k,bogus=huh
> qemu-system-x86_64: -object memory-backend-ram,id=mem1,size=4k,bogus=huh: Property '.bogus' not found
> 
> So the only real concern is that when we re-enable strict checking
> in the QemuOpts visitor, we do not want to start flagging the two
> leftover keys as unvisited.  Rearrange the code to clean out the
> QemuOpts listing in advance, rather than removing items from the
> QDict.  Since "qom-type" is usually an automatic implicit default,
> we don't have to restore it (this does mean that once instantiated,
> QemuOpts is not necessarily an accurate representation of the
> original command line - but this is not the first place to do that);
> however "id" has to be put back (requiring us to cast away a const).
> 
> [As a side note, hmp_object_add() turns a QDict into a QemuOpts,
> then calls user_creatable_add_opts() which converts QemuOpts into
> a new QDict. There are probably a lot of wasteful conversions like
> this, but cleaning them up is a much bigger task than the immediate
> regression fix.]
> 
> CC: qemu-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>

Tested-by: Laurent Vivier <lvivier@redhat.com>

> 
> ---
> v3: enhance commit message
> v2: new patch
> ---
>  qom/object_interfaces.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index 03a95c3..cc9a694 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -114,7 +114,7 @@ Object *user_creatable_add_opts(QemuOpts *opts, Error **errp)
>      QDict *pdict;
>      Object *obj;
>      const char *id = qemu_opts_id(opts);
> -    const char *type = qemu_opt_get(opts, "qom-type");
> +    char *type = qemu_opt_get_del(opts, "qom-type");
> 
>      if (!type) {
>          error_setg(errp, QERR_MISSING_PARAMETER, "qom-type");
> @@ -125,14 +125,15 @@ Object *user_creatable_add_opts(QemuOpts *opts, Error **errp)
>          return NULL;
>      }
> 
> +    qemu_opts_set_id(opts, NULL);
>      pdict = qemu_opts_to_qdict(opts, NULL);
> -    qdict_del(pdict, "qom-type");
> -    qdict_del(pdict, "id");
> 
>      v = opts_visitor_new(opts);
>      obj = user_creatable_add_type(type, id, pdict, v, errp);
>      visit_free(v);
> 
> +    qemu_opts_set_id(opts, (char *) id);
> +    g_free(type);
>      QDECREF(pdict);
>      return obj;
>  }
> 

[Qemu-devel] Possible regression (was: Re: [PATCH v3 2/3] qom: Avoid unvisited 'id'/'qom-type' in user_creatable_add_opts)

[Richard W.M. Jones]

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

On Wed, Mar 22, 2017 at 09:45:24AM -0500, Eric Blake wrote:
> A regression in commit 15c2f669e caused us to silently ignore
> excess input to the QemuOpts visitor.  Later, commit ea4641
> accidentally abused that situation, by removing "qom-type" and
> "id" from the corresponding QDict but leaving them defined in
> the QemuOpts, when using the pair of containers to create a
> user-defined object. Note that since we are already traversing
> two separate items (a QDict and a QemuOpts), we are already
> able to flag bogus arguments, as in:
> 
> $ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio -object memory-backend-ram,id=mem1,size=4k,bogus=huh
> qemu-system-x86_64: -object memory-backend-ram,id=mem1,size=4k,bogus=huh: Property '.bogus' not found
> 
> So the only real concern is that when we re-enable strict checking
> in the QemuOpts visitor, we do not want to start flagging the two
> leftover keys as unvisited.  Rearrange the code to clean out the
> QemuOpts listing in advance, rather than removing items from the
> QDict.  Since "qom-type" is usually an automatic implicit default,
> we don't have to restore it (this does mean that once instantiated,
> QemuOpts is not necessarily an accurate representation of the
> original command line - but this is not the first place to do that);
> however "id" has to be put back (requiring us to cast away a const).
> 
> [As a side note, hmp_object_add() turns a QDict into a QemuOpts,
> then calls user_creatable_add_opts() which converts QemuOpts into
> a new QDict. There are probably a lot of wasteful conversions like
> this, but cleaning them up is a much bigger task than the immediate
> regression fix.]
> 
> CC: qemu-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>

This commit causes a problem for libguestfs:

  [02192ms] /home/rjones/d/qemu/x86_64-softmmu/qemu-system-x86_64 \
  [...]
      -object rng-random,filename=/dev/urandom,id=rng0 \
      -device virtio-rng-pci,rng=rng0 \
  [...]
  qemu-system-x86_64: -object rng-random,filename=/dev/urandom,id=rng0: Parameter 'qom-type' is missing

(The full log is attached).  I don't know if we should be including
the qom-type parameter here, and if we should what it should be set to.

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

[-- Attachment #2: log --]
[-- Type: text/plain, Size: 9784 bytes --]

./run test-tool/libguestfs-test-tool -t 60
     ************************************************************
     *                    IMPORTANT NOTICE
     *
     * When reporting bugs, include the COMPLETE, UNEDITED
     * output below in your bug report.
     *
     ************************************************************
LD_LIBRARY_PATH=/home/rjones/d/libguestfs/ruby/ext/guestfs:/home/rjones/d/libguestfs/lib/.libs:/home/rjones/d/libguestfs/java/.libs:/home/rjones/d/libguestfs/gobject/.libs
SUPERMIN_KERNEL=/home/rjones/d/linux/arch/x86/boot/bzImage
LIBGUESTFS_BACKEND=direct
LIBGUESTFS_CACHEDIR=/home/rjones/d/libguestfs/tmp
LIBGUESTFS_HV=/home/rjones/d/qemu/x86_64-softmmu/qemu-system-x86_64
LIBGUESTFS_TMPDIR=/home/rjones/d/libguestfs/tmp
LIBGUESTFS_PATH=/home/rjones/d/libguestfs/appliance
SUPERMIN_MODULES=/tmp/lib/modules/4.11.0-rc3+
PATH=/home/rjones/d/libguestfs/v2v:/home/rjones/d/libguestfs/tools:/home/rjones/d/libguestfs/test-tool:/home/rjones/d/libguestfs/sysprep:/home/rjones/d/libguestfs/sparsify:/home/rjones/d/libguestfs/resize:/home/rjones/d/libguestfs/rescue:/home/rjones/d/libguestfs/p2v:/home/rjones/d/libguestfs/make-fs:/home/rjones/d/libguestfs/inspector:/home/rjones/d/libguestfs/get-kernel:/home/rjones/d/libguestfs/fuse:/home/rjones/d/libguestfs/format:/home/rjones/d/libguestfs/fish:/home/rjones/d/libguestfs/erlang:/home/rjones/d/libguestfs/edit:/home/rjones/d/libguestfs/diff:/home/rjones/d/libguestfs/dib:/home/rjones/d/libguestfs/df:/home/rjones/d/libguestfs/customize:/home/rjones/d/libguestfs/cat:/home/rjones/d/libguestfs/builder:/home/rjones/d/libguestfs/align:/usr/libexec/python2-sphinx:/usr/local/bin:/usr/bin:/usr/local/sbin:/usr/sbin:/home/rjones/.local/bin:/home/rjones/bin
XDG_RUNTIME_DIR=/run/user/1000
SELinux: Enforcing
guestfs_get_append: (null)
guestfs_get_autosync: 1
guestfs_get_backend: direct
guestfs_get_backend_settings: []
guestfs_get_cachedir: /home/rjones/d/libguestfs/tmp
guestfs_get_hv: /home/rjones/d/qemu/x86_64-softmmu/qemu-system-x86_64
guestfs_get_memsize: 500
guestfs_get_network: 0
guestfs_get_path: /home/rjones/d/libguestfs/appliance
guestfs_get_pgroup: 0
guestfs_get_program: libguestfs-test-tool
guestfs_get_recovery_proc: 1
guestfs_get_smp: 1
guestfs_get_sockdir: /run/user/1000
guestfs_get_tmpdir: /home/rjones/d/libguestfs/tmp
guestfs_get_trace: 0
guestfs_get_verbose: 1
host_cpu: x86_64
Launching appliance, timeout set to 60 seconds.
libguestfs: launch: program=libguestfs-test-tool
libguestfs: launch: version=1.37.6
libguestfs: launch: backend registered: unix
libguestfs: launch: backend registered: uml
libguestfs: launch: backend registered: libvirt
libguestfs: launch: backend registered: direct
libguestfs: launch: backend=direct
libguestfs: launch: tmpdir=/home/rjones/d/libguestfs/tmp/libguestfstZZFOk
libguestfs: launch: umask=0002
libguestfs: launch: euid=1000
libguestfs: begin building supermin appliance
libguestfs: run supermin
libguestfs: command: run: /usr/bin/supermin
libguestfs: command: run: \ --build
libguestfs: command: run: \ --verbose
libguestfs: command: run: \ --if-newer
libguestfs: command: run: \ --lock /home/rjones/d/libguestfs/tmp/.guestfs-1000/lock
libguestfs: command: run: \ --copy-kernel
libguestfs: command: run: \ -f ext2
libguestfs: command: run: \ --host-cpu x86_64
libguestfs: command: run: \ /home/rjones/d/libguestfs/appliance/supermin.d
libguestfs: command: run: \ -o /home/rjones/d/libguestfs/tmp/.guestfs-1000/appliance.d
supermin: version: 5.1.17
supermin: rpm: detected RPM version 4.13
supermin: package handler: fedora/rpm
supermin: acquiring lock on /home/rjones/d/libguestfs/tmp/.guestfs-1000/lock
supermin: build: /home/rjones/d/libguestfs/appliance/supermin.d
supermin: reading the supermin appliance
supermin: build: visiting /home/rjones/d/libguestfs/appliance/supermin.d/base.tar.gz type gzip base image (tar)
supermin: build: visiting /home/rjones/d/libguestfs/appliance/supermin.d/daemon.tar.gz type gzip base image (tar)
supermin: build: visiting /home/rjones/d/libguestfs/appliance/supermin.d/excludefiles type uncompressed excludefiles
supermin: build: visiting /home/rjones/d/libguestfs/appliance/supermin.d/hostfiles type uncompressed hostfiles
supermin: build: visiting /home/rjones/d/libguestfs/appliance/supermin.d/init.tar.gz type gzip base image (tar)
supermin: build: visiting /home/rjones/d/libguestfs/appliance/supermin.d/packages type uncompressed packages
supermin: build: visiting /home/rjones/d/libguestfs/appliance/supermin.d/udev-rules.tar.gz type gzip base image (tar)
supermin: build: visiting /home/rjones/d/libguestfs/appliance/supermin.d/zz-packages-dib type uncompressed packages
supermin: build: visiting /home/rjones/d/libguestfs/appliance/supermin.d/zz-packages-xfs type uncompressed packages
supermin: mapping package names to installed packages
supermin: resolving full list of package dependencies
supermin: build: 199 packages, including dependencies
supermin: build: 32878 files
supermin: build: 6136 files, after matching excludefiles
supermin: build: 6146 files, after adding hostfiles
supermin: build: 6127 files, after removing unreadable files
supermin: build: 6156 files, after munging
supermin: kernel: SUPERMIN_KERNEL environment variable /home/rjones/d/linux/arch/x86/boot/bzImage
supermin: kernel: SUPERMIN_KERNEL version 4.11.0-rc3+
supermin: kernel: SUPERMIN_MODULES environment variable = /tmp/lib/modules/4.11.0-rc3+
supermin: kernel: kernel_version 4.11.0-rc3+
supermin: kernel: modules /tmp/lib/modules/4.11.0-rc3+
supermin: ext2: creating empty ext2 filesystem '/home/rjones/d/libguestfs/tmp/.guestfs-1000/appliance.d.ry5zvnen/root'
supermin: ext2: populating from base image
supermin: ext2: copying files from host filesystem
supermin: warning: /usr/libexec/dbus-1/dbus-daemon-launch-helper: Permission denied (ignored)
Some distro files are not public readable, so supermin cannot copy them
into the appliance.  This is a problem with your Linux distro.  Please ask
your distro to stop doing pointless security by obscurity.
You can ignore these warnings.  You *do not* need to use sudo.
supermin: warning: /usr/libexec/utempter/utempter: Permission denied (ignored)
supermin: warning: /usr/sbin/build-locale-archive: Permission denied (ignored)
supermin: warning: /usr/sbin/glibc_post_upgrade.x86_64: Permission denied (ignored)
supermin: warning: /usr/sbin/unix_update: Permission denied (ignored)
supermin: warning: /var/lib/systemd/random-seed: Permission denied (ignored)
supermin: ext2: copying kernel modules
supermin: ext2: creating minimal initrd '/home/rjones/d/libguestfs/tmp/.guestfs-1000/appliance.d.ry5zvnen/initrd'
supermin: ext2: wrote 19 modules to minimal initrd
supermin: renaming /home/rjones/d/libguestfs/tmp/.guestfs-1000/appliance.d.ry5zvnen to /home/rjones/d/libguestfs/tmp/.guestfs-1000/appliance.d
libguestfs: finished building supermin appliance
libguestfs: begin testing qemu features
libguestfs: checking for previously cached test results of /home/rjones/d/qemu/x86_64-softmmu/qemu-system-x86_64, in /home/rjones/d/libguestfs/tmp/.guestfs-1000
libguestfs: command: run: /home/rjones/d/qemu/x86_64-softmmu/qemu-system-x86_64
libguestfs: command: run: \ -display none
libguestfs: command: run: \ -help
libguestfs: qemu version 2.8
libguestfs: command: run: /home/rjones/d/qemu/x86_64-softmmu/qemu-system-x86_64
libguestfs: command: run: \ -display none
libguestfs: command: run: \ -machine accel=kvm:tcg
libguestfs: command: run: \ -device ?
libguestfs: saving test results
libguestfs: finished testing qemu features
[02192ms] /home/rjones/d/qemu/x86_64-softmmu/qemu-system-x86_64 \
    -global virtio-blk-pci.scsi=off \
    -nodefconfig \
    -enable-fips \
    -nodefaults \
    -display none \
    -machine accel=kvm:tcg \
    -cpu host \
    -m 500 \
    -no-reboot \
    -rtc driftfix=slew \
    -no-hpet \
    -global kvm-pit.lost_tick_policy=discard \
    -kernel /home/rjones/d/libguestfs/tmp/.guestfs-1000/appliance.d/kernel \
    -initrd /home/rjones/d/libguestfs/tmp/.guestfs-1000/appliance.d/initrd \
    -object rng-random,filename=/dev/urandom,id=rng0 \
    -device virtio-rng-pci,rng=rng0 \
    -device virtio-scsi-pci,id=scsi \
    -drive file=/home/rjones/d/libguestfs/tmp/libguestfstZZFOk/scratch.1,cache=unsafe,format=raw,id=hd0,if=none \
    -device scsi-hd,drive=hd0 \
    -drive file=/home/rjones/d/libguestfs/tmp/.guestfs-1000/appliance.d/root,snapshot=on,id=appliance,cache=unsafe,if=none,format=raw \
    -device scsi-hd,drive=appliance \
    -device virtio-serial-pci \
    -serial stdio \
    -device sga \
    -chardev socket,path=/run/user/1000/libguestfspgU74E/guestfsd.sock,id=channel0 \
    -device virtserialport,chardev=channel0,name=org.libguestfs.channel.0 \
    -append 'panic=1 console=ttyS0 edd=off udevtimeout=6000 udev.event-timeout=6000 no_timer_check printk.time=1 cgroup_disable=memory usbcore.nousb cryptomgr.notests tsc=reliable 8250.nr_uarts=1 root=/dev/sdb selinux=0 guestfs_verbose=1 TERM=xterm-256color'
qemu-system-x86_64: -object rng-random,filename=/dev/urandom,id=rng0: Parameter 'qom-type' is missing
libguestfs: error: appliance closed the connection unexpectedly, see earlier error messages
libguestfs: child_cleanup: 0x556ca1412b60: child process died
libguestfs: sending SIGTERM to process 11520
libguestfs: error: /home/rjones/d/qemu/x86_64-softmmu/qemu-system-x86_64 exited with error status 1, see debug messages above
libguestfs: error: guestfs_launch failed, see earlier error messages
libguestfs: closing guestfs handle 0x556ca1412b60 (state 0)
libguestfs: command: run: rm
libguestfs: command: run: \ -rf /home/rjones/d/libguestfs/tmp/libguestfstZZFOk
libguestfs: command: run: rm
libguestfs: command: run: \ -rf /run/user/1000/libguestfspgU74E
make: *** [Makefile:2490: quickcheck] Error 1

Re: [Qemu-devel] Possible regression

[Eric Blake]

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

On 03/23/2017 10:37 AM, Richard W.M. Jones wrote:
> On Wed, Mar 22, 2017 at 09:45:24AM -0500, Eric Blake wrote:
>> A regression in commit 15c2f669e caused us to silently ignore
>> excess input to the QemuOpts visitor.  Later, commit ea4641
>> accidentally abused that situation, by removing "qom-type" and
>> "id" from the corresponding QDict but leaving them defined in
>> the QemuOpts, when using the pair of containers to create a
>> user-defined object. Note that since we are already traversing
>> two separate items (a QDict and a QemuOpts), we are already
>> able to flag bogus arguments, as in:
>>
>> $ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio -object memory-backend-ram,id=mem1,size=4k,bogus=huh
>> qemu-system-x86_64: -object memory-backend-ram,id=mem1,size=4k,bogus=huh: Property '.bogus' not found
>>
>> So the only real concern is that when we re-enable strict checking
>> in the QemuOpts visitor, we do not want to start flagging the two
>> leftover keys as unvisited.  Rearrange the code to clean out the
>> QemuOpts listing in advance, rather than removing items from the
>> QDict.  Since "qom-type" is usually an automatic implicit default,
>> we don't have to restore it (this does mean that once instantiated,
>> QemuOpts is not necessarily an accurate representation of the
>> original command line - but this is not the first place to do that);

Well, my assumption that qom-type was not necessary to restore...

> This commit causes a problem for libguestfs:
> 
>   [02192ms] /home/rjones/d/qemu/x86_64-softmmu/qemu-system-x86_64 \
>   [...]
>       -object rng-random,filename=/dev/urandom,id=rng0 \
>       -device virtio-rng-pci,rng=rng0 \
>   [...]
>   qemu-system-x86_64: -object rng-random,filename=/dev/urandom,id=rng0: Parameter 'qom-type' is missing

...was just proven false :(

I should have been more conservative.  I'll post the obvious follow-up
patch that restores qom-type after the temporary removal from the
QemuOpts that is necessary for the duration of the user_creatable_add.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] Possible regression

[Markus Armbruster]

"Richard W.M. Jones" <rjones@redhat.com> writes:

> On Wed, Mar 22, 2017 at 09:45:24AM -0500, Eric Blake wrote:
>> A regression in commit 15c2f669e caused us to silently ignore
>> excess input to the QemuOpts visitor.  Later, commit ea4641
>> accidentally abused that situation, by removing "qom-type" and
>> "id" from the corresponding QDict but leaving them defined in
>> the QemuOpts, when using the pair of containers to create a
>> user-defined object. Note that since we are already traversing
>> two separate items (a QDict and a QemuOpts), we are already
>> able to flag bogus arguments, as in:
>> 
>> $ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio -object memory-backend-ram,id=mem1,size=4k,bogus=huh
>> qemu-system-x86_64: -object memory-backend-ram,id=mem1,size=4k,bogus=huh: Property '.bogus' not found
>> 
>> So the only real concern is that when we re-enable strict checking
>> in the QemuOpts visitor, we do not want to start flagging the two
>> leftover keys as unvisited.  Rearrange the code to clean out the
>> QemuOpts listing in advance, rather than removing items from the
>> QDict.  Since "qom-type" is usually an automatic implicit default,
>> we don't have to restore it (this does mean that once instantiated,
>> QemuOpts is not necessarily an accurate representation of the
>> original command line - but this is not the first place to do that);
>> however "id" has to be put back (requiring us to cast away a const).
>> 
>> [As a side note, hmp_object_add() turns a QDict into a QemuOpts,
>> then calls user_creatable_add_opts() which converts QemuOpts into
>> a new QDict. There are probably a lot of wasteful conversions like
>> this, but cleaning them up is a much bigger task than the immediate
>> regression fix.]
>> 
>> CC: qemu-stable@nongnu.org
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> This commit causes a problem for libguestfs:
>
>   [02192ms] /home/rjones/d/qemu/x86_64-softmmu/qemu-system-x86_64 \
>   [...]
>       -object rng-random,filename=/dev/urandom,id=rng0 \
>       -device virtio-rng-pci,rng=rng0 \
>   [...]
>   qemu-system-x86_64: -object rng-random,filename=/dev/urandom,id=rng0: Parameter 'qom-type' is missing
>
> (The full log is attached).  I don't know if we should be including
> the qom-type parameter here, and if we should what it should be set to.
>
> Rich.

Does the appended patch fix it for you?


diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 9c271ad..4d03665 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -134,6 +134,7 @@ Object *user_creatable_add_opts(QemuOpts *opts, Error **errp)
     visit_free(v);
 
     qemu_opts_set_id(opts, (char *) id);
+    qemu_opt_set(opts, "qom-type", type, &error_abort);
     g_free(type);
     QDECREF(pdict);
     return obj;

Re: [Qemu-devel] Possible regression

[Eric Blake]

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

On 03/23/2017 11:06 AM, Markus Armbruster wrote:

> Does the appended patch fix it for you?
> 
> 
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index 9c271ad..4d03665 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -134,6 +134,7 @@ Object *user_creatable_add_opts(QemuOpts *opts, Error **errp)
>      visit_free(v);
>  
>      qemu_opts_set_id(opts, (char *) id);
> +    qemu_opt_set(opts, "qom-type", type, &error_abort);

See my posting, which covers both exit paths, rather than just the
successful exit path.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

[Qemu-devel] [PATCH v3 3/3] qapi: Fix QemuOpts visitor regression on unvisited input

[Eric Blake]

An off-by-one in commit 15c2f669e meant that we were failing to
check for unparsed input in all QemuOpts visitors.  Recent testsuite
additions show that fixing the obvious bug with bogus fields will
also fix the case of an incomplete list visit; update the tests to
match the new behavior.

Simple testcase:

./x86_64-softmmu/qemu-system-x86_64 -nodefaults -nographic -qmp stdio -numa node,size=1g

failed to diagnose that 'size' is not a valid argument to -numa, and
now once again reports:

qemu-system-x86_64: -numa node,size=1g: Invalid parameter 'size'

See also https://bugzilla.redhat.com/show_bug.cgi?id=1434666

CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
Tested-by: Laurent Vivier <lvivier@redhat.com>

---
v3: drop spurious hunk, comment tweak, add BZ number
v2: trivial rebase to comment tweak in patch 1
---
 qapi/opts-visitor.c       |  6 +++---
 tests/test-opts-visitor.c | 13 ++++++++-----
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 026d25b..324b197 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -164,7 +164,7 @@ opts_check_struct(Visitor *v, Error **errp)
     GHashTableIter iter;
     GQueue *any;

-    if (ov->depth > 0) {
+    if (ov->depth > 1) {
         return;
     }

@@ -276,8 +276,8 @@ static void
 opts_check_list(Visitor *v, Error **errp)
 {
     /*
-     * FIXME should set error when unvisited elements remain.  Mostly
-     * harmless, as the generated visits always visit all elements.
+     * Unvisited list elements will be reported later when checking
+     * whether unvisited struct members remain.
      */
 }

diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c
index 8e0dda5..23e8970 100644
--- a/tests/test-opts-visitor.c
+++ b/tests/test-opts-visitor.c
@@ -175,6 +175,7 @@ expect_u64_max(OptsVisitorFixture *f, gconstpointer test_data)
 static void
 test_opts_range_unvisited(void)
 {
+    Error *err = NULL;
     intList *list = NULL;
     intList *tail;
     QemuOpts *opts;
@@ -199,10 +200,11 @@ test_opts_range_unvisited(void)
     g_assert_cmpint(tail->value, ==, 1);
     tail = (intList *)visit_next_list(v, (GenericList *)tail, sizeof(*list));
     g_assert(tail);
-    visit_check_list(v, &error_abort); /* BUG: unvisited tail not reported */
+    visit_check_list(v, &error_abort); /* unvisited tail ignored until... */
     visit_end_list(v, (void **)&list);

-    visit_check_struct(v, &error_abort);
+    visit_check_struct(v, &err); /* ...here */
+    error_free_or_abort(&err);
     visit_end_struct(v, NULL);

     qapi_free_intList(list);
@@ -250,6 +252,7 @@ test_opts_range_beyond(void)
 static void
 test_opts_dict_unvisited(void)
 {
+    Error *err = NULL;
     QemuOpts *opts;
     Visitor *v;
     UserDefOptions *userdef;
@@ -258,11 +261,11 @@ test_opts_dict_unvisited(void)
                            &error_abort);

     v = opts_visitor_new(opts);
-    /* BUG: bogus should be diagnosed */
-    visit_type_UserDefOptions(v, NULL, &userdef, &error_abort);
+    visit_type_UserDefOptions(v, NULL, &userdef, &err);
+    error_free_or_abort(&err);
     visit_free(v);
     qemu_opts_del(opts);
-    qapi_free_UserDefOptions(userdef);
+    g_assert(!userdef);
 }

 int
-- 
2.9.3

Re: [Qemu-devel] [PATCH v3 0/3] Fix QemuOpts regression on bogus keys

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Reported to me off-list by Laurent Vivier, who found the
> problem while working on https://bugzilla.redhat.com/1433193;
> now split into https://bugzilla.redhat.com/show_bug.cgi?id=1434666.
> Broken since 2.7, but the fix is a one-liner (pointing out my
> embarrassing mistake of mis-converting a pre-decrement operator);
> as a bug fix, it still qualifies for 2.9 in spite of hard freeze,
> on the other hand, as the regression was not introduced in 2.9,
> I also understand if it is postponed.
>
> v1 was here:
> https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg04072.html
> v2 was here:
> https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg04275.html
>
> Since then:
> - more commit message tweaks
> - drop a spurious hunk from the test in commit 3

Reviewed-by: Markus Armbruster <armbru@redhat.com>