'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@xxxxxxxxxx>
---
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
