[libvirt PATCH v2 15/24] qemu_snapshot: add merge to external snapshot delete prepare data

[Peter Krempa]

On Tue, Jun 27, 2023 at 17:07:18 +0200, Pavel Hrdina wrote:
> Before external snapshot revert every delete operation did block commit
> in order to delete a snapshot. But now when user reverts to non-leaf
> snapshot deleting leaf snapshot will not have any overlay files so we
> can just simply delete the snapshot images.
> 
> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> ---
>  src/qemu/qemu_snapshot.c | 93 ++++++++++++++++++++++++++--------------
>  1 file changed, 60 insertions(+), 33 deletions(-)
> 
> diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c
> index 08cff2a9a2..9c4d26bad5 100644
> --- a/src/qemu/qemu_snapshot.c
> +++ b/src/qemu/qemu_snapshot.c
> @@ -2569,9 +2569,26 @@ qemuSnapshotFindParentSnapForDisk(virDomainMomentObj *snap,
>  }
>  
>  
> +/**
> + * qemuSnapshotDeleteExternalPrepareData:
> + * @vm: domain object
> + * @snap: snapshot object
> + * @merge: whether we are just deleting image or not
> + * @externalData: prepared data to delete external snapshot
> + *
> + * Validate if we can delete selected snapshot @snap and prepare all necessary
> + * data that will be used when deleting snapshot as @externalData.
> + *
> + * If @merge is set to true we will merge the deleted snapshot into parent one
> + * instead of just deleting it. This is necessary when operating on snapshot
> + * that has existing overlay files.
> + *
> + * Returns -1 on error, 0 on success.
> + */
>  static int
>  qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm,
>                                        virDomainMomentObj *snap,
> +                                      bool merge,
>                                        GSList **externalData)
>  {
>      ssize_t i;
> @@ -2579,7 +2596,6 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm,
>      g_autoslist(qemuSnapshotDeleteExternalData) ret = NULL;
>  
>      for (i = 0; i < snapdef->ndisks; i++) {
> -        g_autofree qemuSnapshotDeleteExternalData *data = NULL;
>          virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]);
>  
>          if (snapDisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
> @@ -2591,14 +2607,18 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm,
>                             snapDisk->name);
>              return -1;
>          }
> +    }
> +
> +    for (i = 0; i < snapdef->ndisks; i++) {

I didn't really find a reason why you've added another loop here. The
only thing the function does is to prepare data, so I don't think there
is anything that'd require another pass through the disks.

> +        virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]);
> +        g_autofree qemuSnapshotDeleteExternalData *data = NULL;
> +
> +        if (snapDisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
> +            continue;

With the extra loop removed:

Reviewed-by: Peter Krempa <pkrempa at redhat.com>