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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
