September 2011 - Devel - libvirt List Archives

[libvirt] [PATCH] locking: avoid dereferencing a NULL pointer
by ajia＠redhat.com 22 Sep '11

22 Sep '11

From: Alex Jia <ajia(a)redhat.com> * src/locking/lock_driver_sanlock.c: in fact, virStrcpy calls virStrncpy(dest, src, strlen(src), destbytes) then return result, if 'path' is NULL, it means 'src' is also NULL, strlen(NULL) will dereference a NULL pointer, which probably causes a segmentation fault. Signed-off-by: Alex Jia <ajia(a)redhat.com> --- src/locking/lock_driver_sanlock.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 2d72510..8c6d873 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -158,7 +158,7 @@ static int virLockManagerSanlockSetupLockspace(void) memcpy(ls.name, VIR_LOCK_MANAGER_SANLOCK_AUTO_DISK_LOCKSPACE, SANLK_NAME_LEN); ls.host_id = 0; /* Doesn't matter for initialization */ ls.flags = 0; - if (!virStrcpy(ls.host_id_disk.path, path, SANLK_PATH_LEN)) { + if (!path || !virStrcpy(ls.host_id_disk.path, path, SANLK_PATH_LEN)) { virLockError(VIR_ERR_INTERNAL_ERROR, _("Lockspace path '%s' exceeded %d characters"), path, SANLK_PATH_LEN); -- 1.7.1

4 4

[libvirt] [PATCH] virsh: fix regression in argv parsing
by Eric Blake 22 Sep '11

22 Sep '11

Commit 85d2810 broke commands with mandatory argv (send-key, qemu-monitor-command). This fixes things, and enhances the test to cover it. * tools/virsh.c (vshCmddefGetOption, vshCmddefGetData) (vshCommandParse): Fix option parsing for required argv option. (vshCmddefOptParse): Check that argv option is last. * tests/virsh-optparse: Enhance test. --- tests/virsh-optparse | 9 +++++++++ tools/virsh.c | 24 +++++++++++++++--------- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/tests/virsh-optparse b/tests/virsh-optparse index 18252d2..d0d4329 100755 --- a/tests/virsh-optparse +++ b/tests/virsh-optparse @@ -118,6 +118,7 @@ for args in \ 'test name desc vda vdb' \ 'test --diskspec vda name --diskspec vdb desc' \ '--description desc --name name --domain test vda vdb' \ + '--description desc --diskspec vda --name name --domain test vdb' \ ; do virsh -q -c $test_url snapshot-create-as --print-xml $args \ >out 2>>err || fail=1 @@ -126,4 +127,12 @@ done test -s err && fail=1 +# Test a required argv +cat <<\EOF > exp-err || framework_failure +error: this function is not supported by the connection driver: virDomainQemuMonitorCommand +EOF +virsh -q -c $test_url qemu-monitor-command test a >out 2>err && fail=1 +test -s out && fail=1 +compare err exp-err || fail=1 + (exit $fail); exit $fail diff --git a/tools/virsh.c b/tools/virsh.c index 371346a..0b187bc 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -13834,6 +13834,7 @@ vshCmddefGetInfo(const vshCmdDef * cmd, const char *name) return NULL; } +/* Validate that the options associated with cmd can be parsed. */ static int vshCmddefOptParse(const vshCmdDef *cmd, uint32_t *opts_need_arg, uint32_t *opts_required) @@ -13871,13 +13872,16 @@ vshCmddefOptParse(const vshCmdDef *cmd, uint32_t *opts_need_arg, } else { optional = true; } + + if (opt->type == VSH_OT_ARGV && cmd->opts[i + 1].name) + return -1; /* argv option must be listed last */ } return 0; } static const vshCmdOptDef * vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, - uint32_t *opts_seen) + uint32_t *opts_seen, int *opt_index) { int i; @@ -13885,12 +13889,12 @@ vshCmddefGetOption(vshControl *ctl, const vshCmdDef *cmd, const char *name, const vshCmdOptDef *opt = &cmd->opts[i]; if (STREQ(opt->name, name)) { - if (*opts_seen & (1 << i)) { + if ((*opts_seen & (1 << i)) && opt->type != VSH_OT_ARGV) { vshError(ctl, _("option --%s already seen"), name); return NULL; } - if (opt->type != VSH_OT_ARGV) - *opts_seen |= 1 << i; + *opts_seen |= 1 << i; + *opt_index = i; return opt; } } @@ -13913,10 +13917,9 @@ vshCmddefGetData(const vshCmdDef *cmd, uint32_t *opts_need_arg, /* Grab least-significant set bit */ i = ffs(*opts_need_arg) - 1; opt = &cmd->opts[i]; - if (opt->type != VSH_OT_ARGV) { + if (opt->type != VSH_OT_ARGV) *opts_need_arg &= ~(1 << i); - *opts_seen |= 1 << i; - } + *opts_seen |= 1 << i; return opt; } @@ -14878,12 +14881,14 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser) } else if (tkdata[0] == '-' && tkdata[1] == '-' && c_isalnum(tkdata[2])) { char *optstr = strchr(tkdata + 2, '='); + int opt_index; + if (optstr) { *optstr = '\0'; /* convert the '=' to '\0' */ optstr = vshStrdup(ctl, optstr + 1); } if (!(opt = vshCmddefGetOption(ctl, cmd, tkdata + 2, - &opts_seen))) { + &opts_seen, &opt_index))) { VIR_FREE(optstr); goto syntaxError; } @@ -14905,7 +14910,8 @@ vshCommandParse(vshControl *ctl, vshCommandParser *parser) VSH_OT_INT ? _("number") : _("string")); goto syntaxError; } - opts_need_arg &= ~opts_seen; + if (opt->type != VSH_OT_ARGV) + opts_need_arg &= ~(1 << opt_index); } else { tkdata = NULL; if (optstr) { -- 1.7.4.4

2 2

[libvirt] [PATCH] locking: remove redundant codes
by ajia＠redhat.com 22 Sep '11

22 Sep '11

From: Alex Jia <ajia(a)redhat.com> * src/locking/lock_driver_sanlock.c: remove redundant codes in error_unlink label from virLockManagerSanlockSetupLockspace, in fact, the codes make sure 'path' is non-null before getting to error_unlink label, so 'if (path)' is redundant, removing it in here and will also silence coverity. Signed-off-by: Alex Jia <ajia(a)redhat.com> --- src/locking/lock_driver_sanlock.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 2d72510..c4dce4f 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -250,8 +250,7 @@ static int virLockManagerSanlockSetupLockspace(void) return 0; error_unlink: - if (path) - unlink(path); + unlink(path); error: VIR_FORCE_CLOSE(fd); VIR_FREE(path); -- 1.7.1

3 2

[libvirt] [PATCH] qemu: avoid dereferencing a NULL pointer
by ajia＠redhat.com 22 Sep '11

22 Sep '11

From: Alex Jia <ajia(a)redhat.com> * src/qemu/qemu_process.c: Taking if (qemuDomainObjEndJob(driver, obj) == 0) true branch then 'obj' is NULL, virDomainObjIsActive(obj) and virDomainObjUnref(obj) will dereference NULL pointer. Signed-off-by: Alex Jia <ajia(a)redhat.com> --- src/qemu/qemu_process.c | 32 +++++++++++++++++--------------- 1 files changed, 17 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index bd49b21..9fdf846 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2661,22 +2661,24 @@ error: if (qemuDomainObjEndJob(driver, obj) == 0) obj = NULL; - if (!virDomainObjIsActive(obj)) { - if (virDomainObjUnref(obj) > 0) - virDomainObjUnlock(obj); - qemuDriverUnlock(driver); - return; - } + if (obj) { + if (!virDomainObjIsActive(obj)) { + if (virDomainObjUnref(obj) > 0) + virDomainObjUnlock(obj); + qemuDriverUnlock(driver); + return; + } - if (virDomainObjUnref(obj) > 0) { - /* We can't get the monitor back, so must kill the VM - * to remove danger of it ending up running twice if - * user tries to start it again later */ - qemuProcessStop(driver, obj, 0, VIR_DOMAIN_SHUTOFF_FAILED); - if (!obj->persistent) - virDomainRemoveInactive(&driver->domains, obj); - else - virDomainObjUnlock(obj); + if (virDomainObjUnref(obj) > 0) { + /* We can't get the monitor back, so must kill the VM + * to remove danger of it ending up running twice if + * user tries to start it again later */ + qemuProcessStop(driver, obj, 0, VIR_DOMAIN_SHUTOFF_FAILED); + if (!obj->persistent) + virDomainRemoveInactive(&driver->domains, obj); + else + virDomainObjUnlock(obj); + } } qemuDriverUnlock(driver); -- 1.7.1

3 2

[libvirt] [PATCH] virsh: More friendly err if no pool is specified for looking up a vol
by Osier Yang 22 Sep '11

22 Sep '11

There are 3 ways to lookup a volume, only virStorageVolLookupByName needs pool object. So if no --pool is specified, it will tries to get the volume via virStorageVolLookupByPath/virStorageVolLookupByKey. But if all 3 ways fails, and no --pool is specified, a friendly error might help the user get right way quickly. --- tools/virsh.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 371346a..4b9e662 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -14714,8 +14714,13 @@ vshCommandOptVolBy(vshControl *ctl, const vshCmd *cmd, vol = virStorageVolLookupByPath(ctl->conn, n); } - if (!vol) - vshError(ctl, _("failed to get vol '%s'"), n); + if (!vol) { + if (pool) + vshError(ctl, _("failed to get vol '%s'"), n); + else + vshError(ctl, _("failed to get vol '%s', specifying --pool " + "might help"), n); + } if (pool) virStoragePoolFree(pool); -- 1.7.6

2 2

[libvirt] [PATCH] storage: Wait udev events are handled before removing lvm vol
by Osier Yang 21 Sep '11

21 Sep '11

Related #BZ: https://bugzilla.redhat.com/show_bug.cgi?id=702260. There are two problems described in the BZ: 1) "Can't remove open logical volume". 2) "Unable to deactivate logical volume "foo"" This patch just intends to fix 2), as 1) is expected if the vol is still used by something, and you never known if "lvchange -an" will fail or not either (sometime, it will succeed, sometimes not). We'd better not look for trouble, :-) For 2), that's caused by race between lvremove and udev event handling, the only workable way now is to wait the events handling are finished, though it might introduce latencies, as "udevadmin settle" exits after *all* events are handled, it's the only way we can fix the racing in libvirt layer. See https://bugzilla.redhat.com/show_bug.cgi?id=570359 for more details. --- src/storage/storage_backend_logical.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 4f42047..23d80cb 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -686,6 +686,8 @@ virStorageBackendLogicalDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED, virCheckFlags(0, -1); + virFileWaitForDevices(); + if (virRun(cmdargv, NULL) < 0) return -1; -- 1.7.6

2 2

[libvirt] [PATCH] qemu: avoid dereferencing a NULL pointer
by ajia＠redhat.com 21 Sep '11

21 Sep '11

From: Alex Jia <ajia(a)redhat.com> * src/qemu/qemu_process.c: Taking if (qemuDomainObjEndJob(driver, obj) == 0) true branch then 'obj' is NULL, virDomainObjIsActive(obj) and virDomainObjUnref(obj) will dereference NULL pointer. Signed-off-by: Alex Jia <ajia(a)redhat.com> --- src/qemu/qemu_process.c | 32 +++++++++++++++++--------------- 1 files changed, 17 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index bd49b21..9fdf846 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2661,22 +2661,24 @@ error: if (qemuDomainObjEndJob(driver, obj) == 0) obj = NULL; - if (!virDomainObjIsActive(obj)) { - if (virDomainObjUnref(obj) > 0) - virDomainObjUnlock(obj); - qemuDriverUnlock(driver); - return; - } + if (obj) { + if (!virDomainObjIsActive(obj)) { + if (virDomainObjUnref(obj) > 0) + virDomainObjUnlock(obj); + qemuDriverUnlock(driver); + return; + } - if (virDomainObjUnref(obj) > 0) { - /* We can't get the monitor back, so must kill the VM - * to remove danger of it ending up running twice if - * user tries to start it again later */ - qemuProcessStop(driver, obj, 0, VIR_DOMAIN_SHUTOFF_FAILED); - if (!obj->persistent) - virDomainRemoveInactive(&driver->domains, obj); - else - virDomainObjUnlock(obj); + if (virDomainObjUnref(obj) > 0) { + /* We can't get the monitor back, so must kill the VM + * to remove danger of it ending up running twice if + * user tries to start it again later */ + qemuProcessStop(driver, obj, 0, VIR_DOMAIN_SHUTOFF_FAILED); + if (!obj->persistent) + virDomainRemoveInactive(&driver->domains, obj); + else + virDomainObjUnlock(obj); + } } qemuDriverUnlock(driver); -- 1.7.1

1 0

[libvirt] virsh qemu-monitor-command broken with 0.9.5
by Jason Krieg 21 Sep '11

21 Sep '11

Hi, the virsh qemu-monitor-command is not working with libvirt 0.9.5 with git commit 85d2810823a31634b12145d6c196930b40425370 *opts_seen moved into the != VSH_OT_ARGV if statement so now opts_seen is only set if not VSH_OT_ARGV see attached fix Regards Jason

4 5

[libvirt] [PATCH] qemu: Avoid loop of fake reboots
by Jiri Denemark 21 Sep '11

21 Sep '11

Once virDomainReboot is called for a domain, guest OS initiated shutdown would always result in reboot instead of shutdown. Only virDomainShutdown would actually shutd such domain down. That's because we forgot to reset fakeReboot flag once we asked the domain to reboot. --- src/qemu/qemu_process.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3baaa19..bd49b21 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -445,6 +445,7 @@ qemuProcessHandleShutdown(qemuMonitorPtr mon ATTRIBUTE_UNUSED, priv->gotShutdown = true; if (priv->fakeReboot) { + priv->fakeReboot = false; virDomainObjRef(vm); virThread th; if (virThreadCreate(&th, -- 1.7.6.1

2 2

[libvirt] [PATCH v2] qemu: Fix shutdown regression with buggy qemu
by Jiri Denemark 21 Sep '11

21 Sep '11

The commit that prevents disk corruption on domain shutdown (96fc4784177ecb70357518fa863442455e45ad0e) causes regression with QEMU 0.14.* and 0.15.* because of a regression bug in QEMU that was fixed only recently in QEMU git. The affected versions of QEMU do not quit on SIGTERM if started with -no-shutdown, which we use to implement fake reboot. Since -no-shutdown tells QEMU not to quit automatically on guest shutdown, domains started using the affected QEMU cannot be shutdown properly and stay in a paused state. This patch disables fake reboot feature on such QEMU by not using -no-shutdown, which makes shutdown work as expected. However, virDomainReboot will not work in this case and it will report "Requested operation is not valid: Reboot is not supported with this QEMU binary". --- src/qemu/qemu_capabilities.c | 8 ++++++++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 2 +- src/qemu/qemu_driver.c | 6 ++++++ 4 files changed, 16 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 36f47a9..850d46e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -136,6 +136,7 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST, "pci-ohci", "usb-redir", "usb-hub", + "no-shutdown", ); struct qemu_feature_flags { @@ -1008,6 +1009,13 @@ qemuCapsComputeCmdFlags(const char *help, qemuCapsSet(flags, QEMU_CAPS_VHOST_NET); } + /* Do not use -no-shutdown if qemu doesn't support it or SIGTERM handling + * is most likely buggy when used with -no-shutdown (which applies for qemu + * 0.14.* and 0.15.*) + */ + if (strstr(help, "-no-shutdown") && (version < 14000 || version > 15999)) + qemuCapsSet(flags, QEMU_CAPS_NO_SHUTDOWN); + /* * Handling of -incoming arg with varying features * -incoming tcp (kvm >= 79, qemu >= 0.10.0) diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 96b7a3b..74d3ab2 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -110,6 +110,7 @@ enum qemuCapsFlags { QEMU_CAPS_PCI_OHCI = 71, /* -device pci-ohci */ QEMU_CAPS_USB_REDIR = 72, /* -device usb-redir */ QEMU_CAPS_USB_HUB = 73, /* -device usb-hub */ + QEMU_CAPS_NO_SHUTDOWN = 74, /* usable -no-shutdown */ QEMU_CAPS_LAST, /* this must always be the last item */ }; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index ee4b52b..0adc56a 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3574,7 +3574,7 @@ qemuBuildCommandLine(virConnectPtr conn, * when QEMU stops. If we use no-shutdown, then we can * watch for this event and do a soft/warm reboot. */ - if (monitor_json) + if (monitor_json && qemuCapsGet(qemuCaps, QEMU_CAPS_NO_SHUTDOWN)) virCommandAddArg(cmd, "-no-shutdown"); if (!(def->features & (1 << VIR_DOMAIN_FEATURE_ACPI))) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f4ee4c3..67c43ab 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1557,6 +1557,12 @@ static int qemuDomainReboot(virDomainPtr dom, unsigned int flags) { priv = vm->privateData; if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_MONITOR_JSON)) { + if (!qemuCapsGet(priv->qemuCaps, QEMU_CAPS_NO_SHUTDOWN)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Reboot is not supported with this QEMU binary")); + goto cleanup; + } + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; -- 1.7.6.1

2 2