[PATCH v3 10/18] snapshot: Split out virDomainSnapshotRedefineValidate helper

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

 



Pull out the portion of virDomainSnapshotRefinePrep() that deals
with definition sanity into a separate helper routine that can
be reused with bulk redefine, leaving behind only the code
specific to loop checking and in-place updates that are only
needed in single-definition handling.

Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
---
 src/conf/snapshot_conf.c | 186 ++++++++++++++++++++-------------------
 1 file changed, 96 insertions(+), 90 deletions(-)

diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 386ec82d15..a5b05eadf4 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -426,6 +426,87 @@ virDomainSnapshotDefParseString(const char *xmlStr,
 }


+/* Perform sanity checking on a redefined snapshot definition. If
+ * @other is non-NULL, this may include swapping def->dom from other
+ * into def. */
+static int
+virDomainSnapshotRedefineValidate(virDomainSnapshotDefPtr def,
+                                  const unsigned char *domain_uuid,
+                                  virDomainSnapshotObjPtr other,
+                                  virDomainXMLOptionPtr xmlopt,
+                                  unsigned int flags)
+{
+    int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
+    bool align_match = true;
+    bool external = def->state == VIR_SNAP_STATE_DISK_SNAPSHOT ||
+        virDomainSnapshotDefIsExternal(def);
+
+    if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) && !external) {
+        virReportError(VIR_ERR_INVALID_ARG,
+                       _("disk-only flag for snapshot %s requires "
+                         "disk-snapshot state"),
+                       def->name);
+        return -1;
+    }
+    if (def->dom && memcmp(def->dom->uuid, domain_uuid, VIR_UUID_BUFLEN)) {
+        char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+        virUUIDFormat(domain_uuid, uuidstr);
+        virReportError(VIR_ERR_INVALID_ARG,
+                       _("definition for snapshot %s must use uuid %s"),
+                       def->name, uuidstr);
+        return -1;
+    }
+
+    if (other) {
+        if ((other->def->state == VIR_SNAP_STATE_RUNNING ||
+             other->def->state == VIR_SNAP_STATE_PAUSED) !=
+            (def->state == VIR_SNAP_STATE_RUNNING ||
+             def->state == VIR_SNAP_STATE_PAUSED)) {
+            virReportError(VIR_ERR_INVALID_ARG,
+                           _("cannot change between online and offline "
+                             "snapshot state in snapshot %s"),
+                           def->name);
+            return -1;
+        }
+
+        if ((other->def->state == VIR_SNAP_STATE_DISK_SNAPSHOT) !=
+            (def->state == VIR_SNAP_STATE_DISK_SNAPSHOT)) {
+            virReportError(VIR_ERR_INVALID_ARG,
+                           _("cannot change between disk only and "
+                             "full system in snapshot %s"),
+                           def->name);
+            return -1;
+        }
+
+        if (other->def->dom) {
+            if (def->dom) {
+                if (!virDomainDefCheckABIStability(other->def->dom,
+                                                   def->dom, xmlopt))
+                    return -1;
+            } else {
+                /* Transfer the domain def */
+                def->dom = other->def->dom;
+                other->def->dom = NULL;
+            }
+        }
+    }
+
+    if (def->dom) {
+        if (external) {
+            align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
+            align_match = false;
+        }
+        if (virDomainSnapshotAlignDisks(def, align_location,
+                                        align_match) < 0)
+            return -1;
+    }
+
+
+    return 0;
+}
+
+
 /**
  * virDomainSnapshotDefAssignExternalNames:
  * @def: snapshot def object
@@ -1325,12 +1406,8 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
                               unsigned int flags)
 {
     virDomainSnapshotDefPtr def = *defptr;
-    int ret = -1;
-    int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
-    bool align_match = true;
     virDomainSnapshotObjPtr other;
-    bool external = def->state == VIR_SNAP_STATE_DISK_SNAPSHOT ||
-        virDomainSnapshotDefIsExternal(def);
+    bool check_stolen;

     /* Prevent circular chains */
     if (def->parent) {
@@ -1338,21 +1415,21 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
             virReportError(VIR_ERR_INVALID_ARG,
                            _("cannot set snapshot %s as its own parent"),
                            def->name);
-            goto cleanup;
+            return -1;
         }
         other = virDomainSnapshotFindByName(vm->snapshots, def->parent);
         if (!other) {
             virReportError(VIR_ERR_INVALID_ARG,
                            _("parent %s for snapshot %s not found"),
                            def->parent, def->name);
-            goto cleanup;
+            return -1;
         }
         while (other->def->parent) {
             if (STREQ(other->def->parent, def->name)) {
                 virReportError(VIR_ERR_INVALID_ARG,
                                _("parent %s would create cycle to %s"),
                                other->def->name, def->name);
-                goto cleanup;
+                return -1;
             }
             other = virDomainSnapshotFindByName(vm->snapshots,
                                                 other->def->parent);
@@ -1364,77 +1441,18 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
         }
     }

-    /* Check that any replacement is compatible */
-    if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) && !external) {
-        virReportError(VIR_ERR_INVALID_ARG,
-                       _("disk-only flag for snapshot %s requires "
-                         "disk-snapshot state"),
-                       def->name);
-        goto cleanup;
-    }
-
-    if (def->dom &&
-        memcmp(def->dom->uuid, domain->uuid, VIR_UUID_BUFLEN)) {
-        char uuidstr[VIR_UUID_STRING_BUFLEN];
-
-        virUUIDFormat(domain->uuid, uuidstr);
-        virReportError(VIR_ERR_INVALID_ARG,
-                       _("definition for snapshot %s must use uuid %s"),
-                       def->name, uuidstr);
-        goto cleanup;
-    }
-
     other = virDomainSnapshotFindByName(vm->snapshots, def->name);
+    check_stolen = other && other->def->dom;
+    if (virDomainSnapshotRedefineValidate(def, domain->uuid, other, xmlopt,
+                                          flags) < 0) {
+        /* revert stealing of the snapshot domain definition */
+        if (check_stolen && def->dom && !other->def->dom) {
+            other->def->dom = def->dom;
+            def->dom = NULL;
+        }
+        return -1;
+    }
     if (other) {
-        if ((other->def->state == VIR_SNAP_STATE_RUNNING ||
-             other->def->state == VIR_SNAP_STATE_PAUSED) !=
-            (def->state == VIR_SNAP_STATE_RUNNING ||
-             def->state == VIR_SNAP_STATE_PAUSED)) {
-            virReportError(VIR_ERR_INVALID_ARG,
-                           _("cannot change between online and offline "
-                             "snapshot state in snapshot %s"),
-                           def->name);
-            goto cleanup;
-        }
-
-        if ((other->def->state == VIR_SNAP_STATE_DISK_SNAPSHOT) !=
-            (def->state == VIR_SNAP_STATE_DISK_SNAPSHOT)) {
-            virReportError(VIR_ERR_INVALID_ARG,
-                           _("cannot change between disk only and "
-                             "full system in snapshot %s"),
-                           def->name);
-            goto cleanup;
-        }
-
-        if (other->def->dom) {
-            if (def->dom) {
-                if (!virDomainDefCheckABIStability(other->def->dom,
-                                                   def->dom, xmlopt))
-                    goto cleanup;
-            } else {
-                /* Transfer the domain def */
-                def->dom = other->def->dom;
-                other->def->dom = NULL;
-            }
-        }
-
-        if (def->dom) {
-            if (external) {
-                align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
-                align_match = false;
-            }
-
-            if (virDomainSnapshotAlignDisks(def, align_location,
-                                            align_match) < 0) {
-                /* revert stealing of the snapshot domain definition */
-                if (def->dom && !other->def->dom) {
-                    other->def->dom = def->dom;
-                    def->dom = NULL;
-                }
-                goto cleanup;
-            }
-        }
-
         if (other == vm->current_snapshot) {
             *update_current = true;
             vm->current_snapshot = NULL;
@@ -1447,19 +1465,7 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
         other->def = def;
         *defptr = NULL;
         *snap = other;
-    } else {
-        if (def->dom) {
-            if (external) {
-                align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
-                align_match = false;
-            }
-            if (virDomainSnapshotAlignDisks(def, align_location,
-                                            align_match) < 0)
-                goto cleanup;
-        }
     }

-    ret = 0;
- cleanup:
-    return ret;
+    return 0;
 }
-- 
2.20.1

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux