Re: [libvirt] [PATCHv2 15/20] snapshot: qemu: Add support for external inactive snapshots

On 11/05/2012 05:06 PM, Eric Blake wrote: Looks like I forgot to attach it after all. This still applies, and in fact I know you are already refactoring things; so I'll see what happens for v3. From d02d92d6f24a18d1e50508dd03e13f9b8945c781 Mon Sep 17 00:00:00 2001 From: Eric Blake <eblake(a)redhat.com&gt; Date: Mon, 5 Nov 2012 17:09:36 -0700 Subject: [PATCH] fixup #1 to 15/20 --- src/qemu/qemu_driver.c | 87 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 56 insertions(+), 31 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 080a862..c393b88 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10865,11 +10865,11 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, { int ret = -1; int i; - bool found = false; bool active = virDomainObjIsActive(vm); struct stat st; bool reuse = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; bool atomic = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC) != 0; + bool found_internal = false; int external = 0; qemuDomainObjPrivatePtr priv = vm->privateData; @@ -10890,7 +10890,6 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, dom_disk->type == VIR_DOMAIN_DISK_TYPE_NETWORK && (dom_disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_SHEEPDOG || dom_disk->protocol == VIR_DOMAIN_DISK_PROTOCOL_RBD)) { - found = true; break; } if (vm->def->disks[i]->format > 0 && @@ -10910,7 +10909,7 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, disk->name); goto cleanup; } - found = true; + found_internal = true; break; case VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL: @@ -10944,7 +10943,6 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, disk->name, disk->file); goto cleanup; } - found = true; external++; break; @@ -10959,15 +10957,34 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, } } - /* external snapshot is possible without specifying a disk to snapshot */ - if (!found && - def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { + /* internal snapshot requires an internal disk */ + if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL && + !found_internal) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("internal and disk-only snapshots require at least " + _("internal snapshots require at least " + "one disk to be selected for snapshot")); + goto cleanup; + } + /* 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) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("mixing internal and external snapshots is not " + "supported yet")); + goto cleanup; + } + /* disk snapshot requires at least one disk */ + if (def->state == VIR_DOMAIN_DISK_SNAPSHOT && !external) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk-only snapshots require at least " "one disk to be selected for snapshot")); goto cleanup; } + /* Alter flags to let later users know what we learned. */ + if (external && !active) + *flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; if (def->state != VIR_DOMAIN_DISK_SNAPSHOT && active) { if (external == 1 || qemuCapsGet(priv->caps, QEMU_CAPS_TRANSACTION)) { @@ -11460,6 +11477,25 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, parse_flags))) goto cleanup; + /* reject the VIR_DOMAIN_SNAPSHOT_CREATE_LIVE flag where not supported */ + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_LIVE && + (!virDomainObjIsActive(vm) || + snap->def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL || + flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("live snapshot creation is supported only " + "with external checkpoints")); + goto cleanup; + } + if ((snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL || + snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL) && + flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("disk-only snapshot creation is not compatible with " + "memory snapshot")); + goto cleanup; + } + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) { /* Prevent circular chains */ if (def->parent) { @@ -11574,7 +11610,10 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; align_match = false; - def->state = VIR_DOMAIN_DISK_SNAPSHOT; + if (virDomainObjIsActive(vm)) + def->state = VIR_DOMAIN_DISK_SNAPSHOT; + else + def->state = VIR_DOMAIN_SHUTOFF; def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; } else if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { def->state = virDomainObjGetState(vm, NULL); @@ -11617,25 +11656,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } } - /* reject the VIR_DOMAIN_SNAPSHOT_CREATE_LIVE flag where not supported */ - if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_LIVE && - (!virDomainObjIsActive(vm) || - snap->def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL || - flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("live snapshot creation is supported only " - "with external checkpoints")); - goto cleanup; - } - if ((snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL || - snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL) && - flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("disk-only snapshot creation is not compatible with " - "memory snapshot")); - goto cleanup; - } - /* actually do the snapshot */ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) { /* XXX Should we validate that the redefined snapshot even @@ -11655,9 +11675,14 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; } } else { - /* inactive */ - if (snap->def->state == VIR_DOMAIN_DISK_SNAPSHOT) { - if (qemuDomainSnapshotCreateInactiveExternal(driver, vm, snap, false) < 0) + /* inactive; qemuDomainSnapshotPrepare guaranteed that we + * aren't mixing internal and external, and altered flags to + * contain DISK_ONLY if there is an external disk. */ + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { + bool reuse = !!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT); + + if (qemuDomainSnapshotCreateInactiveExternal(driver, vm, snap, + reuse) < 0) goto cleanup; } else { if (qemuDomainSnapshotCreateInactiveInternal(driver, vm, snap) < 0) -- 1.7.11.7 -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org