On 11/05/2012 05:06 PM, Eric Blake wrote:
>
> Here's the first round of things to squash - I noticed that my earlier
> suggestion of checking for _LIVE and _REDEFINE being mutually exclusive
> is done too late - that needs to be done _before_ the point where we
> alter the current snapshot when update_current is true. Also, this
> tries to keep things so that def->state == VIR_DOMAIN_DISK_SNAPSHOT is
> only possible for a running domain; an offline domain is always recorded
> as VIR_DOMAIN_SHUTOFF, whether the snapshots are internal or external.
>
Looks like I forgot to attach it after all.
> I'm not sure how much of this should be done as an independent patch; in
> fact, it may make sense to split this into multiple patches since I'm
> attempting to address multiple issues.
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@xxxxxxxxxx>
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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list