On Mon, Mar 13, 2023 at 16:42:05 +0100, Pavel Hrdina wrote: [...] [...] Always use explicit comparison against 0 for non-pointer, non-booleans. Also preferrably use virXMLFormatElement. Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

Extract the code that updates disks in domain definition while creating external snapshots. We will use it later in the external snapshot revert code. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 63 ++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 7aa4195f04..924731fdf1 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -142,6 +142,42 @@ qemuSnapshotFSThaw(virDomainObj *vm, } +static int +qemuSnapshotDomainDefUpdateDisk(virDomainDef *domdef, + virDomainSnapshotDef *snapdef, + bool reuse) +{ + size_t i; + + for (i = 0; i < snapdef->ndisks; i++) { + g_autoptr(virStorageSource) newsrc = NULL; + virDomainSnapshotDiskDef *snapdisk = &(snapdef->disks[i]); + virDomainDiskDef *defdisk = domdef->disks[i]; + + if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + continue; + + if (!(newsrc = virStorageSourceCopy(snapdisk->src, false))) + return -1; + + if (virStorageSourceInitChainElement(newsrc, defdisk->src, false) < 0) + return -1; + + if (!reuse && + virStorageSourceHasBacking(defdisk->src)) { + defdisk->src->readonly = true; + newsrc->backingStore = g_steal_pointer(&defdisk->src); + } else { + virObjectUnref(defdisk->src); + } + + defdisk->src = g_steal_pointer(&newsrc); + } + + return 0; +} + + /* The domain is expected to be locked and inactive. */ static int qemuSnapshotCreateInactiveInternal(virQEMUDriver *driver, @@ -216,31 +252,8 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, } /* update disk definitions */ - for (i = 0; i < snapdef->ndisks; i++) { - g_autoptr(virStorageSource) newsrc = NULL; - - snapdisk = &(snapdef->disks[i]); - defdisk = vm->def->disks[i]; - - if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) - continue; - - if (!(newsrc = virStorageSourceCopy(snapdisk->src, false))) - goto cleanup; - - if (virStorageSourceInitChainElement(newsrc, defdisk->src, false) < 0) - goto cleanup; - - if (!reuse && - virStorageSourceHasBacking(defdisk->src)) { - defdisk->src->readonly = true; - newsrc->backingStore = g_steal_pointer(&defdisk->src); - } else { - virObjectUnref(defdisk->src); - } - - defdisk->src = g_steal_pointer(&newsrc); - } + if (qemuSnapshotDomainDefUpdateDisk(vm->def, snapdef, reuse) < 0) + goto cleanup; if (virDomainDefSave(vm->def, driver->xmlopt, cfg->configDir) < 0) goto cleanup; -- 2.39.2

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; --- 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 924731fdf1..bbef753db6 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.39.2

Extract creation of qcow2 files for external snapshots to separate function as we will need it for external snapshot revert code. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 67 +++++++++++++++++++++++++--------------- 1 file changed, 42 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index bbef753db6..06ed20c815 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -181,35 +181,21 @@ qemuSnapshotDomainDefUpdateDisk(virDomainDef *domdef, } -/* The domain is expected to be locked and inactive. */ static int -qemuSnapshotCreateInactiveInternal(virQEMUDriver *driver, - virDomainObj *vm, - virDomainMomentObj *snap) -{ - return qemuDomainSnapshotForEachQcow2(driver, vm->def, snap, "-c", false); -} - - -/* The domain is expected to be locked and inactive. */ -static int -qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, - virDomainObj *vm, - virDomainMomentObj *snap, - bool reuse) +qemuSnapshotCreateQcow2Files(virDomainObj *vm, + virDomainSnapshotDef *snapdef, + virBitmap *created, + bool reuse) { size_t i; - virDomainSnapshotDiskDef *snapdisk; - virDomainDiskDef *defdisk; const char *qemuImgPath; - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - int ret = -1; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; - virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); - g_autoptr(virBitmap) created = virBitmapNew(snapdef->ndisks); + virQEMUDriver *driver = ((qemuDomainObjPrivate *) vm->privateData)->driver; + virDomainSnapshotDiskDef *snapdisk = NULL; + virDomainDiskDef *defdisk = NULL; if (!(qemuImgPath = qemuFindQemuImgBinary(driver))) - goto cleanup; + return -1; /* If reuse is true, then qemuSnapshotPrepare already * ensured that the new files exist, and it was up to the user to @@ -218,6 +204,7 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, g_autoptr(virCommand) cmd = NULL; snapdisk = &(snapdef->disks[i]); defdisk = vm->def->disks[i]; + if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) continue; @@ -225,7 +212,7 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, snapdisk->src->format = VIR_STORAGE_FILE_QCOW2; if (qemuDomainStorageSourceValidateDepth(defdisk->src, 1, defdisk->dst) < 0) - goto cleanup; + return -1; /* creates cmd line args: qemu-img create -f qcow2 -o */ if (!(cmd = virCommandNewArgList(qemuImgPath, @@ -234,7 +221,7 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, virStorageFileFormatTypeToString(snapdisk->src->format), "-o", NULL))) - goto cleanup; + return -1; /* adds cmd line arg: backing_fmt=format,backing_file=/path/to/backing/file */ virBufferAsprintf(&buf, "backing_fmt=%s,backing_file=", @@ -251,9 +238,39 @@ qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, ignore_value(virBitmapSetBit(created, i)); if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + return -1; } + return 0; +} + + +/* The domain is expected to be locked and inactive. */ +static int +qemuSnapshotCreateInactiveInternal(virQEMUDriver *driver, + virDomainObj *vm, + virDomainMomentObj *snap) +{ + return qemuDomainSnapshotForEachQcow2(driver, vm->def, snap, "-c", false); +} + + +/* The domain is expected to be locked and inactive. */ +static int +qemuSnapshotCreateInactiveExternal(virQEMUDriver *driver, + virDomainObj *vm, + virDomainMomentObj *snap, + bool reuse) +{ + virDomainSnapshotDiskDef *snapdisk; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + int ret = -1; + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); + g_autoptr(virBitmap) created = virBitmapNew(snapdef->ndisks); + + if (qemuSnapshotCreateQcow2Files(vm, snapdef, created, reuse) < 0) + goto cleanup; + /* update disk definitions */ if (qemuSnapshotDomainDefUpdateDisk(vm->def, snapdef, reuse) < 0) goto cleanup; -- 2.39.2

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 | 143 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 139 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 9e4b978b1b..af4e2ea6aa 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" @@ -1968,6 +1970,119 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm, } +static int +qemuSnapshotRevertExternal(virDomainObj *vm, + virDomainMomentObj *snap, + virDomainDef *config, + virDomainDef *inactiveConfig, + int *memsnapFD, + char **memsnapPath) +{ + size_t i; + virDomainDef *domdef = NULL; + virDomainSnapshotLocation location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + virDomainMomentObj *curSnap = virDomainSnapshotGetCurrent(vm->snapshots); + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); + virDomainSnapshotDef *curdef = virDomainSnapshotObjGetDef(curSnap); + g_autoptr(virDomainSnapshotDef) tmpsnapdef = NULL; + g_autoptr(virBitmap) created = NULL; + int ret = -1; + + if (config) { + domdef = config; + } else { + domdef = inactiveConfig; + } + + if (!(tmpsnapdef = virDomainSnapshotDefNew())) + return -1; + + if (virDomainMomentDefPostParse(&tmpsnapdef->parent) < 0) + return -1; + + if (virDomainSnapshotAlignDisks(tmpsnapdef, domdef, location, false) < 0) + return -1; + + created = virBitmapNew(tmpsnapdef->ndisks); + + if (qemuSnapshotCreateQcow2Files(vm, domdef, tmpsnapdef, created, 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); + } + + if (config) { + if (qemuSnapshotDomainDefUpdateDisk(config, tmpsnapdef, false) < 0) + goto cleanup; + } + + if (qemuSnapshotDomainDefUpdateDisk(inactiveConfig, tmpsnapdef, false) < 0) + goto cleanup; + + if (curdef->revertdisks) { + for (i = 0; i < curdef->nrevertdisks; i++) { + virDomainSnapshotDiskDef *snapdisk = &(curdef->revertdisks[i]); + + if (unlink(snapdisk->src->path) < 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 (unlink(snapdisk->src->path) < 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; + } + + ret = 0; + + cleanup: + if (ret != 0 && created) { + ssize_t bit = -1; + + while ((bit = virBitmapNextSetBit(created, bit)) >= 0) { + virDomainSnapshotDiskDef *snapdisk = &(tmpsnapdef->disks[bit]); + + if (unlink(snapdisk->src->path) < 0) { + VIR_WARN("Failed to remove snapshot image '%s'", + snapdisk->src->path); + } + } + } + + return ret; +} + + static int qemuSnapshotRevertActive(virDomainObj *vm, virDomainSnapshotPtr snapshot, @@ -1982,6 +2097,9 @@ 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; @@ -2003,6 +2121,15 @@ qemuSnapshotRevertActive(virDomainObj *vm, virObjectEventStateQueue(driver->domainEventState, event); } + if (virDomainSnapshotIsExternal(snap)) { + if (qemuSnapshotRevertExternal(vm, snap, *config, *inactiveConfig, + &memsnapFD, &memsnapPath) < 0) { + return -1; + } + } else { + loadSnap = snap; + } + if (*inactiveConfig) { virDomainObjAssignDef(vm, inactiveConfig, false, NULL); defined = true; @@ -2019,7 +2146,8 @@ qemuSnapshotRevertActive(virDomainObj *vm, rc = qemuProcessStart(snapshot->domain->conn, driver, vm, cookie ? cookie->cpu : NULL, - VIR_ASYNC_JOB_START, NULL, -1, NULL, snap, + VIR_ASYNC_JOB_START, NULL, memsnapFD, + memsnapPath, loadSnap, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, start_flags); virDomainAuditStart(vm, "from-snapshot", rc >= 0); @@ -2124,9 +2252,16 @@ qemuSnapshotRevertInactive(virDomainObj *vm, virObjectEventStateQueue(driver->domainEventState, event); } - if (qemuSnapshotInternalRevertInactive(driver, vm, snap) < 0) { - qemuDomainRemoveInactive(driver, vm, 0, false); - return -1; + if (virDomainSnapshotIsExternal(snap)) { + if (qemuSnapshotRevertExternal(vm, snap, NULL, *inactiveConfig, + NULL, NULL) < 0) { + return -1; + } + } else { + if (qemuSnapshotInternalRevertInactive(driver, vm, snap) < 0) { + qemuDomainRemoveInactive(driver, vm, 0, false); + return -1; + } } if (*inactiveConfig) { -- 2.39.2

On Tue, Apr 04, 2023 at 03:03:56PM +0200, Peter Krempa wrote: I can probably just rename the function to reflect that it just generates creation time and uses it as default name so in the future if we need to add more post parse functionality we would have to create true post parse function calling this renamed helper. This should not be an issue as the tmpsnapdef is a new empty snapshot definition so no disk should have VIR_DOMAIN_SNAPSHOT_LOCATION_NO. This code should just take existing domain definition and fill in the new empty snapshot definition with all disks the domain is currently using and the domain definition is from the snapshot metadata that we are reverting to. So this part should work correctly. This should not be an issue and the fact we call virDomainSnapshotDefAssignExternalNames actually helps us to not support reverting to non-local disks as with the existing API it is not possible. This function is also called unconditionally when creating external snapshots so without providing correct XML it will also fail. I need to definitely remove all code that directly uses disk->src->path but we should be able to keep virDomainSnapshotAlignDisks as the same is used when creating snapshot.

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; --- 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 af4e2ea6aa..64730dcb0b 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2474,9 +2474,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); @@ -3333,7 +3333,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)) { @@ -3348,7 +3348,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.39.2

This part of code is about to grow to make deletion work when use reverts to non-leaf snapshot. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 89 +++++++++++++++++++++++++--------------- 1 file changed, 55 insertions(+), 34 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 64730dcb0b..d545c984ce 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2557,6 +2557,58 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, } +static int +qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, + virDomainMomentObj *snap, + unsigned int flags, + GSList **externalData, + bool *stop_qemu) +{ + virQEMUDriver *driver = ((qemuDomainObjPrivate *) vm->privateData)->driver; + g_autoslist(qemuSnapshotDeleteExternalData) tmpData = NULL; + + if (!virDomainSnapshotIsExternal(snap)) + return 0; + + if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { + return 0; + } + + /* this also serves as validation whether the snapshot can be deleted */ + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, &tmpData) < 0) + return -1; + + if (!virDomainObjIsActive(vm)) { + if (qemuProcessStart(NULL, driver, vm, NULL, VIR_ASYNC_JOB_SNAPSHOT, + NULL, -1, NULL, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_CREATE, + VIR_QEMU_PROCESS_START_PAUSED) < 0) { + return -1; + } + + *stop_qemu = true; + + /* Call the prepare again as some data require that the VM is + * running to get everything we need. */ + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, externalData) < 0) + return -1; + } else { + qemuDomainJobPrivate *jobPriv = vm->job->privateData; + + *externalData = g_steal_pointer(&tmpData); + + /* If the VM is running we need to indicate that the async snapshot + * job is snapshot delete job. */ + jobPriv->snapshotDelete = true; + + qemuDomainSaveStatus(vm); + } + + return 0; +} + + typedef struct _virQEMUMomentReparent virQEMUMomentReparent; struct _virQEMUMomentReparent { const char *dir; @@ -3327,40 +3379,9 @@ qemuSnapshotDelete(virDomainObj *vm, if (qemuSnapshotDeleteValidate(vm, snap, flags) < 0) goto endjob; - if (virDomainSnapshotIsExternal(snap) && - !(flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | - VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY))) { - g_autoslist(qemuSnapshotDeleteExternalData) tmpData = NULL; - - /* this also serves as validation whether the snapshot can be deleted */ - if (qemuSnapshotDeleteExternalPrepareData(vm, snap, &tmpData) < 0) - goto endjob; - - if (!virDomainObjIsActive(vm)) { - if (qemuProcessStart(NULL, driver, vm, NULL, VIR_ASYNC_JOB_SNAPSHOT, - NULL, -1, NULL, NULL, - VIR_NETDEV_VPORT_PROFILE_OP_CREATE, - VIR_QEMU_PROCESS_START_PAUSED) < 0) { - goto endjob; - } - - stop_qemu = true; - - /* Call the prepare again as some data require that the VM is - * running to get everything we need. */ - if (qemuSnapshotDeleteExternalPrepareData(vm, snap, &externalData) < 0) - goto endjob; - } else { - qemuDomainJobPrivate *jobPriv = vm->job->privateData; - - externalData = g_steal_pointer(&tmpData); - - /* If the VM is running we need to indicate that the async snapshot - * job is snapshot delete job. */ - jobPriv->snapshotDelete = true; - - qemuDomainSaveStatus(vm); - } + if (qemuSnapshotDeleteExternalPrepare(vm, snap, flags, + &externalData, &stop_qemu) < 0) { + goto endjob; } } -- 2.39.2

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(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 102 ++++++++++++++++++++++++--------------- 1 file changed, 62 insertions(+), 40 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index d545c984ce..6ff18d7a9e 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2476,6 +2476,7 @@ qemuSnapshotFindParentSnapForDisk(virDomainMomentObj *snap, static int qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, virDomainMomentObj *snap, + bool blockCommit, GSList **externalData) { ssize_t i; @@ -2483,7 +2484,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) @@ -2495,56 +2495,78 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, snapDisk->name); return -1; } + } + + if (!blockCommit) { + if (!snap->parent) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("snapshot '%s' doesn't have a parent snapshot"), + snapdef->parent.name); + return -1; + } + + snapdef = virDomainSnapshotObjGetDef(snap->parent); + } + + for (i = 0; i < snapdef->ndisks; i++) { + virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]); + g_autofree qemuSnapshotDeleteExternalData *data = NULL; + + if (snapDisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + continue; data = g_new0(qemuSnapshotDeleteExternalData, 1); data->snapDisk = snapDisk; - data->domDisk = qemuDomainDiskByName(vm->def, snapDisk->name); - if (!data->domDisk) - return -1; - - data->parentDomDisk = virDomainDiskByTarget(snapdef->parent.dom, - data->snapDisk->name); - if (!data->parentDomDisk) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("failed to find disk '%s' in snapshot VM XML"), - snapDisk->name); - return -1; - } - - if (virDomainObjIsActive(vm)) { - data->diskSrc = virStorageSourceChainLookupBySource(data->domDisk->src, - data->snapDisk->src, - &data->prevDiskSrc); - if (!data->diskSrc) + if (blockCommit) { + data->domDisk = qemuDomainDiskByName(vm->def, snapDisk->name); + if (!data->domDisk) 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")); + data->parentDomDisk = virDomainDiskByTarget(snapdef->parent.dom, + data->snapDisk->name); + if (!data->parentDomDisk) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("failed to find disk '%s' in snapshot VM XML"), + snapDisk->name); return -1; } - data->parentDiskSrc = data->diskSrc->backingStore; - if (!virStorageSourceIsBacking(data->parentDiskSrc)) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("failed to find parent disk source in backing chain")); - return -1; - } + if (virDomainObjIsActive(vm)) { + data->diskSrc = virStorageSourceChainLookupBySource(data->domDisk->src, + data->snapDisk->src, + &data->prevDiskSrc); + if (!data->diskSrc) + 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 -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 -1; + } + + data->parentDiskSrc = data->diskSrc->backingStore; + if (!virStorageSourceIsBacking(data->parentDiskSrc)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("failed to find parent disk source in backing chain")); + 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 -1; + } } - } - data->parentSnap = qemuSnapshotFindParentSnapForDisk(snap, data->snapDisk); + data->parentSnap = qemuSnapshotFindParentSnapForDisk(snap, data->snapDisk); - if (data->parentSnap && !virDomainSnapshotIsExternal(data->parentSnap)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("deleting external snapshot that has internal snapshot as parent not supported")); - return -1; + if (data->parentSnap && !virDomainSnapshotIsExternal(data->parentSnap)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("deleting external snapshot that has internal snapshot as parent not supported")); + return -1; + } } ret = g_slist_prepend(ret, g_steal_pointer(&data)); @@ -2576,7 +2598,7 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, } /* this also serves as validation whether the snapshot can be deleted */ - if (qemuSnapshotDeleteExternalPrepareData(vm, snap, &tmpData) < 0) + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, true, &tmpData) < 0) return -1; if (!virDomainObjIsActive(vm)) { @@ -2591,7 +2613,7 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, /* Call the prepare again as some data require that the VM is * running to get everything we need. */ - if (qemuSnapshotDeleteExternalPrepareData(vm, snap, externalData) < 0) + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, true, externalData) < 0) return -1; } else { qemuDomainJobPrivate *jobPriv = vm->job->privateData; -- 2.39.2

In this case there is no need to run block commit and using qemu process at all. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 52 ++++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 6ff18d7a9e..be2e5c8cc4 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2597,34 +2597,40 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, return 0; } - /* this also serves as validation whether the snapshot can be deleted */ - if (qemuSnapshotDeleteExternalPrepareData(vm, snap, true, &tmpData) < 0) - return -1; - - if (!virDomainObjIsActive(vm)) { - if (qemuProcessStart(NULL, driver, vm, NULL, VIR_ASYNC_JOB_SNAPSHOT, - NULL, -1, NULL, NULL, - VIR_NETDEV_VPORT_PROFILE_OP_CREATE, - VIR_QEMU_PROCESS_START_PAUSED) < 0) { - return -1; - } - - *stop_qemu = true; - - /* Call the prepare again as some data require that the VM is - * running to get everything we need. */ - if (qemuSnapshotDeleteExternalPrepareData(vm, snap, true, externalData) < 0) + if (snap != virDomainSnapshotGetCurrent(vm->snapshots) && + snap->nchildren == 0) { + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, false, externalData) < 0) return -1; } else { - qemuDomainJobPrivate *jobPriv = vm->job->privateData; + /* this also serves as validation whether the snapshot can be deleted */ + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, true, &tmpData) < 0) + return -1; - *externalData = g_steal_pointer(&tmpData); + if (!virDomainObjIsActive(vm)) { + if (qemuProcessStart(NULL, driver, vm, NULL, VIR_ASYNC_JOB_SNAPSHOT, + NULL, -1, NULL, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_CREATE, + VIR_QEMU_PROCESS_START_PAUSED) < 0) { + return -1; + } - /* If the VM is running we need to indicate that the async snapshot - * job is snapshot delete job. */ - jobPriv->snapshotDelete = true; + *stop_qemu = true; - qemuDomainSaveStatus(vm); + /* Call the prepare again as some data require that the VM is + * running to get everything we need. */ + if (qemuSnapshotDeleteExternalPrepareData(vm, snap, true, externalData) < 0) + return -1; + } else { + qemuDomainJobPrivate *jobPriv = vm->job->privateData; + + *externalData = g_steal_pointer(&tmpData); + + /* If the VM is running we need to indicate that the async snapshot + * job is snapshot delete job. */ + jobPriv->snapshotDelete = true; + + qemuDomainSaveStatus(vm); + } } return 0; -- 2.39.2

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 | 55 +++++++++++++++++++++++++--------------- 1 file changed, 35 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index be2e5c8cc4..dbcdf56758 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2414,6 +2414,7 @@ typedef struct _qemuSnapshotDeleteExternalData { virStorageSource *prevDiskSrc; /* source of disk for which @diskSrc is backing disk */ qemuBlockJobData *job; + bool blockCommit; } qemuSnapshotDeleteExternalData; @@ -2517,8 +2518,9 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, data = g_new0(qemuSnapshotDeleteExternalData, 1); data->snapDisk = snapDisk; + data->blockCommit = blockCommit; - if (blockCommit) { + if (data->blockCommit) { data->domDisk = qemuDomainDiskByName(vm->def, snapDisk->name); if (!data->domDisk) return -1; @@ -2949,31 +2951,41 @@ 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->blockCommit) { + 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 (unlink(data->snapDisk->src->path) < 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->blockCommit) + continue; + if (qemuSnapshotDeleteBlockJobRunning(vm, data->job) < 0) goto error; @@ -2988,6 +3000,9 @@ qemuSnapshotDiscardExternal(virDomainObj *vm, for (cur = externalData; cur; cur = g_slist_next(cur)) { qemuSnapshotDeleteExternalData *data = cur->data; + if (!data->blockCommit) + continue; + if (data->job->state == QEMU_BLOCKJOB_STATE_READY) { if (qemuBlockPivot(vm, data->job, VIR_ASYNC_JOB_SNAPSHOT, NULL) < 0) goto error; -- 2.39.2

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; --- 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 dbcdf56758..513bcb5a86 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3063,6 +3063,41 @@ qemuSnapshotDiscardExternal(virDomainObj *vm, } +static int +qemuSnapshotDeleteUpdateParent(virDomainObj *vm, + virDomainMomentObj *parent) +{ + size_t i; + virQEMUDriver *driver = ((qemuDomainObjPrivate *) vm->privateData)->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, @@ -3102,6 +3137,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.39.2

This would allow creating new branch in the external snapshot history which we currently don't support. The issue with branching the external snapshot history is that deleting some snapshot would require calling block-stream instead of block-commit which is currently not implemented. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 513bcb5a86..95297cfe6a 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -665,6 +665,8 @@ qemuSnapshotPrepare(virDomainObj *vm, bool found_internal = false; bool forbid_internal = false; int external = 0; + virDomainMomentObj *curSnap = NULL; + virDomainSnapshotDef *curDef = NULL; for (i = 0; i < def->ndisks; i++) { virDomainSnapshotDiskDef *disk = &def->disks[i]; @@ -801,6 +803,16 @@ qemuSnapshotPrepare(virDomainObj *vm, return -1; } + curSnap = virDomainSnapshotGetCurrent(vm->snapshots); + if (curSnap) + curDef = virDomainSnapshotObjGetDef(curSnap); + + if (curDef && curDef->revertdisks) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("creating external snapshot after reverting to not last snapshot is not supported")); + return -1; + } + /* Alter flags to let later users know what we learned. */ if (external && !active) *flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; -- 2.39.2

This new helper will allow us to check if we are able to delete external snapshot after user did revert to non-leaf snapshot. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/conf/virdomainmomentobjlist.c | 21 +++++++++++++++++++++ src/conf/virdomainmomentobjlist.h | 4 ++++ src/libvirt_private.syms | 1 + 3 files changed, 26 insertions(+) diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c index f19ec3319a..0c520fb173 100644 --- a/src/conf/virdomainmomentobjlist.c +++ b/src/conf/virdomainmomentobjlist.c @@ -582,3 +582,24 @@ virDomainMomentFindLeaf(virDomainMomentObjList *list) return moment; return NULL; } + + +bool +virDomainMomentIsAncestor(virDomainMomentObj *moment, + virDomainMomentObj *ancestor) +{ + if (!moment) + return false; + + if (moment == ancestor) + return false; + + do { + moment = moment->parent; + + if (moment == ancestor) + return true; + } while (moment); + + return false; +} diff --git a/src/conf/virdomainmomentobjlist.h b/src/conf/virdomainmomentobjlist.h index d2ab3b46b1..2ea6b181c0 100644 --- a/src/conf/virdomainmomentobjlist.h +++ b/src/conf/virdomainmomentobjlist.h @@ -157,3 +157,7 @@ virDomainMomentCheckCycles(virDomainMomentObjList *list, virDomainMomentObj * virDomainMomentFindLeaf(virDomainMomentObjList *list); + +bool +virDomainMomentIsAncestor(virDomainMomentObj *moment, + virDomainMomentObj *ancestor); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dd752ba55c..8a724ef241 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1222,6 +1222,7 @@ virDomainMomentDropChildren; virDomainMomentDropParent; virDomainMomentForEachChild; virDomainMomentForEachDescendant; +virDomainMomentIsAncestor; virDomainMomentMoveChildren; virDomainMomentObjFree; virDomainMomentObjNew; -- 2.39.2

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; --- 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 95297cfe6a..05386b11fe 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3426,18 +3426,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.39.2

With the introduction of external snapshot revert support we need to error out in some cases when trying to delete some snapshots. If users reverts to non-leaf snapshots and would try to delete it after the revert is done it would not work currently as this operation would require using block-stream which is not implemented for now as in this case the snapshot has two children so the disk files have multiple overlays. Similarly if user reverts to non-leaf snapshot and would try to delete snapshot that is non-leaf but not in currently active snapshot chain we would still need to use block-commit operation. The issue here is that in order to do that we would have to start new qemu process with different domain definition than what is currently used by the domain. If the current domain would be running it would complicate things even more so this operation is not yet supported. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 05386b11fe..c6c7b077dd 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3427,6 +3427,8 @@ qemuSnapshotDeleteValidate(virDomainObj *vm, } if (virDomainSnapshotIsExternal(snap)) { + virDomainMomentObj *current = virDomainSnapshotGetCurrent(vm->snapshots); + if (qemuDomainHasBlockjob(vm, false)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("cannot delete external snapshots when there is another active block job")); @@ -3438,6 +3440,19 @@ qemuSnapshotDeleteValidate(virDomainObj *vm, _("deletion of external disk snapshots with children not supported")); return -1; } + + if (snap == current && snap->nchildren != 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("deletion of active external snapshot that is not a leaf snapshot is not supported")); + return -1; + } + + if (snap != current && snap->nchildren != 0 && + virDomainMomentIsAncestor(snap, current)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("deletion of non-leaf external snapshot that is not in active chain is not supported ")); + return -1; + } } return 0; -- 2.39.2