[PATCH 17/17] virDomainSnapshotRedefinePrep: Don't do partial redefine

[Peter Krempa]

'virDomainSnapshotRedefinePrep' does everything needed for a redefine
when the snapshot exists but not when we are defining metadata for a new
snapshot. This gives us weird semantics.

Extract the code for replacing the definition of an existing snapshot
into a new helper 'virDomainSnapshotReplaceDef' and refactor all
callers.

Signed-off-by: Peter Krempa <pkrempa at redhat.com>
---
 src/conf/snapshot_conf.c            | 20 +++-----------------
 src/conf/snapshot_conf.h            |  2 +-
 src/conf/virdomainsnapshotobjlist.c | 19 +++++++++++++++++++
 src/conf/virdomainsnapshotobjlist.h |  3 +++
 src/libvirt_private.syms            |  1 +
 src/qemu/qemu_snapshot.c            |  9 +++++----
 src/test/test_driver.c              |  6 ++++--
 7 files changed, 36 insertions(+), 24 deletions(-)

diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 499fc5ad97..2d4c7190ba 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -957,12 +957,11 @@ virDomainSnapshotIsExternal(virDomainMomentObj *snap)

 int
 virDomainSnapshotRedefinePrep(virDomainObj *vm,
-                              virDomainSnapshotDef **defptr,
+                              virDomainSnapshotDef *snapdef,
                               virDomainMomentObj **snap,
                               virDomainXMLOption *xmlopt,
                               unsigned int flags)
 {
-    virDomainSnapshotDef *snapdef = *defptr;
     virDomainMomentObj *other;
     virDomainSnapshotDef *otherSnapDef = NULL;
     virDomainDef *otherDomDef = NULL;
@@ -976,6 +975,8 @@ virDomainSnapshotRedefinePrep(virDomainObj *vm,
         otherDomDef = otherSnapDef->parent.dom;
     }

+    *snap = other;
+
     if (virDomainSnapshotRedefineValidate(snapdef, vm->def->uuid, other, xmlopt, flags) < 0)
         return -1;

@@ -986,20 +987,5 @@ virDomainSnapshotRedefinePrep(virDomainObj *vm,
     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(otherSnapDef);
-        other->def = &(*defptr)->parent;
-        *defptr = NULL;
-        *snap = other;
-    }
-
     return 0;
 }
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
index b04163efae..c8997c710c 100644
--- a/src/conf/snapshot_conf.h
+++ b/src/conf/snapshot_conf.h
@@ -134,7 +134,7 @@ bool virDomainSnapshotDefIsExternal(virDomainSnapshotDef *def);
 bool virDomainSnapshotIsExternal(virDomainMomentObj *snap);

 int virDomainSnapshotRedefinePrep(virDomainObj *vm,
-                                  virDomainSnapshotDef **def,
+                                  virDomainSnapshotDef *snapdef,
                                   virDomainMomentObj **snap,
                                   virDomainXMLOption *xmlopt,
                                   unsigned int flags);
diff --git a/src/conf/virdomainsnapshotobjlist.c b/src/conf/virdomainsnapshotobjlist.c
index 6b074d5994..2520a4bca4 100644
--- a/src/conf/virdomainsnapshotobjlist.c
+++ b/src/conf/virdomainsnapshotobjlist.c
@@ -54,6 +54,25 @@ virDomainSnapshotAssignDef(virDomainSnapshotObjList *snapshots,
 }


+void
+virDomainSnapshotReplaceDef(virDomainMomentObj *snap,
+                            virDomainSnapshotDef **snapdefptr)
+{
+    virDomainSnapshotDef *snapdef = *snapdefptr;
+    g_autoptr(virDomainSnapshotDef) origsnapdef = virDomainSnapshotObjGetDef(snap);
+
+    /* 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(&origsnapdef->parent.dom);
+
+    /* Drop and rebuild the parent relationship, but keep all child relations by reusing snap. */
+    virDomainMomentDropParent(snap);
+    snap->def = &snapdef->parent;
+    *snapdefptr = NULL;
+}
+
+
 static bool
 virDomainSnapshotFilter(virDomainMomentObj *obj,
                         unsigned int flags)
diff --git a/src/conf/virdomainsnapshotobjlist.h b/src/conf/virdomainsnapshotobjlist.h
index bdbc17f6d5..ce9d77e10b 100644
--- a/src/conf/virdomainsnapshotobjlist.h
+++ b/src/conf/virdomainsnapshotobjlist.h
@@ -32,6 +32,9 @@ void virDomainSnapshotObjListFree(virDomainSnapshotObjList *snapshots);
 virDomainMomentObj *virDomainSnapshotAssignDef(virDomainSnapshotObjList *snapshots,
                                                virDomainSnapshotDef **snapdefptr);

+void virDomainSnapshotReplaceDef(virDomainMomentObj *snap,
+                                 virDomainSnapshotDef **snapdefptr);
+
 int virDomainSnapshotObjListGetNames(virDomainSnapshotObjList *snapshots,
                                      virDomainMomentObj *from,
                                      char **const names, int maxnames,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5b76e66e61..f75dea36c4 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1212,6 +1212,7 @@ virDomainSnapshotObjListNew;
 virDomainSnapshotObjListNum;
 virDomainSnapshotObjListRemove;
 virDomainSnapshotObjListRemoveAll;
+virDomainSnapshotReplaceDef;
 virDomainSnapshotSetCurrent;
 virDomainSnapshotUpdateRelations;

diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 3e35ff5463..9cf185026c 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -1715,15 +1715,16 @@ qemuSnapshotRedefine(virDomainObj *vm,
     virDomainSnapshotPtr ret = NULL;
     g_autoptr(virDomainSnapshotDef) snapdef = virObjectRef(snapdeftmp);

-    if (virDomainSnapshotRedefinePrep(vm, &snapdef, &snap,
-                                      driver->xmlopt,
-                                      flags) < 0)
+    if (virDomainSnapshotRedefinePrep(vm, snapdef, &snap, driver->xmlopt, flags) < 0)
         return NULL;

-    if (!snap) {
+    if (snap) {
+        virDomainSnapshotReplaceDef(snap, &snapdef);
+    } else {
         if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, &snapdef)))
             return NULL;
     }
+
     /* XXX Should we validate that the redefined snapshot even
      * makes sense, such as checking that qemu-img recognizes the
      * snapshot name in at least one of the domain's disks?  */
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 14617d4f0d..1504334c30 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -8749,10 +8749,12 @@ testDomainSnapshotRedefine(virDomainObj *vm,
     virDomainMomentObj *snap = NULL;
     g_autoptr(virDomainSnapshotDef) snapdef = virObjectRef(snapdeftmp);

-    if (virDomainSnapshotRedefinePrep(vm, &snapdef, &snap, xmlopt, flags) < 0)
+    if (virDomainSnapshotRedefinePrep(vm, snapdef, &snap, xmlopt, flags) < 0)
         return NULL;

-    if (!snap) {
+    if (snap) {
+        virDomainSnapshotReplaceDef(snap, &snapdef);
+    } else {
         if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, &snapdef)))
             return NULL;
     }
-- 
2.31.1