[libvirt PATCH 0/3] external snapshot deletion fixes
Pavel Hrdina (3): qemu_snapshot: remove memory snapshot when deleting external snapshot qemu_snapshot: refactor qemuSnapshotDeleteExternalPrepare NEWS: document external memory snapshot bug fixes NEWS.rst | 7 +++++++ src/qemu/qemu_snapshot.c | 42 ++++++++++++++++++++++++++-------------- 2 files changed, 34 insertions(+), 15 deletions(-) -- 2.39.1
When deleting external snapshot we should remove the memory snapshot file as well. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index b8416808b3..5cdcbc6290 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2684,9 +2684,11 @@ qemuSnapshotSetInvalid(virDomainObj *vm, static int qemuSnapshotDiscardExternal(virDomainObj *vm, + virDomainMomentObj *snap, GSList *externalData) { GSList *cur = NULL; + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); for (cur = externalData; cur; cur = g_slist_next(cur)) { qemuSnapshotDeleteExternalData *data = cur->data; @@ -2756,6 +2758,14 @@ qemuSnapshotDiscardExternal(virDomainObj *vm, goto error; } + if (snapdef->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL && + snapdef->memorysnapshotfile) { + if (unlink(snapdef->memorysnapshotfile) < 0) { + VIR_WARN("failed to remove memory snapshot '%s'", + snapdef->memorysnapshotfile); + } + } + return 0; error: @@ -2886,7 +2896,7 @@ qemuSnapshotDiscardImpl(virQEMUDriver *driver, } if (virDomainSnapshotIsExternal(snap)) { - if (qemuSnapshotDiscardExternal(vm, externalData) < 0) + if (qemuSnapshotDiscardExternal(vm, snap, externalData) < 0) return -1; } else { if (qemuDomainSnapshotForEachQcow2(driver, def, snap, "-d", true) < 0) @@ -2894,7 +2904,7 @@ qemuSnapshotDiscardImpl(virQEMUDriver *driver, } } else { if (virDomainSnapshotIsExternal(snap)) { - if (qemuSnapshotDiscardExternal(vm, externalData) < 0) + if (qemuSnapshotDiscardExternal(vm, snap, externalData) < 0) return -1; } else { /* Similarly as internal snapshot creation we would use a regular job -- 2.39.1
On Tue, Feb 21, 2023 at 17:22:59 +0100, Pavel Hrdina wrote:
When deleting external snapshot we should remove the memory snapshot file as well.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
When user creates external snapshot with making only memory snapshot without any disks deleting that snapshot failed without reporting any meaningful error. The issue is that the qemuSnapshotDeleteExternalPrepare function returns NULL because the returned list is empty. This will not change so to make it clear if the function fails or not return int instead and have another parameter where we can pass the list. With the fixed memory snapshot deletion it will now correctly delete memory only snapshot as well. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2170826 Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 5cdcbc6290..cfa531edef 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2301,9 +2301,10 @@ qemuSnapshotFindParentSnapForDisk(virDomainMomentObj *snap, } -static GSList* +static int qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, - virDomainMomentObj *snap) + virDomainMomentObj *snap, + GSList **externalData) { ssize_t i; virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); @@ -2320,7 +2321,7 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, virReportError(VIR_ERR_OPERATION_INVALID, _("snapshot disk '%s' was target of not completed snapshot delete"), snapDisk->name); - return NULL; + return -1; } data = g_new0(qemuSnapshotDeleteExternalData, 1); @@ -2328,18 +2329,18 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, data->domDisk = qemuDomainDiskByName(vm->def, snapDisk->name); if (!data->domDisk) - return NULL; + return -1; data->diskSrc = virStorageSourceChainLookupBySource(data->domDisk->src, data->snapDisk->src, &data->prevDiskSrc); if (!data->diskSrc) - return NULL; + return -1; if (!virStorageSourceIsSameLocation(data->diskSrc, data->snapDisk->src)) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("VM disk source and snapshot disk source are not the same")); - return NULL; + return -1; } data->parentDomDisk = virDomainDiskByTarget(snapdef->parent.dom, @@ -2348,7 +2349,7 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, virReportError(VIR_ERR_OPERATION_FAILED, _("failed to find disk '%s' in snapshot VM XML"), snapDisk->name); - return NULL; + return -1; } if (virDomainObjIsActive(vm)) { @@ -2356,13 +2357,13 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, if (!virStorageSourceIsBacking(data->parentDiskSrc)) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("failed to find parent disk source in backing chain")); - return NULL; + return -1; } if (!virStorageSourceIsSameLocation(data->parentDiskSrc, data->parentDomDisk->src)) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("snapshot VM disk source and parent disk source are not the same")); - return NULL; + return -1; } } @@ -2371,15 +2372,16 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, if (data->parentSnap && !virDomainSnapshotIsExternal(data->parentSnap)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("deleting external snapshot that has internal snapshot as parent not supported")); - return NULL; + return -1; } ret = g_slist_prepend(ret, g_steal_pointer(&data)); } ret = g_slist_reverse(ret); + *externalData = g_steal_pointer(&ret); - return g_steal_pointer(&ret); + return 0; } @@ -3159,7 +3161,7 @@ qemuSnapshotDelete(virDomainObj *vm, g_autoslist(qemuSnapshotDeleteExternalData) tmpData = NULL; /* this also serves as validation whether the snapshot can be deleted */ - if (!(tmpData = qemuSnapshotDeleteExternalPrepare(vm, snap))) + if (qemuSnapshotDeleteExternalPrepare(vm, snap, &tmpData) < 0) goto endjob; if (!virDomainObjIsActive(vm)) { @@ -3174,7 +3176,7 @@ qemuSnapshotDelete(virDomainObj *vm, /* Call the prepare again as some data require that the VM is * running to get everything we need. */ - if (!(externalData = qemuSnapshotDeleteExternalPrepare(vm, snap))) + if (qemuSnapshotDeleteExternalPrepare(vm, snap, &externalData) < 0) goto endjob; } else { qemuDomainJobPrivate *jobPriv = vm->job->privateData; -- 2.39.1
On Tue, Feb 21, 2023 at 17:23:00 +0100, Pavel Hrdina wrote:
When user creates external snapshot with making only memory snapshot without any disks deleting that snapshot failed without reporting any meaningful error.
The issue is that the qemuSnapshotDeleteExternalPrepare function returns NULL because the returned list is empty. This will not change so to make it clear if the function fails or not return int instead and have another parameter where we can pass the list.
With the fixed memory snapshot deletion it will now correctly delete memory only snapshot as well.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2170826
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- NEWS.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 4625a838f8..682b8f9eb2 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -52,6 +52,13 @@ v9.1.0 (unreleased) watchdog that did not fire before will now fire once used. To switch to the previous behavior the watchdog action must be set to ``none``. + * QEMU: fix deleting memory snapshot when deleting external snapshots + + When external snapshot deletion was introduced it did not remove memory + snapshot when it existed. In addition when external memory only snapshot + was created libvirt failed without producing any error. Now both issues + are fixed and memory snapshot file is properly deleted. + v9.0.0 (2023-01-16) =================== -- 2.39.1
On Tue, Feb 21, 2023 at 17:23:01 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- NEWS.rst | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/NEWS.rst b/NEWS.rst index 4625a838f8..682b8f9eb2 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -52,6 +52,13 @@ v9.1.0 (unreleased) watchdog that did not fire before will now fire once used. To switch to the previous behavior the watchdog action must be set to ``none``.
+ * QEMU: fix deleting memory snapshot when deleting external snapshots + + When external snapshot deletion was introduced it did not remove memory + snapshot when it existed. In addition when external memory only snapshot + was created libvirt failed without producing any error. Now both issues + are fixed and memory snapshot file is properly deleted.
You can drop the last sentence. The title implies that you've fixed it. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Pavel Hrdina -
Peter Krempa