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