[PATCH partially-for-8.0 00/17] qemu: Fix use-after free when redefining snapshots and cleanup the code
Patches 1 and 2 should be pushed for 8.0 as the bug was introduced in this dev cycle and the patches are specifically kept very simple. The rest of the series refactors the snapshot validation and helper code to have less weird semantics which lead to this bug. Peter Krempa (17): qemuSnapshotRedefine: Rename 'def' to 'snapdef' qemuSnapshotRedefine: Fix use of snapshot definition after free virDomainMomentAssignDef: Simplify error handling virDomainSnapshotRedefineValidate: Fix validation of VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY flag virDomainSnapshotAlignDisks: Improve function comment virDomainSnapshotAlignDisks: Convert @default_snapshot to virDomainSnapshotLocation virDomainSnapshotAlignDisks: Move 'require_match' selection logic inside virDomainSnapshotAlignDisks: Allow alternate domain definition when redefining virDomainSnapshotRedefineValidate: Unexport virDomainSnapshotRedefinePrep: Use 'snapdef' for snapshot definition object virDomainSnapshotRedefineValidate: Don't modify the snapshot definition testDomainSnapshotCreateXML: Extract snapshot redefinition code qemuSnapshotCreate: Use 'snapdef' instead of 'def' qemuSnapshotCreate: Standardize handling of the reference on @snapdef qemuDomainSnapshotLoad: Refactor handling of snapshot definition object virDomainSnapshotAssignDef: Clear second argument when it is consumed virDomainSnapshotRedefinePrep: Don't do partial redefine src/conf/snapshot_conf.c | 120 +++++++++++++++------------- src/conf/snapshot_conf.h | 13 +-- src/conf/virdomainmomentobjlist.c | 9 +-- src/conf/virdomainsnapshotobjlist.c | 29 ++++++- src/conf/virdomainsnapshotobjlist.h | 5 +- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 18 ++--- src/qemu/qemu_snapshot.c | 35 ++++---- src/test/test_driver.c | 89 ++++++++++++--------- src/vz/vz_sdk.c | 3 +- 10 files changed, 180 insertions(+), 142 deletions(-) -- 2.31.1
'def' is commonly used to refer to domain definition. Most of the snapshot code uses 'snapdef' for the snapshot definition. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 5d383279b0..624ace0314 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1709,7 +1709,7 @@ qemuSnapshotCreateWriteMetadata(virDomainObj *vm, static virDomainSnapshotPtr qemuSnapshotRedefine(virDomainObj *vm, virDomainPtr domain, - virDomainSnapshotDef *def, + virDomainSnapshotDef *snapdef, virQEMUDriver *driver, virQEMUDriverConfig *cfg, unsigned int flags) @@ -1717,13 +1717,13 @@ qemuSnapshotRedefine(virDomainObj *vm, virDomainMomentObj *snap = NULL; virDomainSnapshotPtr ret = NULL; - if (virDomainSnapshotRedefinePrep(vm, &def, &snap, + if (virDomainSnapshotRedefinePrep(vm, &snapdef, &snap, driver->xmlopt, flags) < 0) return NULL; if (!snap) { - if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, def))) + if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, snapdef))) return NULL; } /* XXX Should we validate that the redefined snapshot even -- 2.31.1
On a Wednesday in 2022, Peter Krempa wrote:
'def' is commonly used to refer to domain definition. Most of the snapshot code uses 'snapdef' for the snapshot definition.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Commit f4aae9726df factored out the snapshot redefinition code into a separate function, but didn't account for the fact that the code is consuming the reference to the snapshot definition and by moving the code away the caller (qemuSnapshotCreateXML) now frees the definition which didn't happen before as we cleared the pointer. Fix it by increasing the reference locally. Later patches will refactor the code so that it's more obvious what's happening. Fixes: f4aae9726df Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2039651 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 624ace0314..f92e00f9c0 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1709,13 +1709,14 @@ qemuSnapshotCreateWriteMetadata(virDomainObj *vm, static virDomainSnapshotPtr qemuSnapshotRedefine(virDomainObj *vm, virDomainPtr domain, - virDomainSnapshotDef *snapdef, + virDomainSnapshotDef *snapdeftmp, virQEMUDriver *driver, virQEMUDriverConfig *cfg, unsigned int flags) { virDomainMomentObj *snap = NULL; virDomainSnapshotPtr ret = NULL; + g_autoptr(virDomainSnapshotDef) snapdef = virObjectRef(snapdeftmp); if (virDomainSnapshotRedefinePrep(vm, &snapdef, &snap, driver->xmlopt, @@ -1725,6 +1726,7 @@ qemuSnapshotRedefine(virDomainObj *vm, if (!snap) { if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, snapdef))) return NULL; + snapdef = NULL; } /* XXX Should we validate that the redefined snapshot even * makes sense, such as checking that qemu-img recognizes the -- 2.31.1
On a Wednesday in 2022, Peter Krempa wrote:
Commit f4aae9726df factored out the snapshot redefinition code into a separate function, but didn't account for the fact that the code is consuming the reference to the snapshot definition and by moving the code away the caller (qemuSnapshotCreateXML) now frees the definition which didn't happen before as we cleared the pointer.
Fix it by increasing the reference locally. Later patches will refactor the code so that it's more obvious what's happening.
Fixes: f4aae9726df Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2039651 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Remove error handling from the call to 'virDomainMomentObjNew' as it can't return NULL and replace 'virHashAddEntry' by 'g_hash_table_insert' as we've already checked that snapshot with such name doesn't exist in the hash table. This removes handling for two impossible errors. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/virdomainmomentobjlist.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c index 8993c2310b..2f2467d13f 100644 --- a/src/conf/virdomainmomentobjlist.c +++ b/src/conf/virdomainmomentobjlist.c @@ -245,14 +245,9 @@ virDomainMomentAssignDef(virDomainMomentObjList *moments, return NULL; } - if (!(moment = virDomainMomentObjNew())) - return NULL; - - if (virHashAddEntry(moments->objs, def->name, moment) < 0) { - VIR_FREE(moment); - return NULL; - } + moment = virDomainMomentObjNew(); moment->def = def; + g_hash_table_insert(moments->objs, g_strdup(def->name), moment); return moment; } -- 2.31.1
External snapshot with memory is created without using the VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY flag, but rather with properly configuring the XML. When redefining the code should be checking the same thing as by definition an external snapshot with memory is not a disk-only snapshot. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/snapshot_conf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index fc6f0a859d..d46d9bd335 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -478,7 +478,8 @@ virDomainSnapshotRedefineValidate(virDomainSnapshotDef *def, bool external = def->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT || virDomainSnapshotDefIsExternal(def); - if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) && !external) { + if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) && + def->state != VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT) { virReportError(VIR_ERR_INVALID_ARG, _("disk-only flag for snapshot %s requires " "disk-snapshot state"), -- 2.31.1
Add description of arguments, reword the description for clarity, and fix improper argument names mentioned in the existing description. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/snapshot_conf.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index d46d9bd335..d7e18596bc 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -622,13 +622,22 @@ virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDef *def) } -/* Align def->disks to def->parent.dom. Sort the list of def->disks, - * filling in any missing disks or snapshot state defaults given by - * the domain, with a fallback to a passed in default. Convert paths - * to disk targets for uniformity. Issue an error and return -1 if - * any def->disks[n]->name appears more than once or does not map to - * dom->disks. If require_match, also ensure that there is no - * conflicting requests for both internal and external snapshots. */ +/** + * virDomainSnapshotAlignDisks: + * @snapdef: Snapshot definition to align + * @default_snapshot: snapshot location to assign to disks which don't have any + * @require_match: Require that all disks use the same snapshot mode + * + * Align snapdef->disks to snapdef->parent.dom, filling in any missing disks or + * snapshot state defaults given by the domain, with a fallback to + * @default_snapshot. Ensure that there are no duplicate snapshot disk + * definitions in @snapdef and there are no disks described in @snapdef but + * missing from the domain definition. + * + * Convert paths to disk targets for uniformity. + * + * On error -1 is returned and a libvirt error is reported. + */ int virDomainSnapshotAlignDisks(virDomainSnapshotDef *snapdef, int default_snapshot, -- 2.31.1
Use the appropriate type for the variable and fix all callers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/snapshot_conf.c | 4 ++-- src/conf/snapshot_conf.h | 2 +- src/qemu/qemu_snapshot.c | 2 +- src/test/test_driver.c | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index d7e18596bc..f0ded7919c 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -473,7 +473,7 @@ virDomainSnapshotRedefineValidate(virDomainSnapshotDef *def, virDomainXMLOption *xmlopt, unsigned int flags) { - int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; + virDomainSnapshotLocation align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; bool align_match = true; bool external = def->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT || virDomainSnapshotDefIsExternal(def); @@ -640,7 +640,7 @@ virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDef *def) */ int virDomainSnapshotAlignDisks(virDomainSnapshotDef *snapdef, - int default_snapshot, + virDomainSnapshotLocation default_snapshot, bool require_match) { virDomainDef *domdef = snapdef->parent.dom; diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index a3ec0cd410..e5d4d0fc45 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -126,7 +126,7 @@ char *virDomainSnapshotDefFormat(const char *uuidstr, virDomainXMLOption *xmlopt, unsigned int flags); int virDomainSnapshotAlignDisks(virDomainSnapshotDef *snapshot, - int default_snapshot, + virDomainSnapshotLocation default_snapshot, bool require_match); bool virDomainSnapshotDefIsExternal(virDomainSnapshotDef *def); diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index f92e00f9c0..51df0ed8df 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1630,7 +1630,7 @@ qemuSnapshotCreateAlignDisks(virDomainObj *vm, { g_autofree char *xml = NULL; qemuDomainObjPrivate *priv = vm->privateData; - int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; + virDomainSnapshotLocation align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; bool align_match = true; /* Easiest way to clone inactive portion of vm->def is via diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 0e93b79922..2ff56cf03f 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -8717,7 +8717,7 @@ testDomainSnapshotAlignDisks(virDomainObj *vm, virDomainSnapshotDef *def, unsigned int flags) { - int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; + virDomainSnapshotLocation align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; bool align_match = true; if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { -- 2.31.1
'require_match' set to true is only needed for internal snapshots taken by hypervisors (qemu) which don't have a way to control which disks take part in the snapshot (savevm). To de-clutter callers we can change the argument to mean 'this code path requires uniform snapshot for internal snapshots'. Change the argument and fix the callers. For now all callers pass 'true' but any new hypervisor or even usage in qemu is not going to share the limitation. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/snapshot_conf.c | 24 ++++++++++++++++-------- src/conf/snapshot_conf.h | 2 +- src/qemu/qemu_snapshot.c | 5 +---- src/test/test_driver.c | 5 +---- 4 files changed, 19 insertions(+), 17 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index f0ded7919c..43f984912e 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -474,7 +474,6 @@ virDomainSnapshotRedefineValidate(virDomainSnapshotDef *def, unsigned int flags) { virDomainSnapshotLocation align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; - bool align_match = true; bool external = def->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT || virDomainSnapshotDefIsExternal(def); @@ -533,12 +532,10 @@ virDomainSnapshotRedefineValidate(virDomainSnapshotDef *def, } if (def->parent.dom) { - if (external) { + if (external) align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; - align_match = false; - } - if (virDomainSnapshotAlignDisks(def, align_location, - align_match) < 0) + + if (virDomainSnapshotAlignDisks(def, align_location, true) < 0) return -1; } @@ -626,7 +623,8 @@ virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDef *def) * virDomainSnapshotAlignDisks: * @snapdef: Snapshot definition to align * @default_snapshot: snapshot location to assign to disks which don't have any - * @require_match: Require that all disks use the same snapshot mode + * @uniform_internal_snapshot: Require that for an internal snapshot all disks + * take part in the internal snapshot * * Align snapdef->disks to snapdef->parent.dom, filling in any missing disks or * snapshot state defaults given by the domain, with a fallback to @@ -634,6 +632,11 @@ virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDef *def) * definitions in @snapdef and there are no disks described in @snapdef but * missing from the domain definition. * + * When @uniform_internal_snapshot is true and @default_snapshot is + * VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL, all disks in @snapdef must take part + * in the internal snapshot. This is for hypervisors where granularity of an + * internal snapshot can't be controlled. + * * Convert paths to disk targets for uniformity. * * On error -1 is returned and a libvirt error is reported. @@ -641,11 +644,12 @@ virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDef *def) int virDomainSnapshotAlignDisks(virDomainSnapshotDef *snapdef, virDomainSnapshotLocation default_snapshot, - bool require_match) + bool uniform_internal_snapshot) { virDomainDef *domdef = snapdef->parent.dom; g_autoptr(GHashTable) map = virHashNew(NULL); g_autofree virDomainSnapshotDiskDef *olddisks = NULL; + bool require_match = false; size_t oldndisks; size_t i; @@ -661,6 +665,10 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDef *snapdef, return -1; } + if (uniform_internal_snapshot && + default_snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL) + require_match = true; + /* Unlikely to have a guest without disks but technically possible. */ if (!domdef->ndisks) return 0; diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index e5d4d0fc45..505c9f785d 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -127,7 +127,7 @@ char *virDomainSnapshotDefFormat(const char *uuidstr, unsigned int flags); int virDomainSnapshotAlignDisks(virDomainSnapshotDef *snapshot, virDomainSnapshotLocation default_snapshot, - bool require_match); + bool uniform_internal_snapshot); bool virDomainSnapshotDefIsExternal(virDomainSnapshotDef *def); bool virDomainSnapshotIsExternal(virDomainMomentObj *snap); diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 51df0ed8df..48fa19cebd 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1631,7 +1631,6 @@ qemuSnapshotCreateAlignDisks(virDomainObj *vm, g_autofree char *xml = NULL; qemuDomainObjPrivate *priv = vm->privateData; virDomainSnapshotLocation align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; - bool align_match = true; /* Easiest way to clone inactive portion of vm->def is via * conversion in and back out of xml. */ @@ -1653,7 +1652,6 @@ qemuSnapshotCreateAlignDisks(virDomainObj *vm, if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; - align_match = false; if (virDomainObjIsActive(vm)) def->state = VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT; else @@ -1662,7 +1660,6 @@ qemuSnapshotCreateAlignDisks(virDomainObj *vm, } else if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { def->state = virDomainObjGetState(vm, NULL); align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; - align_match = false; } else { def->state = virDomainObjGetState(vm, NULL); @@ -1678,7 +1675,7 @@ qemuSnapshotCreateAlignDisks(virDomainObj *vm, VIR_DOMAIN_SNAPSHOT_LOCATION_NONE : VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL); } - if (virDomainSnapshotAlignDisks(def, align_location, align_match) < 0) + if (virDomainSnapshotAlignDisks(def, align_location, true) < 0) return -1; return 0; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 2ff56cf03f..44f06530b5 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -8718,11 +8718,9 @@ testDomainSnapshotAlignDisks(virDomainObj *vm, unsigned int flags) { virDomainSnapshotLocation align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; - bool align_match = true; if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; - align_match = false; if (virDomainObjIsActive(vm)) def->state = VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT; else @@ -8731,7 +8729,6 @@ testDomainSnapshotAlignDisks(virDomainObj *vm, } else if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { def->state = virDomainObjGetState(vm, NULL); align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; - align_match = false; } else { def->state = virDomainObjGetState(vm, NULL); def->memory = def->state == VIR_DOMAIN_SNAPSHOT_SHUTOFF ? @@ -8739,7 +8736,7 @@ testDomainSnapshotAlignDisks(virDomainObj *vm, VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; } - return virDomainSnapshotAlignDisks(def, align_location, align_match); + return virDomainSnapshotAlignDisks(def, align_location, true); } static virDomainSnapshotPtr -- 2.31.1
Due to historical reasons we allow users to redefine an existing snapshot without providing the domain definition which would correspond to it. In such case we'd use the domain definition from the snapshot that is being redefined. To prevent callers from doing complex moving of the domain definition object back and forth between the snapshot definitions we can add an argument to virDomainSnapshotAlignDisks which will allow us to pass in the alternate definition if the one from the snapshot is missing. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/snapshot_conf.c | 14 ++++++++++++-- src/conf/snapshot_conf.h | 1 + src/qemu/qemu_snapshot.c | 2 +- src/test/test_driver.c | 2 +- 4 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 43f984912e..be5deadc8c 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -535,7 +535,7 @@ virDomainSnapshotRedefineValidate(virDomainSnapshotDef *def, if (external) align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; - if (virDomainSnapshotAlignDisks(def, align_location, true) < 0) + if (virDomainSnapshotAlignDisks(def, NULL, align_location, true) < 0) return -1; } @@ -622,16 +622,22 @@ virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDef *def) /** * virDomainSnapshotAlignDisks: * @snapdef: Snapshot definition to align + * @existingDomainDef: definition of the domain belonging to a redefined snapshot * @default_snapshot: snapshot location to assign to disks which don't have any * @uniform_internal_snapshot: Require that for an internal snapshot all disks * take part in the internal snapshot * - * Align snapdef->disks to snapdef->parent.dom, filling in any missing disks or + * Align snapdef->disks to domain definition, filling in any missing disks or * snapshot state defaults given by the domain, with a fallback to * @default_snapshot. Ensure that there are no duplicate snapshot disk * definitions in @snapdef and there are no disks described in @snapdef but * missing from the domain definition. * + * By default the domain definition from @snapdef->parent.dom is used, but when + * redefining an existing snapshot the domain definition may be omitted in + * @snapdef. In such case callers must pass in the definition from the snapsot + * being redefined as @existingDomainDef. In all other cases callers pass NULL. + * * When @uniform_internal_snapshot is true and @default_snapshot is * VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL, all disks in @snapdef must take part * in the internal snapshot. This is for hypervisors where granularity of an @@ -643,6 +649,7 @@ virDomainSnapshotDefAssignExternalNames(virDomainSnapshotDef *def) */ int virDomainSnapshotAlignDisks(virDomainSnapshotDef *snapdef, + virDomainDef *existingDomainDef, virDomainSnapshotLocation default_snapshot, bool uniform_internal_snapshot) { @@ -653,6 +660,9 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDef *snapdef, size_t oldndisks; size_t i; + if (!domdef) + domdef = existingDomainDef; + if (!domdef) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing domain in snapshot")); diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 505c9f785d..446d0870f4 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -126,6 +126,7 @@ char *virDomainSnapshotDefFormat(const char *uuidstr, virDomainXMLOption *xmlopt, unsigned int flags); int virDomainSnapshotAlignDisks(virDomainSnapshotDef *snapshot, + virDomainDef *existingDomainDef, virDomainSnapshotLocation default_snapshot, bool uniform_internal_snapshot); diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 48fa19cebd..797972f4d4 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1675,7 +1675,7 @@ qemuSnapshotCreateAlignDisks(virDomainObj *vm, VIR_DOMAIN_SNAPSHOT_LOCATION_NONE : VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL); } - if (virDomainSnapshotAlignDisks(def, align_location, true) < 0) + if (virDomainSnapshotAlignDisks(def, NULL, align_location, true) < 0) return -1; return 0; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 44f06530b5..dde7bf1b8e 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -8736,7 +8736,7 @@ testDomainSnapshotAlignDisks(virDomainObj *vm, VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; } - return virDomainSnapshotAlignDisks(def, align_location, true); + return virDomainSnapshotAlignDisks(def, NULL, align_location, true); } static virDomainSnapshotPtr -- 2.31.1
The function isn't used outside of src/conf/snapshot_conf.c Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/snapshot_conf.c | 2 +- src/conf/snapshot_conf.h | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index be5deadc8c..1d61f93b65 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -466,7 +466,7 @@ virDomainSnapshotDefParseString(const char *xmlStr, /* Perform sanity checking on a redefined snapshot definition. If * @other is non-NULL, this may include swapping def->parent.dom from other * into def. */ -int +static int virDomainSnapshotRedefineValidate(virDomainSnapshotDef *def, const unsigned char *domain_uuid, virDomainMomentObj *other, diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 446d0870f4..b04163efae 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -139,11 +139,5 @@ int virDomainSnapshotRedefinePrep(virDomainObj *vm, virDomainXMLOption *xmlopt, unsigned int flags); -int virDomainSnapshotRedefineValidate(virDomainSnapshotDef *def, - const unsigned char *domain_uuid, - virDomainMomentObj *other, - virDomainXMLOption *xmlopt, - unsigned int flags); - VIR_ENUM_DECL(virDomainSnapshotLocation); VIR_ENUM_DECL(virDomainSnapshotState); -- 2.31.1
On a Wednesday in 2022, Peter Krempa wrote:
The function isn't used outside of src/conf/snapshot_conf.c
Exported by 21b2651e72ee33211e057d5c3921d9af65465f8e for the code motion in 9b75154c07dc50fb50296f35543f5dba0337cbb8 but its user was later reverted in 57d252c7401f981f1caea90250d72e0f87caac19
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/snapshot_conf.c | 2 +- src/conf/snapshot_conf.h | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-)
We use this variable name to distinguish it from the domain definition. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/snapshot_conf.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 1d61f93b65..4d2cfae128 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -980,23 +980,23 @@ virDomainSnapshotRedefinePrep(virDomainObj *vm, virDomainXMLOption *xmlopt, unsigned int flags) { - virDomainSnapshotDef *def = *defptr; + virDomainSnapshotDef *snapdef = *defptr; virDomainMomentObj *other; virDomainSnapshotDef *otherdef = NULL; bool check_if_stolen; - if (virDomainSnapshotCheckCycles(vm->snapshots, def, vm->def->name) < 0) + if (virDomainSnapshotCheckCycles(vm->snapshots, snapdef, vm->def->name) < 0) return -1; - other = virDomainSnapshotFindByName(vm->snapshots, def->parent.name); + other = virDomainSnapshotFindByName(vm->snapshots, snapdef->parent.name); if (other) otherdef = virDomainSnapshotObjGetDef(other); check_if_stolen = other && otherdef->parent.dom; - if (virDomainSnapshotRedefineValidate(def, vm->def->uuid, other, xmlopt, + if (virDomainSnapshotRedefineValidate(snapdef, vm->def->uuid, other, xmlopt, flags) < 0) { /* revert any stealing of the snapshot domain definition */ - if (check_if_stolen && def->parent.dom && !otherdef->parent.dom) - otherdef->parent.dom = g_steal_pointer(&def->parent.dom); + if (check_if_stolen && snapdef->parent.dom && !otherdef->parent.dom) + otherdef->parent.dom = g_steal_pointer(&snapdef->parent.dom); return -1; } if (other) { -- 2.31.1
It is not expected that a function with 'Validate' in the name actually modifies the validated object, even worse when it even modifies another object and the ultimatively worst bit is that it doesn't undo the mess if the validation fails midway. Move the stealing of the domain definition from the definition of a snapshot being redefined into the caller along with the call to virDomainSnapshotAlignDisks. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/snapshot_conf.c | 56 +++++++++++++++++----------------------- 1 file changed, 24 insertions(+), 32 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 4d2cfae128..499fc5ad97 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -463,9 +463,7 @@ virDomainSnapshotDefParseString(const char *xmlStr, } -/* Perform sanity checking on a redefined snapshot definition. If - * @other is non-NULL, this may include swapping def->parent.dom from other - * into def. */ +/* Perform sanity checking on a redefined snapshot definition. */ static int virDomainSnapshotRedefineValidate(virDomainSnapshotDef *def, const unsigned char *domain_uuid, @@ -473,10 +471,6 @@ virDomainSnapshotRedefineValidate(virDomainSnapshotDef *def, virDomainXMLOption *xmlopt, unsigned int flags) { - virDomainSnapshotLocation align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; - bool external = def->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT || - virDomainSnapshotDefIsExternal(def); - if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) && def->state != VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT) { virReportError(VIR_ERR_INVALID_ARG, @@ -524,22 +518,10 @@ virDomainSnapshotRedefineValidate(virDomainSnapshotDef *def, if (!virDomainDefCheckABIStability(otherdef->parent.dom, def->parent.dom, xmlopt)) return -1; - } else { - /* Transfer the domain def */ - def->parent.dom = g_steal_pointer(&otherdef->parent.dom); } } } - if (def->parent.dom) { - if (external) - align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; - - if (virDomainSnapshotAlignDisks(def, NULL, align_location, true) < 0) - return -1; - } - - return 0; } @@ -982,28 +964,38 @@ virDomainSnapshotRedefinePrep(virDomainObj *vm, { virDomainSnapshotDef *snapdef = *defptr; virDomainMomentObj *other; - virDomainSnapshotDef *otherdef = NULL; - bool check_if_stolen; + virDomainSnapshotDef *otherSnapDef = NULL; + virDomainDef *otherDomDef = NULL; + virDomainSnapshotLocation align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL; if (virDomainSnapshotCheckCycles(vm->snapshots, snapdef, vm->def->name) < 0) return -1; - other = virDomainSnapshotFindByName(vm->snapshots, snapdef->parent.name); - if (other) - otherdef = virDomainSnapshotObjGetDef(other); - check_if_stolen = other && otherdef->parent.dom; - if (virDomainSnapshotRedefineValidate(snapdef, vm->def->uuid, other, xmlopt, - flags) < 0) { - /* revert any stealing of the snapshot domain definition */ - if (check_if_stolen && snapdef->parent.dom && !otherdef->parent.dom) - otherdef->parent.dom = g_steal_pointer(&snapdef->parent.dom); - return -1; + if ((other = virDomainSnapshotFindByName(vm->snapshots, snapdef->parent.name))) { + otherSnapDef = virDomainSnapshotObjGetDef(other); + otherDomDef = otherSnapDef->parent.dom; } + + if (virDomainSnapshotRedefineValidate(snapdef, vm->def->uuid, other, xmlopt, flags) < 0) + return -1; + + if (snapdef->state == VIR_DOMAIN_SNAPSHOT_DISK_SNAPSHOT || + virDomainSnapshotDefIsExternal(snapdef)) + align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + + 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(otherdef); + virObjectUnref(otherSnapDef); other->def = &(*defptr)->parent; *defptr = NULL; *snap = other; -- 2.31.1
The test driver code was copied from qemu but wasn't refactored recently. Split out the redefinition code similarly to what qemu driver did. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/test/test_driver.c | 76 +++++++++++++++++++++++++++--------------- 1 file changed, 49 insertions(+), 27 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index dde7bf1b8e..e772b2be2b 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -8739,6 +8739,33 @@ testDomainSnapshotAlignDisks(virDomainObj *vm, return virDomainSnapshotAlignDisks(def, NULL, align_location, true); } + +static virDomainSnapshotPtr +testDomainSnapshotRedefine(virDomainObj *vm, + virDomainPtr domain, + virDomainSnapshotDef *snapdeftmp, + virDomainMomentObj **snapout, + virDomainXMLOption *xmlopt, + unsigned int flags) +{ + virDomainMomentObj *snap = NULL; + g_autoptr(virDomainSnapshotDef) snapdef = virObjectRef(snapdeftmp); + + if (virDomainSnapshotRedefinePrep(vm, &snapdef, &snap, xmlopt, flags) < 0) + return NULL; + + if (!snap) { + if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, snapdef))) + return NULL; + snapdef = NULL; + } + + *snapout = snap; + + return virGetDomainSnapshot(domain, snap->def->name); +} + + static virDomainSnapshotPtr testDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, @@ -8805,37 +8832,32 @@ testDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; if (redefine) { - if (virDomainSnapshotRedefinePrep(vm, &def, &snap, - privconn->xmlopt, - flags) < 0) - goto cleanup; - } else { - if (!(def->parent.dom = virDomainDefCopy(vm->def, - privconn->xmlopt, - NULL, - true))) - goto cleanup; - - if (testDomainSnapshotAlignDisks(vm, def, flags) < 0) - goto cleanup; + snapshot = testDomainSnapshotRedefine(vm, domain, def, &snap, + privconn->xmlopt, flags); + goto cleanup; } - if (!snap) { - if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, def))) - goto cleanup; - def = NULL; - } + if (!(def->parent.dom = virDomainDefCopy(vm->def, + privconn->xmlopt, + NULL, + true))) + goto cleanup; - if (!redefine) { - snap->def->parent_name = g_strdup(virDomainSnapshotGetCurrentName(vm->snapshots)); + if (testDomainSnapshotAlignDisks(vm, def, flags) < 0) + goto cleanup; - if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT) && - virDomainObjIsActive(vm)) { - testDomainShutdownState(domain, vm, - VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT); - event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, - VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); - } + if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, def))) + goto cleanup; + def = NULL; + + snap->def->parent_name = g_strdup(virDomainSnapshotGetCurrentName(vm->snapshots)); + + if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT) && + virDomainObjIsActive(vm)) { + testDomainShutdownState(domain, vm, + VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT); + event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, + VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); } snapshot = virGetDomainSnapshot(domain, snap->def->name); -- 2.31.1
'def' is commonly used for domain definition. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 797972f4d4..5f256a77df 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1750,7 +1750,7 @@ qemuSnapshotRedefine(virDomainObj *vm, static virDomainSnapshotPtr qemuSnapshotCreate(virDomainObj *vm, virDomainPtr domain, - virDomainSnapshotDef *def, + virDomainSnapshotDef *snapdef, virQEMUDriver *driver, virQEMUDriverConfig *cfg, unsigned int flags) @@ -1760,17 +1760,17 @@ qemuSnapshotCreate(virDomainObj *vm, virDomainMomentObj *current = NULL; virDomainSnapshotPtr ret = NULL; - if (qemuSnapshotCreateAlignDisks(vm, def, driver, flags) < 0) + if (qemuSnapshotCreateAlignDisks(vm, snapdef, driver, flags) < 0) return NULL; - if (qemuSnapshotPrepare(vm, def, &flags) < 0) + if (qemuSnapshotPrepare(vm, snapdef, &flags) < 0) return NULL; if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA) { snap = tmpsnap = virDomainMomentObjNew(); - snap->def = &def->parent; + snap->def = &snapdef->parent; } else { - if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, def))) + if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, snapdef))) return NULL; if ((current = virDomainSnapshotGetCurrent(vm->snapshots))) { @@ -1778,7 +1778,7 @@ qemuSnapshotCreate(virDomainObj *vm, } } - virObjectRef(def); + virObjectRef(snapdef); /* actually do the snapshot */ if (virDomainObjIsActive(vm)) { -- 2.31.1
As with qemuSnapshotRedefine, make an extra reference in a temporary autocleaned variable and use that instead of refing the definition after it's stolen. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 5f256a77df..c5379d583e 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1750,11 +1750,12 @@ qemuSnapshotRedefine(virDomainObj *vm, static virDomainSnapshotPtr qemuSnapshotCreate(virDomainObj *vm, virDomainPtr domain, - virDomainSnapshotDef *snapdef, + virDomainSnapshotDef *snapdeftmp, virQEMUDriver *driver, virQEMUDriverConfig *cfg, unsigned int flags) { + g_autoptr(virDomainSnapshotDef) snapdef = virObjectRef(snapdeftmp); g_autoptr(virDomainMomentObj) tmpsnap = NULL; virDomainMomentObj *snap = NULL; virDomainMomentObj *current = NULL; @@ -1769,17 +1770,17 @@ qemuSnapshotCreate(virDomainObj *vm, if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA) { snap = tmpsnap = virDomainMomentObjNew(); snap->def = &snapdef->parent; + snapdef = NULL; } else { if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, snapdef))) return NULL; + snapdef = NULL; if ((current = virDomainSnapshotGetCurrent(vm->snapshots))) { snap->def->parent_name = g_strdup(current->def->name); } } - virObjectRef(snapdef); - /* actually do the snapshot */ if (virDomainObjIsActive(vm)) { if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY || -- 2.31.1
Move the variable holding the snapshot definition into the loop and use automatic clearing for it. Adjust the code for parity. Note that the clearing of 'snapdef' on success of 'virDomainSnapshotAssignDef' will be refactored in upcoming patches. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 65ac5ef367..b3588f9478 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -343,7 +343,6 @@ qemuDomainSnapshotLoad(virDomainObj *vm, g_autofree char *snapDir = NULL; g_autoptr(DIR) dir = NULL; struct dirent *entry; - virDomainSnapshotDef *def = NULL; virDomainMomentObj *snap = NULL; virDomainMomentObj *current = NULL; bool cur; @@ -367,6 +366,7 @@ qemuDomainSnapshotLoad(virDomainObj *vm, goto cleanup; while ((direrr = virDirRead(dir, &entry, NULL)) > 0) { + g_autoptr(virDomainSnapshotDef) snapdef = NULL; g_autofree char *xmlStr = NULL; g_autofree char *fullpath = NULL; @@ -384,11 +384,11 @@ qemuDomainSnapshotLoad(virDomainObj *vm, continue; } - def = virDomainSnapshotDefParseString(xmlStr, - qemu_driver->xmlopt, - priv->qemuCaps, &cur, - flags); - if (def == NULL) { + snapdef = virDomainSnapshotDefParseString(xmlStr, + qemu_driver->xmlopt, + priv->qemuCaps, &cur, + flags); + if (snapdef == NULL) { /* Nothing we can do here, skip this one */ virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to parse snapshot XML from file '%s'"), @@ -396,10 +396,10 @@ qemuDomainSnapshotLoad(virDomainObj *vm, continue; } - snap = virDomainSnapshotAssignDef(vm->snapshots, def); - if (snap == NULL) { - virObjectUnref(def); - } else if (cur) { + snap = virDomainSnapshotAssignDef(vm->snapshots, snapdef); + if (snap) + snapdef = NULL; + if (cur && snap) { if (current) virReportError(VIR_ERR_INTERNAL_ERROR, _("Too many snapshots claiming to be current for domain %s"), -- 2.31.1
Rather than callers second-guessing when the snapshot definition is assigned turn it into a double pointer and clear it on success. Fix callers to work with the new semantics. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/virdomainsnapshotobjlist.c | 10 ++++++++-- src/conf/virdomainsnapshotobjlist.h | 2 +- src/qemu/qemu_driver.c | 4 +--- src/qemu/qemu_snapshot.c | 6 ++---- src/test/test_driver.c | 12 ++++-------- src/vz/vz_sdk.c | 3 +-- 6 files changed, 17 insertions(+), 20 deletions(-) diff --git a/src/conf/virdomainsnapshotobjlist.c b/src/conf/virdomainsnapshotobjlist.c index 0ccdf31ae0..6b074d5994 100644 --- a/src/conf/virdomainsnapshotobjlist.c +++ b/src/conf/virdomainsnapshotobjlist.c @@ -42,9 +42,15 @@ struct _virDomainSnapshotObjList { virDomainMomentObj * virDomainSnapshotAssignDef(virDomainSnapshotObjList *snapshots, - virDomainSnapshotDef *def) + virDomainSnapshotDef **snapdefptr) { - return virDomainMomentAssignDef(snapshots->base, &def->parent); + virDomainSnapshotDef *snapdef = *snapdefptr; + virDomainMomentObj *ret = virDomainMomentAssignDef(snapshots->base, &snapdef->parent); + + if (ret) + *snapdefptr = NULL; + + return ret; } diff --git a/src/conf/virdomainsnapshotobjlist.h b/src/conf/virdomainsnapshotobjlist.h index eebeb9f5a3..bdbc17f6d5 100644 --- a/src/conf/virdomainsnapshotobjlist.h +++ b/src/conf/virdomainsnapshotobjlist.h @@ -30,7 +30,7 @@ virDomainSnapshotObjList *virDomainSnapshotObjListNew(void); void virDomainSnapshotObjListFree(virDomainSnapshotObjList *snapshots); virDomainMomentObj *virDomainSnapshotAssignDef(virDomainSnapshotObjList *snapshots, - virDomainSnapshotDef *def); + virDomainSnapshotDef **snapdefptr); int virDomainSnapshotObjListGetNames(virDomainSnapshotObjList *snapshots, virDomainMomentObj *from, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b3588f9478..e150b86cef 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -396,9 +396,7 @@ qemuDomainSnapshotLoad(virDomainObj *vm, continue; } - snap = virDomainSnapshotAssignDef(vm->snapshots, snapdef); - if (snap) - snapdef = NULL; + snap = virDomainSnapshotAssignDef(vm->snapshots, &snapdef); if (cur && snap) { if (current) virReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index c5379d583e..3e35ff5463 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -1721,9 +1721,8 @@ qemuSnapshotRedefine(virDomainObj *vm, return NULL; if (!snap) { - if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, snapdef))) + if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, &snapdef))) return NULL; - snapdef = NULL; } /* XXX Should we validate that the redefined snapshot even * makes sense, such as checking that qemu-img recognizes the @@ -1772,9 +1771,8 @@ qemuSnapshotCreate(virDomainObj *vm, snap->def = &snapdef->parent; snapdef = NULL; } else { - if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, snapdef))) + if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, &snapdef))) return NULL; - snapdef = NULL; if ((current = virDomainSnapshotGetCurrent(vm->snapshots))) { snap->def->parent_name = g_strdup(current->def->name); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index e772b2be2b..14617d4f0d 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -968,7 +968,7 @@ testParseDomainSnapshots(testDriver *privconn, for (i = 0; i < nsdata->num_snap_nodes; i++) { virDomainMomentObj *snap; - virDomainSnapshotDef *def; + g_autoptr(virDomainSnapshotDef) def = NULL; xmlNodePtr node = testParseXMLDocFromFile(nodes[i], file, "domainsnapshot"); if (!node) @@ -984,10 +984,8 @@ testParseDomainSnapshots(testDriver *privconn, if (!def) return -1; - if (!(snap = virDomainSnapshotAssignDef(domobj->snapshots, def))) { - virObjectUnref(def); + if (!(snap = virDomainSnapshotAssignDef(domobj->snapshots, &def))) return -1; - } if (cur) { if (virDomainSnapshotGetCurrent(domobj->snapshots)) { @@ -8755,9 +8753,8 @@ testDomainSnapshotRedefine(virDomainObj *vm, return NULL; if (!snap) { - if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, snapdef))) + if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, &snapdef))) return NULL; - snapdef = NULL; } *snapout = snap; @@ -8846,9 +8843,8 @@ testDomainSnapshotCreateXML(virDomainPtr domain, if (testDomainSnapshotAlignDisks(vm, def, flags) < 0) goto cleanup; - if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, def))) + if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, &def))) goto cleanup; - def = NULL; snap->def->parent_name = g_strdup(virDomainSnapshotGetCurrentName(vm->snapshots)); diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 5ed33902fd..94c6cd5c7a 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -4661,9 +4661,8 @@ prlsdkParseSnapshotTree(const char *treexml) } VIR_FREE(xmlstr); - if (!(snapshot = virDomainSnapshotAssignDef(snapshots, def))) + if (!(snapshot = virDomainSnapshotAssignDef(snapshots, &def))) goto cleanup; - def = NULL; xmlstr = virXPathString("string(./@current)", ctxt); if (xmlstr && STREQ("yes", xmlstr)) { -- 2.31.1
'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> --- 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
On a Wednesday in 2022, Peter Krempa wrote:
Patches 1 and 2 should be pushed for 8.0 as the bug was introduced in this dev cycle and the patches are specifically kept very simple.
The rest of the series refactors the snapshot validation and helper code to have less weird semantics which lead to this bug.
Peter Krempa (17): qemuSnapshotRedefine: Rename 'def' to 'snapdef' qemuSnapshotRedefine: Fix use of snapshot definition after free virDomainMomentAssignDef: Simplify error handling virDomainSnapshotRedefineValidate: Fix validation of VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY flag virDomainSnapshotAlignDisks: Improve function comment virDomainSnapshotAlignDisks: Convert @default_snapshot to virDomainSnapshotLocation virDomainSnapshotAlignDisks: Move 'require_match' selection logic inside virDomainSnapshotAlignDisks: Allow alternate domain definition when redefining virDomainSnapshotRedefineValidate: Unexport virDomainSnapshotRedefinePrep: Use 'snapdef' for snapshot definition object virDomainSnapshotRedefineValidate: Don't modify the snapshot definition testDomainSnapshotCreateXML: Extract snapshot redefinition code qemuSnapshotCreate: Use 'snapdef' instead of 'def' qemuSnapshotCreate: Standardize handling of the reference on @snapdef qemuDomainSnapshotLoad: Refactor handling of snapshot definition object virDomainSnapshotAssignDef: Clear second argument when it is consumed virDomainSnapshotRedefinePrep: Don't do partial redefine
src/conf/snapshot_conf.c | 120 +++++++++++++++------------- src/conf/snapshot_conf.h | 13 +-- src/conf/virdomainmomentobjlist.c | 9 +-- src/conf/virdomainsnapshotobjlist.c | 29 ++++++- src/conf/virdomainsnapshotobjlist.h | 5 +- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 18 ++--- src/qemu/qemu_snapshot.c | 35 ++++---- src/test/test_driver.c | 89 ++++++++++++--------- src/vz/vz_sdk.c | 3 +- 10 files changed, 180 insertions(+), 142 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa