From bec2a922bd2c53262236d2e90c59c83f737ba1ef Mon Sep 17 00:00:00 2001
From: Peter Krempa <pkrempa@redhat.com>
Date: Wed, 12 Jan 2022 15:08:25 +0100
Subject: [PATCH] virDomainSnapshotRedefineValidate: Don't modify the snapshot
 definition
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

It is not expected that a function with 'Validate' in the name actually
modifies the validated object, even worse when it even modifies another
object and the ultimatively worst bit is that it doesn't undo the mess
if the validation fails midway.

Move the stealing of the domain definition from the definition of a
snapshot being redefined into the caller along with the call to
virDomainSnapshotAlignDisks.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
---
 src/conf/snapshot_conf.c | 56 +++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 32 deletions(-)

diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 4d2cfae128..499fc5ad97 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -463,9 +463,7 @@ virDomainSnapshotDefParseString(const char *xmlStr,
 }
 
 
-/* Perform sanity checking on a redefined snapshot definition. If
- * @other is non-NULL, this may include swapping def->parent.dom from other
- * into def. */
+/* Perform sanity checking on a redefined snapshot definition. */
 static int
 virDomainSnapshotRedefineValidate(virDomainSnapshotDef *def,
                                   const unsigned char *domain_uuid,
@@ -473,10 +471,6 @@ virDomainSnapshotRedefineValidate(virDomainSnapshotDef *def,
                                   virDomainXMLOption *xmlopt,
                                   unsigned int flags)
 {
-    virDomainSnapshotLocation align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
-    bool external = def->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT ||
-        virDomainSnapshotDefIsExternal(def);
-
     if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) &&
         def->state != VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT) {
         virReportError(VIR_ERR_INVALID_ARG,
@@ -524,22 +518,10 @@ virDomainSnapshotRedefineValidate(virDomainSnapshotDef *def,
                 if (!virDomainDefCheckABIStability(otherdef->parent.dom,
                                                    def->parent.dom, xmlopt))
                     return -1;
-            } else {
-                /* Transfer the domain def */
-                def->parent.dom = g_steal_pointer(&otherdef->parent.dom);
             }
         }
     }
 
-    if (def->parent.dom) {
-        if (external)
-            align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
-
-        if (virDomainSnapshotAlignDisks(def, NULL, align_location, true) < 0)
-            return -1;
-    }
-
-
     return 0;
 }
 
@@ -982,28 +964,38 @@ virDomainSnapshotRedefinePrep(virDomainObj *vm,
 {
     virDomainSnapshotDef *snapdef = *defptr;
     virDomainMomentObj *other;
-    virDomainSnapshotDef *otherdef = NULL;
-    bool check_if_stolen;
+    virDomainSnapshotDef *otherSnapDef = NULL;
+    virDomainDef *otherDomDef = NULL;
+    virDomainSnapshotLocation align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
 
     if (virDomainSnapshotCheckCycles(vm->snapshots, snapdef, vm->def->name) < 0)
         return -1;
 
-    other = virDomainSnapshotFindByName(vm->snapshots, snapdef->parent.name);
-    if (other)
-        otherdef = virDomainSnapshotObjGetDef(other);
-    check_if_stolen = other && otherdef->parent.dom;
-    if (virDomainSnapshotRedefineValidate(snapdef, vm->def->uuid, other, xmlopt,
-                                          flags) < 0) {
-        /* revert any stealing of the snapshot domain definition */
-        if (check_if_stolen && snapdef->parent.dom && !otherdef->parent.dom)
-            otherdef->parent.dom = g_steal_pointer(&snapdef->parent.dom);
-        return -1;
+    if ((other = virDomainSnapshotFindByName(vm->snapshots, snapdef->parent.name))) {
+        otherSnapDef = virDomainSnapshotObjGetDef(other);
+        otherDomDef = otherSnapDef->parent.dom;
     }
+
+    if (virDomainSnapshotRedefineValidate(snapdef, vm->def->uuid, other, xmlopt, flags) < 0)
+        return -1;
+
+    if (snapdef->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT ||
+        virDomainSnapshotDefIsExternal(snapdef))
+        align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
+
+    if (virDomainSnapshotAlignDisks(snapdef, otherDomDef, align_location, true) < 0)
+        return -1;
+
     if (other) {
+        /* steal the domain definition if redefining an existing snapshot which
+         * with a snapshot definition lacking the domain definition */
+        if (!snapdef->parent.dom)
+            snapdef->parent.dom = g_steal_pointer(&otherSnapDef->parent.dom);
+
         /* Drop and rebuild the parent relationship, but keep all
          * child relations by reusing snap. */
         virDomainMomentDropParent(other);
-        virObjectUnref(otherdef);
+        virObjectUnref(otherSnapDef);
         other->def = &(*defptr)->parent;
         *defptr = NULL;
         *snap = other;