[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 redhat com>
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 redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]