[libvirt] [PATCH 0/2] qemu: Fix few snapshot issues
Peter Krempa (2): qemu: snapshot: Reject internal active snapshot without memory state qemu: snapshot: Improve detection of mixed snapshots src/qemu/qemu_driver.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) -- 1.9.3
A internal snapshot of a active VM with the memory snapshot disabled explicitly would actually still take the memory snapshot. Reject it explicitly. Before: $ virsh snapshot-create-as --domain VM --diskspec vda,snapshot=internal --memspec snapshot=no Domain snapshot 1401353155 created After: $ virsh snapshot-create-as --domain VM --diskspec vda,snapshot=internal --memspec snapshot=no error: Operation not supported: internal snapshot of a running VM must include the memory state Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1083345 --- src/qemu/qemu_driver.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9becc0a..272687d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13495,6 +13495,15 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, align_match = false; } else { def->state = virDomainObjGetState(vm, NULL); + + if (virDomainObjIsActive(vm) && + def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("internal snapshot of a running VM " + "must include the memory state")); + goto cleanup; + } + def->memory = (def->state == VIR_DOMAIN_SHUTOFF ? VIR_DOMAIN_SNAPSHOT_LOCATION_NONE : VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL); -- 1.9.3
On 05/29/2014 03:07 AM, Peter Krempa wrote:
A internal snapshot of a active VM with the memory snapshot disabled explicitly would actually still take the memory snapshot. Reject it explicitly.
Before: $ virsh snapshot-create-as --domain VM --diskspec vda,snapshot=internal --memspec snapshot=no Domain snapshot 1401353155 created
After: $ virsh snapshot-create-as --domain VM --diskspec vda,snapshot=internal --memspec snapshot=no error: Operation not supported: internal snapshot of a running VM must include the memory state
Technically, qemu 1.7 added blockdev-snapshot-internal-sync, so we CAN take internal disk-only snapshots now. But as that is more complex to implement, I agree with this patch as an intermediate fix (and we'll still have to keep this code path for qemu 1.6 and earlier, based on a qemu_capabilities.h bit).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1083345 --- src/qemu/qemu_driver.c | 9 +++++++++ 1 file changed, 9 insertions(+)
ACK, safe for 1.2.5. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/29/14 16:36, Eric Blake wrote:
On 05/29/2014 03:07 AM, Peter Krempa wrote:
A internal snapshot of a active VM with the memory snapshot disabled explicitly would actually still take the memory snapshot. Reject it explicitly.
Before: $ virsh snapshot-create-as --domain VM --diskspec vda,snapshot=internal --memspec snapshot=no Domain snapshot 1401353155 created
After: $ virsh snapshot-create-as --domain VM --diskspec vda,snapshot=internal --memspec snapshot=no error: Operation not supported: internal snapshot of a running VM must include the memory state
Technically, qemu 1.7 added blockdev-snapshot-internal-sync, so we CAN take internal disk-only snapshots now. But as that is more complex to implement, I agree with this patch as an intermediate fix (and we'll still have to keep this code path for qemu 1.6 and earlier, based on a qemu_capabilities.h bit).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1083345 --- src/qemu/qemu_driver.c | 9 +++++++++ 1 file changed, 9 insertions(+)
ACK, safe for 1.2.5.
Pushed; Thanks. I'll open a BZ to track the addition. Peter
Currently we don't support mixed (external + internal) snapshots. The code detecting the snapshot type didn't make sure that the memory image was consistent with the snapshot type leading into strange error message: $ virsh snapshot-create-as --domain VM --diskspec vda,snapshot=internal --memspec snapshot=external,file=/tmp/blah error: internal error: unexpected code path Fix the mixed detection code to detect this kind of mistake: $ virsh snapshot-create-as --domain VM --diskspec vda,snapshot=internal --memspec snapshot=external,file=/tmp/blah error: unsupported configuration: mixing internal and external targets for a snapshot is not yet supported --- src/qemu/qemu_driver.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 272687d..59185c6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12755,7 +12755,9 @@ qemuDomainSnapshotPrepare(virConnectPtr conn, /* For now, we don't allow mixing internal and external disks. * XXX technically, we could mix internal and external disks for * offline snapshots */ - if (found_internal && external) { + if ((found_internal && external) || + (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL && external) || + (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL && found_internal)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("mixing internal and external targets for a snapshot " "is not yet supported")); -- 1.9.3
On 05/29/2014 03:08 AM, Peter Krempa wrote:
Currently we don't support mixed (external + internal) snapshots. The code detecting the snapshot type didn't make sure that the memory image was consistent with the snapshot type leading into strange error message:
$ virsh snapshot-create-as --domain VM --diskspec vda,snapshot=internal --memspec snapshot=external,file=/tmp/blah error: internal error: unexpected code path
Fix the mixed detection code to detect this kind of mistake:
$ virsh snapshot-create-as --domain VM --diskspec vda,snapshot=internal --memspec snapshot=external,file=/tmp/blah error: unsupported configuration: mixing internal and external targets for a snapshot is not yet supported
It may still be nice to support some day down the road, but a better error message in the meantime is worth having.
--- src/qemu/qemu_driver.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
ACK; safe for 1.2.5. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/29/14 16:38, Eric Blake wrote:
On 05/29/2014 03:08 AM, Peter Krempa wrote:
Currently we don't support mixed (external + internal) snapshots. The code detecting the snapshot type didn't make sure that the memory image was consistent with the snapshot type leading into strange error message:
$ virsh snapshot-create-as --domain VM --diskspec vda,snapshot=internal --memspec snapshot=external,file=/tmp/blah error: internal error: unexpected code path
Fix the mixed detection code to detect this kind of mistake:
$ virsh snapshot-create-as --domain VM --diskspec vda,snapshot=internal --memspec snapshot=external,file=/tmp/blah error: unsupported configuration: mixing internal and external targets for a snapshot is not yet supported
It may still be nice to support some day down the road, but a better error message in the meantime is worth having.
--- src/qemu/qemu_driver.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
ACK; safe for 1.2.5.
Pushed; Thanks. Peter
participants (2)
-
Eric Blake -
Peter Krempa