virDomainSnapshotRedefinePrep: Don't do partial redefine

'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@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>

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;
 }

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);

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)

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,

src/libvirt_private.syms

@@ -1212,6 +1212,7 @@ virDomainSnapshotObjListNew;
 virDomainSnapshotObjListNum;
 virDomainSnapshotObjListRemove;
 virDomainSnapshotObjListRemoveAll;
+virDomainSnapshotReplaceDef;
 virDomainSnapshotSetCurrent;
 virDomainSnapshotUpdateRelations;
 

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,
+    if (virDomainSnapshotRedefinePrep(vm, snapdef, &snap, driver->xmlopt, flags) < 0)
-                                      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?  */

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;
     }