[PATCH partially-for-8.0 00/17] qemu: Fix use-after free when redefining snapshots and cleanup the code
2:10 a.m.
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
2:10 a.m.
New subject: [PATCH 01/17] qemuSnapshotRedefine: Rename 'def' to 'snapdef'
'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(a)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
4:20 p.m.
New subject: [PATCH 01/17] qemuSnapshotRedefine: Rename 'def' to 'snapdef'
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(a)redhat.com>
---
src/qemu/qemu_snapshot.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
2:10 a.m.
New subject: [PATCH 02/17] qemuSnapshotRedefine: Fix use of snapshot definition after free
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(a)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
4:20 p.m.
New subject: [PATCH 02/17] qemuSnapshotRedefine: Fix use of snapshot definition after free
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(a)redhat.com>
---
src/qemu/qemu_snapshot.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
2:10 a.m.
New subject: [PATCH 03/17] virDomainMomentAssignDef: Simplify error handling
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(a)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
2:10 a.m.
New subject: [PATCH 04/17] virDomainSnapshotRedefineValidate: Fix validation of VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY flag
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(a)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
2:10 a.m.
New subject: [PATCH 05/17] virDomainSnapshotAlignDisks: Improve function comment
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(a)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
2:10 a.m.
New subject: [PATCH 06/17] virDomainSnapshotAlignDisks: Convert @default_snapshot to virDomainSnapshotLocation
Use the appropriate type for the variable and fix all callers.
Signed-off-by: Peter Krempa <pkrempa(a)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
2:10 a.m.
New subject: [PATCH 07/17] virDomainSnapshotAlignDisks: Move 'require_match' selection logic inside
'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(a)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
2:10 a.m.
New subject: [PATCH 08/17] virDomainSnapshotAlignDisks: Allow alternate domain definition when redefining
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(a)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
2:10 a.m.
New subject: [PATCH 09/17] virDomainSnapshotRedefineValidate: Unexport
The function isn't used outside of src/conf/snapshot_conf.c
Signed-off-by: Peter Krempa <pkrempa(a)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
12:49 a.m.
New subject: [PATCH 09/17] virDomainSnapshotRedefineValidate: Unexport
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(a)redhat.com>
---
src/conf/snapshot_conf.c | 2 +-
src/conf/snapshot_conf.h | 6 ------
2 files changed, 1 insertion(+), 7 deletions(-)
2:10 a.m.
New subject: [PATCH 10/17] virDomainSnapshotRedefinePrep: Use 'snapdef' for snapshot definition object
We use this variable name to distinguish it from the domain definition.
Signed-off-by: Peter Krempa <pkrempa(a)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
2:10 a.m.
New subject: [PATCH 11/17] virDomainSnapshotRedefineValidate: Don't modify the snapshot definition
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(a)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
2:10 a.m.
New subject: [PATCH 12/17] testDomainSnapshotCreateXML: Extract snapshot redefinition code
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(a)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
2:10 a.m.
New subject: [PATCH 13/17] qemuSnapshotCreate: Use 'snapdef' instead of 'def'
'def' is commonly used for domain definition.
Signed-off-by: Peter Krempa <pkrempa(a)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
2:10 a.m.
New subject: [PATCH 14/17] qemuSnapshotCreate: Standardize handling of the reference on @snapdef
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(a)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
2:10 a.m.
New subject: [PATCH 15/17] qemuDomainSnapshotLoad: Refactor handling of snapshot definition object
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(a)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
2:10 a.m.
New subject: [PATCH 16/17] virDomainSnapshotAssignDef: Clear second argument when it is consumed
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(a)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
2:10 a.m.
New subject: [PATCH 17/17] 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(a)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
12:59 a.m.
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(a)redhat.com>
Jano
1021
days inactive
1023
days old
21 comments
2 participants
participants (2)
-
Ján Tomko -
Peter Krempa