From 717f1cc4d116b864c22908f6510dbce445b5f5eb Mon Sep 17 00:00:00 2001 From: Peter Krempa Date: Wed, 12 Jan 2022 17:00:22 +0100 Subject: [PATCH] virDomainSnapshotRedefinePrep: Don't do partial redefine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit '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 Reviewed-by: Ján Tomko --- 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; }