* [Qemu-devel] [PATCH v3 0/3] Fix QemuOpts regression on bogus keys @ 2017-03-22 14:45 Eric Blake 2017-03-22 14:45 ` [Qemu-devel] [PATCH v3 1/3] tests: Expose regression in QemuOpts visitor Eric Blake ` (3 more replies) 0 siblings, 4 replies; 10+ messages in thread From: Eric Blake @ 2017-03-22 14:45 UTC (permalink / raw) To: qemu-devel; +Cc: armbru, lvivier 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v3 1/3] tests: Expose regression in QemuOpts visitor 2017-03-22 14:45 [Qemu-devel] [PATCH v3 0/3] Fix QemuOpts regression on bogus keys Eric Blake @ 2017-03-22 14:45 ` Eric Blake 2017-03-22 14:45 ` [Qemu-devel] [PATCH v3 2/3] qom: Avoid unvisited 'id'/'qom-type' in user_creatable_add_opts Eric Blake ` (2 subsequent siblings) 3 siblings, 0 replies; 10+ messages in thread From: Eric Blake @ 2017-03-22 14:45 UTC (permalink / raw) To: qemu-devel; +Cc: armbru, lvivier, qemu-stable, Michael Roth 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v3 2/3] qom: Avoid unvisited 'id'/'qom-type' in user_creatable_add_opts 2017-03-22 14:45 [Qemu-devel] [PATCH v3 0/3] Fix QemuOpts regression on bogus keys Eric Blake 2017-03-22 14:45 ` [Qemu-devel] [PATCH v3 1/3] tests: Expose regression in QemuOpts visitor Eric Blake @ 2017-03-22 14:45 ` Eric Blake 2017-03-22 15:02 ` Laurent Vivier 2017-03-23 15:37 ` [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 2017-03-22 14:45 ` [Qemu-devel] [PATCH v3 3/3] qapi: Fix QemuOpts visitor regression on unvisited input Eric Blake 2017-03-22 15:58 ` [Qemu-devel] [PATCH v3 0/3] Fix QemuOpts regression on bogus keys Markus Armbruster 3 siblings, 2 replies; 10+ messages in thread From: Eric Blake @ 2017-03-22 14:45 UTC (permalink / raw) To: qemu-devel; +Cc: armbru, lvivier, qemu-stable, Andreas Färber 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] qom: Avoid unvisited 'id'/'qom-type' in user_creatable_add_opts 2017-03-22 14:45 ` [Qemu-devel] [PATCH v3 2/3] qom: Avoid unvisited 'id'/'qom-type' in user_creatable_add_opts Eric Blake @ 2017-03-22 15:02 ` Laurent Vivier 2017-03-23 15:37 ` [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 1 sibling, 0 replies; 10+ messages in thread From: Laurent Vivier @ 2017-03-22 15:02 UTC (permalink / raw) To: Eric Blake, qemu-devel; +Cc: armbru, qemu-stable, Andreas Färber 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; > } > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] Possible regression (was: Re: [PATCH v3 2/3] qom: Avoid unvisited 'id'/'qom-type' in user_creatable_add_opts) 2017-03-22 14:45 ` [Qemu-devel] [PATCH v3 2/3] qom: Avoid unvisited 'id'/'qom-type' in user_creatable_add_opts Eric Blake 2017-03-22 15:02 ` Laurent Vivier @ 2017-03-23 15:37 ` Richard W.M. Jones 2017-03-23 15:48 ` [Qemu-devel] Possible regression Eric Blake 2017-03-23 16:06 ` Markus Armbruster 1 sibling, 2 replies; 10+ messages in thread From: Richard W.M. Jones @ 2017-03-23 15:37 UTC (permalink / raw) To: Eric Blake, armbru; +Cc: qemu-devel, lvivier, Andreas Färber, qemu-stable [-- 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] Possible regression 2017-03-23 15:37 ` [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 @ 2017-03-23 15:48 ` Eric Blake 2017-03-23 16:06 ` Markus Armbruster 1 sibling, 0 replies; 10+ messages in thread From: Eric Blake @ 2017-03-23 15:48 UTC (permalink / raw) To: Richard W.M. Jones, armbru Cc: qemu-devel, lvivier, Andreas Färber, qemu-stable [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] Possible regression 2017-03-23 15:37 ` [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 2017-03-23 15:48 ` [Qemu-devel] Possible regression Eric Blake @ 2017-03-23 16:06 ` Markus Armbruster 2017-03-23 16:11 ` Eric Blake 1 sibling, 1 reply; 10+ messages in thread From: Markus Armbruster @ 2017-03-23 16:06 UTC (permalink / raw) To: Richard W.M. Jones Cc: Eric Blake, lvivier, qemu-stable, qemu-devel, Andreas Färber "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; ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] Possible regression 2017-03-23 16:06 ` Markus Armbruster @ 2017-03-23 16:11 ` Eric Blake 0 siblings, 0 replies; 10+ messages in thread From: Eric Blake @ 2017-03-23 16:11 UTC (permalink / raw) To: Markus Armbruster, Richard W.M. Jones Cc: lvivier, qemu-stable, qemu-devel, Andreas Färber [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v3 3/3] qapi: Fix QemuOpts visitor regression on unvisited input 2017-03-22 14:45 [Qemu-devel] [PATCH v3 0/3] Fix QemuOpts regression on bogus keys Eric Blake 2017-03-22 14:45 ` [Qemu-devel] [PATCH v3 1/3] tests: Expose regression in QemuOpts visitor Eric Blake 2017-03-22 14:45 ` [Qemu-devel] [PATCH v3 2/3] qom: Avoid unvisited 'id'/'qom-type' in user_creatable_add_opts Eric Blake @ 2017-03-22 14:45 ` Eric Blake 2017-03-22 15:58 ` [Qemu-devel] [PATCH v3 0/3] Fix QemuOpts regression on bogus keys Markus Armbruster 3 siblings, 0 replies; 10+ messages in thread From: Eric Blake @ 2017-03-22 14:45 UTC (permalink / raw) To: qemu-devel; +Cc: armbru, lvivier, qemu-stable, Michael Roth 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/3] Fix QemuOpts regression on bogus keys 2017-03-22 14:45 [Qemu-devel] [PATCH v3 0/3] Fix QemuOpts regression on bogus keys Eric Blake ` (2 preceding siblings ...) 2017-03-22 14:45 ` [Qemu-devel] [PATCH v3 3/3] qapi: Fix QemuOpts visitor regression on unvisited input Eric Blake @ 2017-03-22 15:58 ` Markus Armbruster 3 siblings, 0 replies; 10+ messages in thread From: Markus Armbruster @ 2017-03-22 15:58 UTC (permalink / raw) To: Eric Blake; +Cc: qemu-devel, lvivier 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-03-23 16:11 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-22 14:45 [Qemu-devel] [PATCH v3 0/3] Fix QemuOpts regression on bogus keys Eric Blake 2017-03-22 14:45 ` [Qemu-devel] [PATCH v3 1/3] tests: Expose regression in QemuOpts visitor Eric Blake 2017-03-22 14:45 ` [Qemu-devel] [PATCH v3 2/3] qom: Avoid unvisited 'id'/'qom-type' in user_creatable_add_opts Eric Blake 2017-03-22 15:02 ` Laurent Vivier 2017-03-23 15:37 ` [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 2017-03-23 15:48 ` [Qemu-devel] Possible regression Eric Blake 2017-03-23 16:06 ` Markus Armbruster 2017-03-23 16:11 ` Eric Blake 2017-03-22 14:45 ` [Qemu-devel] [PATCH v3 3/3] qapi: Fix QemuOpts visitor regression on unvisited input Eric Blake 2017-03-22 15:58 ` [Qemu-devel] [PATCH v3 0/3] Fix QemuOpts regression on bogus keys Markus Armbruster
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.