[libvirt PATCH v2 09/24] qemu_snapshot: allow using alternate domain definition when creating qcow2 files

[Peter Krempa]

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 at 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 at redhat.com>