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>

diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 499fc5ad97964956c5e9a212de3e1324f4c27552..2d4c7190ba304df80ffe2d6b47c62ffef27e4ca5 100644 (file)
--- 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 b04163efae67e8b4217c8987243e8ad7893921f1..c8997c710c81c2d9b9f9b5ef3dabc55e073fc012 100644 (file)
--- 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 6b074d59941b0ff29b3844a0bd86ebe70a3a0e0c..2520a4bca464d8dee7f35f55231ec20aec22705b 100644 (file)
--- 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 bdbc17f6d54729dcd927fda361aae0789f842c06..ce9d77e10bd1f65007a24682d95257cbca3bb863 100644 (file)
--- 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 5b76e66e610efd43cd9f243aef5cea3c28ff37a8..f75dea36c403b4adb8dd7d6f0aef558a2588bb29 100644 (file)
--- 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 3e35ff54631e60066fd3056c58bdbf5f7f6b8524..9cf185026ca11698780c400cedce2cc31053cd03 100644 (file)
--- 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 14617d4f0d789c7675b9855d0e5869a3f4e2f8ee..1504334c30da9b25cfa565c19b8df836e6433b7e 100644 (file)
--- 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;
     }