qemu: snapshot: Break out redefine preparation to shared function

src/conf/snapshot_conf.c

@@ -1099,3 +1099,153 @@ virDomainSnapshotIsExternal(virDomainSnapshotObjPtr snap)
 {
     return virDomainSnapshotDefIsExternal(snap->def);
 }
+
+int
+virDomainSnapshotRedefinePrep(virDomainPtr domain,
+                              virDomainObjPtr vm,
+                              virDomainSnapshotDefPtr *defptr,
+                              virDomainSnapshotObjPtr *snap,
+                              bool *update_current,
+                              unsigned int flags)
+{
+    virDomainSnapshotDefPtr def = *defptr;
+    int ret = -1;
+    int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
+    int align_match = true;
+    char uuidstr[VIR_UUID_STRING_BUFLEN];
+    virDomainSnapshotObjPtr other;
+
+    virUUIDFormat(domain->uuid, uuidstr);
+
+    /* Prevent circular chains */
+    if (def->parent) {
+        if (STREQ(def->name, def->parent)) {
+            virReportError(VIR_ERR_INVALID_ARG,
+                           _("cannot set snapshot %s as its own parent"),
+                           def->name);
+            goto cleanup;
+        }
+        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;
+        }
+        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;
+            }
+            other = virDomainSnapshotFindByName(vm->snapshots,
+                                                other->def->parent);
+            if (!other) {
+                VIR_WARN("snapshots are inconsistent for %s",
+                         vm->def->name);
+                break;
+            }
+        }
+    }
+
+    /* Check that any replacement is compatible */
+    if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) &&
+        def->state != VIR_DOMAIN_DISK_SNAPSHOT) {
+        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)) {
+        virReportError(VIR_ERR_INVALID_ARG,
+                       _("definition for snapshot %s must use uuid %s"),
+                       def->name, uuidstr);
+        goto cleanup;
+    }
+
+    other = virDomainSnapshotFindByName(vm->snapshots, def->name);
+    if (other) {
+        if ((other->def->state == VIR_DOMAIN_RUNNING ||
+             other->def->state == VIR_DOMAIN_PAUSED) !=
+            (def->state == VIR_DOMAIN_RUNNING ||
+             def->state == VIR_DOMAIN_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_DOMAIN_DISK_SNAPSHOT) !=
+            (def->state == VIR_DOMAIN_DISK_SNAPSHOT)) {
+            virReportError(VIR_ERR_INVALID_ARG,
+                           _("cannot change between disk snapshot and "
+                             "system checkpoint in snapshot %s"),
+                           def->name);
+            goto cleanup;
+        }
+
+        if (other->def->dom) {
+            if (def->dom) {
+                if (!virDomainDefCheckABIStability(other->def->dom,
+                                                   def->dom))
+                    goto cleanup;
+            } else {
+                /* Transfer the domain def */
+                def->dom = other->def->dom;
+                other->def->dom = NULL;
+            }
+        }
+
+        if (def->dom) {
+            if (def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
+                virDomainSnapshotDefIsExternal(def)) {
+                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;
+        }
+
+        /* Drop and rebuild the parent relationship, but keep all
+         * child relations by reusing snap.  */
+        virDomainSnapshotDropParent(other);
+        virDomainSnapshotDefFree(other->def);
+        other->def = def;
+        *defptr = NULL;
+        *snap = other;
+    } else {
+        if (def->dom) {
+            if (def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
+                def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_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;
+}

src/conf/snapshot_conf.h

@@ -176,6 +176,13 @@ int virDomainListSnapshots(virDomainSnapshotObjListPtr snapshots,
 bool virDomainSnapshotDefIsExternal(virDomainSnapshotDefPtr def);
 bool virDomainSnapshotIsExternal(virDomainSnapshotObjPtr snap);
 
+int virDomainSnapshotRedefinePrep(virDomainPtr domain,
+                                  virDomainObjPtr vm,
+                                  virDomainSnapshotDefPtr *def,
+                                  virDomainSnapshotObjPtr *snap,
+                                  bool *update_current,
+                                  unsigned int flags);
+
 VIR_ENUM_DECL(virDomainSnapshotLocation)
 VIR_ENUM_DECL(virDomainSnapshotState)
 

src/libvirt_private.syms

@@ -652,6 +652,7 @@ virDomainSnapshotLocationTypeToString;
 virDomainSnapshotObjListGetNames;
 virDomainSnapshotObjListNum;
 virDomainSnapshotObjListRemove;
+virDomainSnapshotRedefinePrep;
 virDomainSnapshotStateTypeFromString;
 virDomainSnapshotStateTypeToString;
 virDomainSnapshotUpdateRelations;

src/qemu/qemu_driver.c

@@ -12272,7 +12272,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
     char *xml = NULL;
     virDomainSnapshotObjPtr snap = NULL;
     virDomainSnapshotPtr snapshot = NULL;
-    char uuidstr[VIR_UUID_STRING_BUFLEN];
     virDomainSnapshotDefPtr def = NULL;
     bool update_current = true;
     bool redefine = flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE;
@@ -12306,8 +12305,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
     if (redefine)
         parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE;
 
-    virUUIDFormat(domain->uuid, uuidstr);
-
     if (!(vm = qemuDomObjFromDomain(domain)))
         goto cleanup;
 
@@ -12376,133 +12373,9 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
     }
 
     if (redefine) {
-        /* Prevent circular chains */
+        if (!virDomainSnapshotRedefinePrep(domain, vm, &def, &snap,
-        if (def->parent) {
+                                           &update_current, flags) < 0)
-            if (STREQ(def->name, def->parent)) {
-                virReportError(VIR_ERR_INVALID_ARG,
-                               _("cannot set snapshot %s as its own parent"),
-                               def->name);
-                goto cleanup;
-            }
-            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;
-            }
-            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;
-                }
-                other = virDomainSnapshotFindByName(vm->snapshots,
-                                                    other->def->parent);
-                if (!other) {
-                    VIR_WARN("snapshots are inconsistent for %s",
-                             vm->def->name);
-                    break;
-                }
-            }
-        }
-
-        /* Check that any replacement is compatible */
-        if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) &&
-            def->state != VIR_DOMAIN_DISK_SNAPSHOT) {
-            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)) {
-            virReportError(VIR_ERR_INVALID_ARG,
-                           _("definition for snapshot %s must use uuid %s"),
-                           def->name, uuidstr);
-            goto cleanup;
-        }
-
-        other = virDomainSnapshotFindByName(vm->snapshots, def->name);
-        if (other) {
-            if ((other->def->state == VIR_DOMAIN_RUNNING ||
-                 other->def->state == VIR_DOMAIN_PAUSED) !=
-                (def->state == VIR_DOMAIN_RUNNING ||
-                 def->state == VIR_DOMAIN_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_DOMAIN_DISK_SNAPSHOT) !=
-                (def->state == VIR_DOMAIN_DISK_SNAPSHOT)) {
-                virReportError(VIR_ERR_INVALID_ARG,
-                               _("cannot change between disk snapshot and "
-                                 "system checkpoint in snapshot %s"),
-                               def->name);
-                goto cleanup;
-            }
-
-            if (other->def->dom) {
-                if (def->dom) {
-                    if (!virDomainDefCheckABIStability(other->def->dom,
-                                                       def->dom))
-                        goto cleanup;
-                } else {
-                    /* Transfer the domain def */
-                    def->dom = other->def->dom;
-                    other->def->dom = NULL;
-                }
-            }
-
-            if (def->dom) {
-                if (def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
-                    virDomainSnapshotDefIsExternal(def)) {
-                    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;
-            }
-
-            /* Drop and rebuild the parent relationship, but keep all
-             * child relations by reusing snap.  */
-            virDomainSnapshotDropParent(other);
-            virDomainSnapshotDefFree(other->def);
-            other->def = def;
-            def = NULL;
-            snap = other;
-        } else {
-            if (def->dom) {
-                if (def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
-                    def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
-                    align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
-                    align_match = false;
-                }
-                if (virDomainSnapshotAlignDisks(def, align_location,
-                                                align_match) < 0)
-                    goto cleanup;
-            }
-        }
     } else {
         /* Easiest way to clone inactive portion of vm->def is via
          * conversion in and back out of xml.  */