We will need to call this function from qemu_snapshot when introducing external snapshot revert support. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/snapshot_conf.c | 2 +- src/conf/snapshot_conf.h | 3 +++ src/libvirt_private.syms | 1 + 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 0acba95d7f..cc59bddbc8 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -74,7 +74,7 @@ VIR_ENUM_IMPL(virDomainSnapshotState, ); /* Snapshot Def functions */ -static void +void virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDef *disk) { VIR_FREE(disk->name); diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 96c77ef42b..ad49990a1e 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -59,6 +59,9 @@ struct _virDomainSnapshotDiskDef { virStorageSource *src; }; +void +virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDef *disk); + void virDomainSnapshotDiskDefFree(virDomainSnapshotDiskDef *disk); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index eed4c17d49..2b1d4e4512 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1037,6 +1037,7 @@ virDomainSnapshotDefFormat; virDomainSnapshotDefIsExternal; virDomainSnapshotDefNew; virDomainSnapshotDefParseString; +virDomainSnapshotDiskDefClear; virDomainSnapshotDiskDefFree; virDomainSnapshotDiskDefParseXML; virDomainSnapshotFormatConvertXMLFlags; -- 2.41.0

This new element will hold the new disk overlay created when reverting to non-leaf snapshot in order to remember the files libvirt created. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/schemas/domainsnapshot.rng | 7 +++++++ src/conf/snapshot_conf.c | 27 +++++++++++++++++++++++++++ src/conf/snapshot_conf.h | 5 +++++ 3 files changed, 39 insertions(+) diff --git a/src/conf/schemas/domainsnapshot.rng b/src/conf/schemas/domainsnapshot.rng index 45f01b96cd..2549c47b22 100644 --- a/src/conf/schemas/domainsnapshot.rng +++ b/src/conf/schemas/domainsnapshot.rng @@ -60,6 +60,13 @@ </zeroOrMore> </element> </optional> + <optional> + <element name="revertDisks"> + <zeroOrMore> + <ref name="disksnapshot"/> + </zeroOrMore> + </element> + </optional> <optional> <element name="active"> <choice> diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index ac5aba1753..f6725c0e7b 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -376,6 +376,22 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, return NULL; } + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { + g_autofree xmlNodePtr *revertDiskNodes = NULL; + + if ((n = virXPathNodeSet("./revertDisks/*", ctxt, &revertDiskNodes)) < 0) + return NULL; + if (n) + def->revertdisks = g_new0(virDomainSnapshotDiskDef, n); + def->nrevertdisks = n; + for (i = 0; i < def->nrevertdisks; i++) { + if (virDomainSnapshotDiskDefParseXML(revertDiskNodes[i], ctxt, + &def->revertdisks[i], + flags, xmlopt) < 0) + return NULL; + } + } + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL) { int active; @@ -834,6 +850,17 @@ virDomainSnapshotDefFormatInternal(virBuffer *buf, virBufferAddLit(buf, "</disks>\n"); } + if (def->nrevertdisks > 0) { + g_auto(virBuffer) childBuf = VIR_BUFFER_INIT_CHILD(buf); + + for (i = 0; i < def->nrevertdisks; i++) { + if (virDomainSnapshotDiskDefFormat(&childBuf, &def->revertdisks[i], xmlopt) < 0) + return -1; + } + + virXMLFormatElement(buf, "revertDisks", NULL, &childBuf); + } + if (def->parent.dom) { if (virDomainDefFormatInternal(def->parent.dom, xmlopt, buf, domainflags) < 0) diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index ad49990a1e..ab76af604a 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -80,6 +80,11 @@ struct _virDomainSnapshotDef { size_t ndisks; /* should not exceed dom->ndisks */ virDomainSnapshotDiskDef *disks; + /* When we revert to non-leaf snapshot we need to + * store the new overlay disks. */ + size_t nrevertdisks; + virDomainSnapshotDiskDef *revertdisks; + virObject *cookie; }; -- 2.41.0

When creating external snapshot this function is called only when the VM is not running so there is only one definition to care about. However, it will be used by external snapshot revert code for active and inactive definition and they may be different if a disk was (un)plugged only for the active or inactive definition. The current code would crash so use virDomainDiskByName() to get the correct disk from the domain definition based on the disk name and make sure it exists. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 72a0f71d4f..8e1eb21b5d 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -152,11 +152,14 @@ qemuSnapshotDomainDefUpdateDisk(virDomainDef *domdef, for (i = 0; i < snapdef->ndisks; i++) { g_autoptr(virStorageSource) newsrc = NULL; virDomainSnapshotDiskDef *snapdisk = &(snapdef->disks[i]); - virDomainDiskDef *defdisk = domdef->disks[i]; + virDomainDiskDef *defdisk = virDomainDiskByName(domdef, snapdisk->name, false); if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) continue; + if (!defdisk) + continue; + if (!(newsrc = virStorageSourceCopy(snapdisk->src, false))) return -1; -- 2.41.0

On Tue, Jun 27, 2023 at 17:07:11 +0200, Pavel Hrdina wrote: Please document the arguments of the new helper. Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Tue, Jun 27, 2023 at 17:07:12 +0200, Pavel Hrdina wrote: 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'. And drop this. and adjust this to pass the vm def explicitly. With adjustments: Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Tue, Jun 27, 2023 at 17:07:14 +0200, Pavel Hrdina wrote: So what is this actually doing? It looks like we are declaring that an async job is used here ... ... but it was never initiated as async. It is unclear if you are fixing a bug, or doing a change for consistency or what is happening here actually.

When reverting to external snapshot we need to create new overlay qcow2 files from the disk files the VM had when the snapshot was taken. There are some specifics and limitations when reverting to a snapshot: 1) When reverting to last snapshot we need to first create new overlay files before we can safely delete the old overlay files in case the creation fails so we have still recovery option when we error out. These new files will not have the suffix as when the snapshot was created as renaming the original files in order to use the same file names as when the snapshot was created would add unnecessary complexity to the code. 2) When reverting to any snapshot we will always create overlay files for every disk the VM had when the snapshot was done. Otherwise we would have to figure out if there is any other qcow2 image already using any of the VM disks as backing store and that itself might be extremely complex and in some cases impossible. 3) When reverting from any state the current overlay files will be always removed as that VM state is not meant to be saved. It's the same as with internal snapshots. If user want's to keep the current state before reverting they need to create a new snapshot. For now this will only work if the current snapshot is the last. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 232 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 228 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 1cb0ea55de..dbf2cdd5db 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -18,6 +18,8 @@ #include <config.h> +#include <fcntl.h> + #include "qemu_snapshot.h" #include "qemu_monitor.h" @@ -1975,6 +1977,183 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm, } +static int +qemuSnapshotRevertExternalPrepare(virDomainObj *vm, + virDomainSnapshotDef *tmpsnapdef, + virDomainMomentObj *snap, + virDomainDef *config, + virDomainDef *inactiveConfig, + int *memsnapFD, + char **memsnapPath) +{ + ssize_t i; + bool active = virDomainObjIsActive(vm); + virDomainDef *domdef = NULL; + virDomainSnapshotLocation location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); + + if (config) { + domdef = config; + } else { + domdef = inactiveConfig; + } + + /* We need this to generate creation timestamp that is used as default + * snapshot name. */ + if (virDomainMomentDefPostParse(&tmpsnapdef->parent) < 0) + return -1; + + if (virDomainSnapshotAlignDisks(tmpsnapdef, domdef, location, false, true) < 0) + return -1; + + for (i = 0; i < tmpsnapdef->ndisks; i++) { + virDomainSnapshotDiskDef *snapdisk = &tmpsnapdef->disks[i]; + virDomainDiskDef *domdisk = domdef->disks[i]; + + if (qemuSnapshotPrepareDiskExternal(domdisk, snapdisk, active, false) < 0) + return -1; + } + + if (memsnapFD && memsnapPath && snapdef->memorysnapshotfile) { + virQEMUDriver *driver = ((qemuDomainObjPrivate *) vm->privateData)->driver; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + + *memsnapPath = snapdef->memorysnapshotfile; + *memsnapFD = qemuDomainOpenFile(cfg, NULL, *memsnapPath, O_RDONLY, NULL); + } + + return 0; +} + + +static int +qemuSnapshotRevertExternalActive(virDomainObj *vm, + virDomainSnapshotDef *tmpsnapdef) +{ + ssize_t i; + g_autoptr(GHashTable) blockNamedNodeData = NULL; + g_autoptr(qemuSnapshotDiskContext) snapctxt = NULL; + + snapctxt = qemuSnapshotDiskContextNew(tmpsnapdef->ndisks, vm, VIR_ASYNC_JOB_SNAPSHOT); + + if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, VIR_ASYNC_JOB_SNAPSHOT))) + return -1; + + for (i = 0; i < tmpsnapdef->ndisks; i++) { + if (qemuSnapshotDiskPrepareOne(snapctxt, + vm->def->disks[i], + tmpsnapdef->disks + i, + blockNamedNodeData, + false, + true) < 0) + return -1; + } + + if (qemuSnapshotDiskCreate(snapctxt) < 0) + return -1; + + return 0; +} + + +static int +qemuSnapshotRevertExternalInactive(virDomainObj *vm, + virDomainSnapshotDef *tmpsnapdef, + virDomainDef *config, + virDomainDef *inactiveConfig) +{ + virDomainDef *domdef = NULL; + g_autoptr(virBitmap) created = NULL; + + created = virBitmapNew(tmpsnapdef->ndisks); + + if (config) { + domdef = config; + } else { + domdef = inactiveConfig; + } + + if (config) { + if (qemuSnapshotDomainDefUpdateDisk(config, tmpsnapdef, false) < 0) + return -1; + } + + if (qemuSnapshotDomainDefUpdateDisk(inactiveConfig, tmpsnapdef, false) < 0) + return -1; + + if (qemuSnapshotCreateQcow2Files(vm, domdef, tmpsnapdef, created, false) < 0) { + ssize_t bit = -1; + + while ((bit = virBitmapNextSetBit(created, bit)) >= 0) { + virDomainSnapshotDiskDef *snapdisk = &(tmpsnapdef->disks[bit]); + + if (virStorageSourceInit(snapdisk->src) < 0 || + virStorageSourceUnlink(snapdisk->src) < 0) { + VIR_WARN("Failed to remove snapshot image '%s'", + snapdisk->src->path); + } + } + + return -1; + } + + return 0; +} + + +static void +qemuSnapshotRevertExternalFinish(virDomainObj *vm, + virDomainSnapshotDef *tmpsnapdef, + virDomainMomentObj *snap) +{ + size_t i; + virDomainMomentObj *curSnap = virDomainSnapshotGetCurrent(vm->snapshots); + virDomainSnapshotDef *curdef = virDomainSnapshotObjGetDef(curSnap); + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); + + if (curdef->revertdisks) { + for (i = 0; i < curdef->nrevertdisks; i++) { + virDomainSnapshotDiskDef *snapdisk = &(curdef->revertdisks[i]); + + if (virStorageSourceInit(snapdisk->src) < 0 || + virStorageSourceUnlink(snapdisk->src) < 0) { + VIR_WARN("Failed to remove snapshot image '%s'", + snapdisk->src->path); + } + + virDomainSnapshotDiskDefClear(snapdisk); + } + + g_clear_pointer(&curdef->revertdisks, g_free); + curdef->nrevertdisks = 0; + } else { + for (i = 0; i < curdef->ndisks; i++) { + virDomainSnapshotDiskDef *snapdisk = &(curdef->disks[i]); + + if (virStorageSourceInit(snapdisk->src) < 0 || + virStorageSourceUnlink(snapdisk->src) < 0) { + VIR_WARN("Failed to remove snapshot image '%s'", + snapdisk->src->path); + } + } + } + + if (snap->nchildren != 0) { + snapdef->revertdisks = g_steal_pointer(&tmpsnapdef->disks); + snapdef->nrevertdisks = tmpsnapdef->ndisks; + tmpsnapdef->ndisks = 0; + } else { + for (i = 0; i < snapdef->ndisks; i++) { + virDomainSnapshotDiskDefClear(&snapdef->disks[i]); + } + g_free(snapdef->disks); + snapdef->disks = g_steal_pointer(&tmpsnapdef->disks); + snapdef->ndisks = tmpsnapdef->ndisks; + tmpsnapdef->ndisks = 0; + } +} + + static int qemuSnapshotRevertActive(virDomainObj *vm, virDomainSnapshotPtr snapshot, @@ -1989,10 +2168,14 @@ qemuSnapshotRevertActive(virDomainObj *vm, { virObjectEvent *event = NULL; virObjectEvent *event2 = NULL; + virDomainMomentObj *loadSnap = NULL; + VIR_AUTOCLOSE memsnapFD = -1; + char *memsnapPath = NULL; int detail; bool defined = false; qemuDomainSaveCookie *cookie = (qemuDomainSaveCookie *) snapdef->cookie; int rc; + g_autoptr(virDomainSnapshotDef) tmpsnapdef = NULL; start_flags |= VIR_QEMU_PROCESS_START_PAUSED; @@ -2010,6 +2193,19 @@ qemuSnapshotRevertActive(virDomainObj *vm, virObjectEventStateQueue(driver->domainEventState, event); } + if (virDomainSnapshotIsExternal(snap)) { + if (!(tmpsnapdef = virDomainSnapshotDefNew())) + return -1; + + if (qemuSnapshotRevertExternalPrepare(vm, tmpsnapdef, snap, + *config, *inactiveConfig, + &memsnapFD, &memsnapPath) < 0) { + return -1; + } + } else { + loadSnap = snap; + } + if (*inactiveConfig) { virDomainObjAssignDef(vm, inactiveConfig, false, NULL); defined = true; @@ -2026,7 +2222,8 @@ qemuSnapshotRevertActive(virDomainObj *vm, rc = qemuProcessStart(snapshot->domain->conn, driver, vm, cookie ? cookie->cpu : NULL, - VIR_ASYNC_JOB_SNAPSHOT, NULL, -1, NULL, snap, + VIR_ASYNC_JOB_SNAPSHOT, NULL, memsnapFD, + memsnapPath, loadSnap, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, start_flags); virDomainAuditStart(vm, "from-snapshot", rc >= 0); @@ -2038,6 +2235,14 @@ qemuSnapshotRevertActive(virDomainObj *vm, if (rc < 0) return -1; + + if (virDomainSnapshotIsExternal(snap)) { + if (qemuSnapshotRevertExternalActive(vm, tmpsnapdef) < 0) + return -1; + + qemuSnapshotRevertExternalFinish(vm, tmpsnapdef, snap); + } + /* Touch up domain state. */ if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) && (snapdef->state == VIR_DOMAIN_SNAPSHOT_PAUSED || @@ -2112,6 +2317,7 @@ qemuSnapshotRevertInactive(virDomainObj *vm, int detail; bool defined = false; int rc; + g_autoptr(virDomainSnapshotDef) tmpsnapdef = NULL; /* Transitions 1, 4, 7 */ /* Newer qemu -loadvm refuses to revert to the state of a snapshot @@ -2131,9 +2337,27 @@ qemuSnapshotRevertInactive(virDomainObj *vm, virObjectEventStateQueue(driver->domainEventState, event); } - if (qemuSnapshotInternalRevertInactive(driver, vm, snap) < 0) { - qemuDomainRemoveInactive(driver, vm, 0, false); - return -1; + if (virDomainSnapshotIsExternal(snap)) { + if (!(tmpsnapdef = virDomainSnapshotDefNew())) + return -1; + + if (qemuSnapshotRevertExternalPrepare(vm, tmpsnapdef, snap, + NULL, *inactiveConfig, + NULL, NULL) < 0) { + return -1; + } + + if (qemuSnapshotRevertExternalInactive(vm, tmpsnapdef, + NULL, *inactiveConfig) < 0) { + return -1; + } + + qemuSnapshotRevertExternalFinish(vm, tmpsnapdef, snap); + } else { + if (qemuSnapshotInternalRevertInactive(driver, vm, snap) < 0) { + qemuDomainRemoveInactive(driver, vm, 0, false); + return -1; + } } if (*inactiveConfig) { -- 2.41.0

The new name reflects that we prepare data for external snapshot deletion and the old name will be used later for different part of code. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index dbf2cdd5db..c5de24917b 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2570,9 +2570,9 @@ qemuSnapshotFindParentSnapForDisk(virDomainMomentObj *snap, static int -qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, - virDomainMomentObj *snap, - GSList **externalData) +qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, + virDomainMomentObj *snap, + GSList **externalData) { ssize_t i; virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); @@ -3429,7 +3429,7 @@ qemuSnapshotDelete(virDomainObj *vm, g_autoslist(qemuSnapshotDeleteExternalData) tmpData = NULL; /* this also serves as validation whether the snapshot can be deleted */ - if (qemuSnapshotDeleteExternalPrepare(vm, snap, &tmpData) < 0) + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, &tmpData) < 0) goto endjob; if (!virDomainObjIsActive(vm)) { @@ -3444,7 +3444,7 @@ qemuSnapshotDelete(virDomainObj *vm, /* Call the prepare again as some data require that the VM is * running to get everything we need. */ - if (qemuSnapshotDeleteExternalPrepare(vm, snap, &externalData) < 0) + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, &externalData) < 0) goto endjob; } else { qemuDomainJobPrivate *jobPriv = vm->job->privateData; -- 2.41.0

On Tue, Jun 27, 2023 at 17:07:18 +0200, Pavel Hrdina wrote: 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. With the extra loop removed: Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

When block commit is not needed we can just simply unlink the disk files. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 56 ++++++++++++++++++++++++++-------------- 1 file changed, 36 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index be94e97340..b08e06d312 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2510,6 +2510,7 @@ typedef struct _qemuSnapshotDeleteExternalData { virStorageSource *prevDiskSrc; /* source of disk for which @diskSrc is backing disk */ qemuBlockJobData *job; + bool merge; } qemuSnapshotDeleteExternalData; @@ -2618,6 +2619,7 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, data = g_new0(qemuSnapshotDeleteExternalData, 1); data->snapDisk = snapDisk; + data->merge = merge; data->parentDomDisk = virDomainDiskByTarget(snapdef->parent.dom, data->snapDisk->name); @@ -2628,7 +2630,7 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, return -1; } - if (merge) { + if (data->merge) { data->domDisk = qemuDomainDiskByName(vm->def, snapDisk->name); if (!data->domDisk) return -1; @@ -3053,31 +3055,42 @@ qemuSnapshotDiscardExternal(virDomainObj *vm, virTristateBool autofinalize = VIR_TRISTATE_BOOL_NO; unsigned int commitFlags = VIR_DOMAIN_BLOCK_COMMIT_DELETE; - if (data->domDisk->src == data->diskSrc) { - commitFlags |= VIR_DOMAIN_BLOCK_COMMIT_ACTIVE; - autofinalize = VIR_TRISTATE_BOOL_YES; + if (data->merge) { + if (data->domDisk->src == data->diskSrc) { + commitFlags |= VIR_DOMAIN_BLOCK_COMMIT_ACTIVE; + autofinalize = VIR_TRISTATE_BOOL_YES; + } + + if (qemuSnapshotSetInvalid(vm, data->parentSnap, data->snapDisk, true) < 0) + goto error; + + data->job = qemuBlockCommit(vm, + data->domDisk, + data->parentDiskSrc, + data->diskSrc, + data->prevDiskSrc, + 0, + VIR_ASYNC_JOB_SNAPSHOT, + autofinalize, + commitFlags); + + if (!data->job) + goto error; + } else { + if (virStorageSourceInit(data->parentDomDisk->src) < 0 || + virStorageSourceUnlink(data->parentDomDisk->src) < 0) { + VIR_WARN("Failed to remove snapshot image '%s'", + data->snapDisk->name); + } } - - if (qemuSnapshotSetInvalid(vm, data->parentSnap, data->snapDisk, true) < 0) - goto error; - - data->job = qemuBlockCommit(vm, - data->domDisk, - data->parentDiskSrc, - data->diskSrc, - data->prevDiskSrc, - 0, - VIR_ASYNC_JOB_SNAPSHOT, - autofinalize, - commitFlags); - - if (!data->job) - goto error; } for (cur = externalData; cur; cur = g_slist_next(cur)) { qemuSnapshotDeleteExternalData *data = cur->data; + if (!data->merge) + continue; + if (qemuSnapshotDeleteBlockJobRunning(vm, data->job) < 0) goto error; @@ -3092,6 +3105,9 @@ qemuSnapshotDiscardExternal(virDomainObj *vm, for (cur = externalData; cur; cur = g_slist_next(cur)) { qemuSnapshotDeleteExternalData *data = cur->data; + if (!data->merge) + continue; + if (data->job->state == QEMU_BLOCKJOB_STATE_READY) { if (qemuBlockPivot(vm, data->job, VIR_ASYNC_JOB_SNAPSHOT, NULL) < 0) goto error; -- 2.41.0

When deleting external snapshot and parent snapshot is the currently active snapshot as user reverted to it we need to properly update the parent snapshot metadata. After the delete is done the new overlay files will be the currently used files created when snapshot revert was done, replacing the original overlay files. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index b08e06d312..a206f015c4 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3168,6 +3168,41 @@ qemuSnapshotDiscardExternal(virDomainObj *vm, } +static int +qemuSnapshotDeleteUpdateParent(virDomainObj *vm, + virDomainMomentObj *parent) +{ + size_t i; + virQEMUDriver *driver = QEMU_DOMAIN_PRIVATE(vm)->driver; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + virDomainSnapshotDef *parentDef = virDomainSnapshotObjGetDef(parent); + + if (!parentDef) + return 0; + + if (!parentDef->revertdisks) + return 0; + + for (i = 0; i < parentDef->ndisks; i++) { + virDomainSnapshotDiskDefClear(&parentDef->disks[i]); + } + g_free(parentDef->disks); + + parentDef->disks = g_steal_pointer(&parentDef->revertdisks); + parentDef->ndisks = parentDef->nrevertdisks; + parentDef->nrevertdisks = 0; + + if (qemuDomainSnapshotWriteMetadata(vm, + parent, + driver->xmlopt, + cfg->snapshotDir) < 0) { + return -1; + } + + return 0; +} + + static int qemuSnapshotDiscardMetadata(virDomainObj *vm, virDomainMomentObj *snap, @@ -3207,6 +3242,11 @@ qemuSnapshotDiscardMetadata(virDomainObj *vm, virDomainMomentMoveChildren(snap, snap->parent); } + if (update_parent && snap->parent) { + if (qemuSnapshotDeleteUpdateParent(vm, snap->parent) < 0) + ret = -1; + } + snapFile = g_strdup_printf("%s/%s/%s.xml", cfg->snapshotDir, vm->def->name, snap->def->name); -- 2.41.0

On Tue, Jun 27, 2023 at 17:07:22 +0200, Pavel Hrdina wrote: At least in one instance the structure's 'name' and 'src' fields are filled with allocated pointers, so this looks like it will leak memory. I also didn't find code which frees the 'reverdisks' field when the snapshot definition is being freed.

With introduction of external snapshot revert we will have to update backing store of qcow images not actively used be QEMU manually. The need for this patch comes from the fact that we stop and start QEMU process therefore after revert not all existing snapshots will be known to that QEMU process due to reverting to non-leaf snapshot or having multiple branches. We need to loop over all existing snapshots and check all disks to see if they happen to have the image we are deleting as backing store and update them to point to the new image except for images currently used by the running QEMU process doing the merge operation. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 95 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 2950ad7d77..337c83f151 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2530,6 +2530,8 @@ typedef struct _qemuSnapshotDeleteExternalData { virStorageSource *parentDiskSrc; /* backing disk source of the @diskSrc */ virStorageSource *prevDiskSrc; /* source of disk for which @diskSrc is backing disk */ + GSList *disksWithBacking; /* list of storage source for which the + deleted storage source is backing store */ qemuBlockJobData *job; bool merge; } qemuSnapshotDeleteExternalData; @@ -2542,6 +2544,7 @@ qemuSnapshotDeleteExternalDataFree(qemuSnapshotDeleteExternalData *data) return; virObjectUnref(data->job); + g_slist_free(data->disksWithBacking); g_free(data); } @@ -2591,6 +2594,60 @@ qemuSnapshotFindParentSnapForDisk(virDomainMomentObj *snap, } +struct _qemuSnapshotDisksWithBackingStoreData { + virDomainMomentObj *current; + virStorageSource *diskSrc; + GSList **disksWithBacking; +}; + + +static int +qemuSnapshotDiskHasBackingDisk(void *payload, + const char *name G_GNUC_UNUSED, + void *opaque) +{ + virDomainMomentObj *snap = payload; + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); + struct _qemuSnapshotDisksWithBackingStoreData *data = opaque; + ssize_t i; + + /* skip snapshots that are within the active snapshot tree as it will be handled + * by qemu */ + if (virDomainMomentIsAncestor(data->current, snap) || data->current == snap) + return 0; + + for (i = 0; i < snapdef->parent.dom->ndisks; i++) { + virDomainDiskDef *disk = snapdef->parent.dom->disks[i]; + + if (!virStorageSourceIsLocalStorage(disk->src)) + continue; + + if (!disk->src->backingStore) + ignore_value(virStorageSourceGetMetadata(disk->src, -1, -1, 1, false)); + + if (virStorageSourceIsSameLocation(disk->src->backingStore, data->diskSrc)) + *data->disksWithBacking = g_slist_prepend(*data->disksWithBacking, disk->src); + } + + return 0; +} + + +static void +qemuSnapshotGetDisksWithBackingStore(virDomainObj *vm, + virDomainMomentObj *snap, + qemuSnapshotDeleteExternalData *data) +{ + struct _qemuSnapshotDisksWithBackingStoreData iterData; + + iterData.current = virDomainSnapshotGetCurrent(vm->snapshots); + iterData.diskSrc = data->diskSrc; + iterData.disksWithBacking = &data->disksWithBacking; + + virDomainMomentForEachDescendant(snap, qemuSnapshotDiskHasBackingDisk, &iterData); +} + + /** * qemuSnapshotDeleteExternalPrepareData: * @vm: domain object @@ -2682,6 +2739,8 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, _("snapshot VM disk source and parent disk source are not the same")); return -1; } + + qemuSnapshotGetDisksWithBackingStore(vm, snap, data); } data->parentSnap = qemuSnapshotFindParentSnapForDisk(snap, data->snapDisk); @@ -3063,6 +3122,40 @@ qemuSnapshotSetInvalid(virDomainObj *vm, } +static void +qemuSnapshotUpdateBackingStore(virDomainObj *vm, + qemuSnapshotDeleteExternalData *data) +{ + GSList *cur = NULL; + const char *qemuImgPath; + virQEMUDriver *driver = QEMU_DOMAIN_PRIVATE(vm)->driver; + + if (!(qemuImgPath = qemuFindQemuImgBinary(driver))) + return; + + for (cur = data->disksWithBacking; cur; cur = g_slist_next(cur)) { + virStorageSource *diskSrc = cur->data; + g_autoptr(virCommand) cmd = NULL; + + /* creates cmd line args: qemu-img create -f qcow2 -o */ + if (!(cmd = virCommandNewArgList(qemuImgPath, + "rebase", + "-u", + "-F", + virStorageFileFormatTypeToString(data->parentDiskSrc->format), + "-f", + virStorageFileFormatTypeToString(diskSrc->format), + "-b", + data->parentDiskSrc->path, + diskSrc->path, + NULL))) + continue; + + ignore_value(virCommandRun(cmd, NULL)); + } +} + + static int qemuSnapshotDiscardExternal(virDomainObj *vm, virDomainMomentObj *snap, @@ -3149,6 +3242,8 @@ qemuSnapshotDiscardExternal(virDomainObj *vm, qemuBlockJobSyncEnd(vm, data->job, VIR_ASYNC_JOB_SNAPSHOT); + qemuSnapshotUpdateBackingStore(vm, data); + if (qemuSnapshotSetInvalid(vm, data->parentSnap, data->snapDisk, false) < 0) goto error; } -- 2.41.0

There will be more external snapshot checks introduced by following patch so group them together. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 337c83f151..9246c02f12 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3635,18 +3635,18 @@ qemuSnapshotDeleteValidate(virDomainObj *vm, } } - if (virDomainSnapshotIsExternal(snap) && - qemuDomainHasBlockjob(vm, false)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("cannot delete external snapshots when there is another active block job")); - return -1; - } + if (virDomainSnapshotIsExternal(snap)) { + if (qemuDomainHasBlockjob(vm, false)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("cannot delete external snapshots when there is another active block job")); + return -1; + } - if (virDomainSnapshotIsExternal(snap) && - (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("deletion of external disk snapshots with children not supported")); - return -1; + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("deletion of external disk snapshots with children not supported")); + return -1; + } } return 0; -- 2.41.0