[libvirt] [PATCH 1/2] Fix error detection in device change
According to qemu-kvm/qerror.c all messages start with a capital "Device ", but the current code only scans for the lower case "device ". This results in "virDomainUpdateDeviceFlags()" to not detect locked CD-ROMs and reporting success even in the case of a failure: # virsh qemu-monitor-command "$VM" change\ drive-ide0-0-0\ \"/var/lib/libvirt/images/ucs_2.4-0-sec4-20110714145916-dvd-amd64.iso\" Device 'drive-ide0-0-0' is locked # virsh update-device "$VM" /dev/stdin <<<"<disk type='file' device='cdrom'><driver name='qemu' type='raw'/><source file='/var/lib/libvirt/images/ucs_2.4-0-sec4-20110714145916-dvd-amd64.iso'/><target dev='hda' bus='ide'/><readonly/><alias name='ide0-0-0'/><address type='drive' controller='0' bus='0' unit='0'/></disk>" Device updated successfully Signed-off-by: Philipp Hahn <hahn@univention.de> --- src/qemu/qemu_monitor_text.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 52d924a..98d9169 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -1064,7 +1064,7 @@ int qemuMonitorTextChangeMedia(qemuMonitorPtr mon, /* If the command failed qemu prints: * device not found, device is locked ... * No message is printed on success it seems */ - if (strstr(reply, "device ")) { + if (c_strcasestr(reply, "device ")) { qemuReportError(VIR_ERR_OPERATION_FAILED, _("could not change media on %s: %s"), devname, reply); goto cleanup; -- 1.7.1
On receiving SIGHUP, libvirtd currently only reloads all persistent configs for qemu domains, but fails to reload the associated snapshot data. Copy code from qemudStartup() to qemudReload() to iterate over all domains and call qemuDomainSnapshotLoad() for every domain. Signed-off-by: Philipp Hahn <hahn@univention.de> --- This depends on commits 9e093f0b4cc5a5fc455a4893d73dc0f2c5355161 to fix SIGHUP and on 839a5295ef568727b025f9546c1720279f49fe62 to fix a memory leak. --- src/qemu/qemu_driver.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c8dda73..fcb56ac 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -702,6 +702,9 @@ qemudReload(void) { qemu_driver->autostartDir, 0, QEMU_EXPECTED_VIRT_TYPES, qemudNotifyLoadDomain, qemu_driver); + + virHashForEach(qemu_driver->domains.objs, qemuDomainSnapshotLoad, + qemu_driver->snapshotDir); qemuDriverUnlock(qemu_driver); qemuAutostartDomains(qemu_driver); -- 1.7.1
On 08/30/2011 08:59 AM, Philipp Hahn wrote:
On receiving SIGHUP, libvirtd currently only reloads all persistent configs for qemu domains, but fails to reload the associated snapshot data.
Copy code from qemudStartup() to qemudReload() to iterate over all domains and call qemuDomainSnapshotLoad() for every domain.
Signed-off-by: Philipp Hahn<hahn@univention.de> ---
Are you sure this is still relevant? In my testing, after this was applied, I got lots of errors: 10:58:09.987: 15794: error : virDomainSnapshotAssignDef:11113 : internal error unexpected domain snapshot 1308171632 already exists and without the patch, it looked to me like the snapshots were still all present in memory across SIGHUP without reloading them from disk. I have to wonder if commit 6766ff10d made the difference in how things are behaving? Perhaps your patch is still needed, but if so, can you give me the test scenario you used that shows the difference in behavior pre- and post-patch? Were you hand-modifying the contents of /var/lib/libvirt/qemu/snapshot/dom/*.xml behind libvirt's back and expecting the SIGHUP to have libvirt incorporate the new xml contents from the disk? Are you missing a step that nukes all existing snapshot data in memory before re-reading snapshot data from the disk? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Hello Eric, Am Dienstag 30 August 2011 19:02:50 schrieb Eric Blake:
On 08/30/2011 08:59 AM, Philipp Hahn wrote:
On receiving SIGHUP, libvirtd currently only reloads all persistent configs for qemu domains, but fails to reload the associated snapshot data. ... Are you sure this is still relevant? In my testing, after this was applied, I got lots of errors:
10:58:09.987: 15794: error : virDomainSnapshotAssignDef:11113 : internal error unexpected domain snapshot 1308171632 already exists
and without the patch, it looked to me like the snapshots were still all present in memory across SIGHUP without reloading them from disk. I have to wonder if commit 6766ff10d made the difference in how things are behaving? Perhaps your patch is still needed, but if so, can you give me the test scenario you used that shows the difference in behavior pre- and post-patch?
I posted that patch more for consistency, since sending a SIGHUP to libvirtd re-scans for changes in the domain XMl files under /etc/libvirt/qemu/, but would miss new or updates snapshots under /var/lib/libvirt/qemu/snapshot/.
Were you hand-modifying the contents of /var/lib/libvirt/qemu/snapshot/dom/*.xml behind libvirt's back and expecting the SIGHUP to have libvirt incorporate the new xml contents from the disk?
Yes, see my other post on snapshots-on-a-shared-storage: I had to get that working with 0.8.7 and just forward-ported my patch to HEAD.
Are you missing a step that nukes all existing snapshot data in memory before re-reading snapshot data from the disk?
Might be, will re-check. Sincerely Philipp -- Philipp Hahn Open Source Software Engineer hahn@univention.de Univention GmbH Linux for Your Business fon: +49 421 22 232- 0 Mary-Somerville-Str.1 D-28359 Bremen fax: +49 421 22 232-99 http://www.univention.de/ ---------------------------------------------------------------------------- Treffen Sie Univention auf der IT&Business vom 20. bis 22. September 2011 auf dem Gemeinschaftsstand der Open Source Business Alliance in Stuttgart in Halle 3 Stand 3D27-7.
On 08/30/2011 08:59 AM, Philipp Hahn wrote:
According to qemu-kvm/qerror.c all messages start with a capital "Device ", but the current code only scans for the lower case "device ". This results in "virDomainUpdateDeviceFlags()" to not detect locked CD-ROMs and reporting success even in the case of a failure: # virsh qemu-monitor-command "$VM" change\ drive-ide0-0-0\ \"/var/lib/libvirt/images/ucs_2.4-0-sec4-20110714145916-dvd-amd64.iso\" Device 'drive-ide0-0-0' is locked # virsh update-device "$VM" /dev/stdin<<<"<disk type='file' device='cdrom'><driver name='qemu' type='raw'/><source file='/var/lib/libvirt/images/ucs_2.4-0-sec4-20110714145916-dvd-amd64.iso'/><target dev='hda' bus='ide'/><readonly/><alias name='ide0-0-0'/><address type='drive' controller='0' bus='0' unit='0'/></disk>" Device updated successfully
Signed-off-by: Philipp Hahn<hahn@univention.de> --- src/qemu/qemu_monitor_text.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 52d924a..98d9169 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -1064,7 +1064,7 @@ int qemuMonitorTextChangeMedia(qemuMonitorPtr mon, /* If the command failed qemu prints: * device not found, device is locked ... * No message is printed on success it seems */ - if (strstr(reply, "device ")) { + if (c_strcasestr(reply, "device ")) {
ACK and pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Philipp Hahn