Re: [libvirt PATCH v2 09/24] qemu_snapshot: allow using alternate domain definition when creating qcow2 files
On Tue, Jun 27, 2023 at 17:07:12 +0200, Pavel Hrdina wrote:
To create new overlay files when external snapshot revert support is
introduced we will be using different domain definition than what is
currently used by the domain.
Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
---
src/qemu/qemu_snapshot.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
index 227c201195..069c4c8ba7 100644
--- a/src/qemu/qemu_snapshot.c
+++ b/src/qemu/qemu_snapshot.c
@@ -183,6 +183,7 @@ qemuSnapshotDomainDefUpdateDisk(virDomainDef *domdef,
static int
qemuSnapshotCreateQcow2Files(virDomainObj *vm,
+ virDomainDef *def,
At this point I'd strongly suggest removing 'vm' argument which is at
this point used only to get the virQEMUDriver pointer from the private
data.
That way you will not have to explain in the comment I've requested in
the previous review why both 'vm' and 'def' are needed and why
'def'
isn't picked from 'vm'.
virDomainSnapshotDef *snapdef,
virBitmap *created,
bool reuse)
@@ -194,6 +195,9 @@ qemuSnapshotCreateQcow2Files(virDomainObj *vm,
virDomainSnapshotDiskDef *snapdisk = NULL;
virDomainDiskDef *defdisk = NULL;
+ if (!def)
+ def = vm->def;
And drop this.
+
if (!(qemuImgPath = qemuFindQemuImgBinary(driver)))
return -1;
@@ -203,7 +207,7 @@ qemuSnapshotCreateQcow2Files(virDomainObj *vm,
for (i = 0; i < snapdef->ndisks && !reuse; i++) {
g_autoptr(virCommand) cmd = NULL;
snapdisk = &(snapdef->disks[i]);
- defdisk = vm->def->disks[i];
+ defdisk = def->disks[i];
if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
continue;
@@ -268,7 +272,7 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver,
virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap);
g_autoptr(virBitmap) created = virBitmapNew(snapdef->ndisks);
- if (qemuSnapshotCreateQcow2Files(vm, snapdef, created, reuse) < 0)
+ if (qemuSnapshotCreateQcow2Files(vm, NULL, snapdef, created, reuse) < 0)a
and adjust this to pass the vm def explicitly.
With adjustments:
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>