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 aeb54d8952..a75156f097 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 | 30 +++++++++++++++++++++++++++++ src/conf/snapshot_conf.h | 5 +++++ 3 files changed, 42 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..8ecf311a17 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -112,6 +112,9 @@ virDomainSnapshotDefDispose(void *obj) for (i = 0; i < def->ndisks; i++) virDomainSnapshotDiskDefClear(&def->disks[i]); g_free(def->disks); + for (i = 0; i < def->nrevertdisks; i++) + virDomainSnapshotDiskDefClear(&def->revertdisks[i]); + g_free(def->revertdisks); virObjectUnref(def->cookie); } @@ -376,6 +379,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 +853,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

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; Reviewed-by: Peter Krempa <pkrempa(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 844b02d427..72a0f71d4f 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.41.0

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 | 85 ++++++++++++++++++++++++++-------------- 1 file changed, 56 insertions(+), 29 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 8e1eb21b5d..f43548e29f 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -181,43 +181,37 @@ qemuSnapshotDomainDefUpdateDisk(virDomainDef *domdef, } -/* The domain is expected to be locked and inactive. */ +/** + * qemuSnapshotCreateQcow2Files: + * @vm: domain object + * @snapdef: snapshot definition + * @created: bitmap to store which disks were created + * + * Create new qcow2 images based on snapshot definition @snapdef and use + * domain object @vm as source for backing images. + * + * Returns 0 on success, -1 on error. + */ 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) { 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 - * create them correctly. */ - for (i = 0; i < snapdef->ndisks && !reuse; i++) { + for (i = 0; i < snapdef->ndisks; i++) { 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 +219,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 +228,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 +245,42 @@ 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 reuse is true, then qemuSnapshotPrepare already + * ensured that the new files exist, and it was up to the user to + * create them correctly. */ + if (!reuse && qemuSnapshotCreateQcow2Files(vm, snapdef, created) < 0) + goto cleanup; + /* update disk definitions */ if (qemuSnapshotDomainDefUpdateDisk(vm->def, snapdef, reuse) < 0) goto cleanup; -- 2.41.0

We will need to reuse the functionality when reverting external snapshots. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 50f871baf3..a090ca13d4 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -527,6 +527,25 @@ qemuSnapshotPrepareDiskExternal(virDomainDiskDef *disk, bool active, bool reuse) { + if (!snapdisk->src->format) { + snapdisk->src->format = VIR_STORAGE_FILE_QCOW2; + } else if (snapdisk->src->format != VIR_STORAGE_FILE_QCOW2 && + snapdisk->src->format != VIR_STORAGE_FILE_QED) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("external snapshot format for disk %1$s is unsupported: %2$s"), + snapdisk->name, + virStorageFileFormatTypeToString(snapdisk->src->format)); + return -1; + } + + if (snapdisk->src->metadataCacheMaxSize > 0) { + if (snapdisk->src->format != VIR_STORAGE_FILE_QCOW2) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("metadata cache max size control is supported only with qcow2 images")); + return -1; + } + } + if (qemuTranslateSnapshotDiskSourcePool(snapdisk) < 0) return -1; @@ -707,25 +726,6 @@ qemuSnapshotPrepare(virDomainObj *vm, break; case VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL: - if (!disk->src->format) { - disk->src->format = VIR_STORAGE_FILE_QCOW2; - } else if (disk->src->format != VIR_STORAGE_FILE_QCOW2 && - disk->src->format != VIR_STORAGE_FILE_QED) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("external snapshot format for disk %1$s is unsupported: %2$s"), - disk->name, - virStorageFileFormatTypeToString(disk->src->format)); - return -1; - } - - if (disk->src->metadataCacheMaxSize > 0) { - if (disk->src->format != VIR_STORAGE_FILE_QCOW2) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("metadata cache max size control is supported only with qcow2 images")); - return -1; - } - } - if (qemuSnapshotPrepareDiskExternal(dom_disk, disk, active, reuse) < 0) return -1; -- 2.41.0

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; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 278 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 274 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 82fbb79ab0..0534989e37 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" @@ -1982,6 +1984,229 @@ qemuSnapshotRevertWriteMetadata(virDomainObj *vm, } +/** + * qemuSnapshotRevertExternalPrepare: + * @vm: domain object + * @tmpsnapdef: temporary snapshot definition + * @snap: snapshot object we are reverting to + * @config: live domain definition + * @inactiveConfig: offline domain definition + * memsnapFD: pointer to store memory state file FD or NULL + * memsnapPath: pointer to store memory state file path or NULL + * + * Prepare new temporary snapshot definition @tmpsnapdef that will + * be used while creating new overlay files after reverting to snapshot + * @snap. In case we are reverting to snapshot with memory state it will + * open it and pass FD via @memsnapFD and path to the file via + * @memsnapPath, caller is responsible for freeing both @memsnapFD and + * memsnapPath. + * + * Returns 0 in success, -1 on error. + */ +static int +qemuSnapshotRevertExternalPrepare(virDomainObj *vm, + virDomainSnapshotDef *tmpsnapdef, + virDomainMomentObj *snap, + virDomainDef *config, + virDomainDef *inactiveConfig, + int *memsnapFD, + char **memsnapPath) +{ + size_t i; + bool active = virDomainObjIsActive(vm); + virDomainDef *domdef = NULL; + 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, + VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL, + 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; +} + + +/** + * qemuSnapshotRevertExternalActive: + * @vm: domain object + * @tmpsnapdef: temporary snapshot definition + * + * Creates a new disk overlays using the temporary snapshot + * definition @tmpsnapdef for running VM by calling QMP APIs. + * + * Returns 0 on success, -1 on error. + */ +static int +qemuSnapshotRevertExternalActive(virDomainObj *vm, + virDomainSnapshotDef *tmpsnapdef) +{ + size_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; +} + + +/** + * qemuSnapshotRevertExternalInactive: + * @vm: domain object + * @tmpsnapdef: temporary snapshot definition + * @domdef: offline domain definition + * + * Creates a new disk overlays using the temporary snapshot + * definition @tmpsnapdef for offline VM by calling qemu-img. + * + * Returns 0 on success, -1 on error. + */ +static int +qemuSnapshotRevertExternalInactive(virDomainObj *vm, + virDomainSnapshotDef *tmpsnapdef, + virDomainDef *domdef) +{ + virQEMUDriver *driver = QEMU_DOMAIN_PRIVATE(vm)->driver; + g_autoptr(virBitmap) created = NULL; + + created = virBitmapNew(tmpsnapdef->ndisks); + + if (qemuSnapshotDomainDefUpdateDisk(domdef, tmpsnapdef, false) < 0) + return -1; + + if (qemuSnapshotCreateQcow2Files(driver, domdef, tmpsnapdef, created) < 0) { + ssize_t bit = -1; + virErrorPtr err = NULL; + + virErrorPreserveLast(&err); + + 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); + } + } + + virErrorRestore(&err); + + return -1; + } + + return 0; +} + + +/** + * qemuSnapshotRevertExternalFinish: + * @vm: domain object + * @tmpsnapdef: temporary snapshot definition + * @snap: snapshot object we are reverting to + * + * Finishes disk overlay creation by removing existing overlays that + * will no longer be used if there are any and updating snapshot @snap + * metadata and current snapshot metadata so it can be saved once the + * revert is completed. + */ +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, @@ -1996,10 +2221,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; @@ -2017,6 +2246,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; @@ -2033,7 +2275,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); @@ -2045,6 +2288,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 || @@ -2119,6 +2370,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 @@ -2138,9 +2390,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, + *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

This part of code is about to grow to make deletion work when user reverts to non-leaf snapshot. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 104 ++++++++++++++++++++++++++------------- 1 file changed, 70 insertions(+), 34 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index dd88e478b8..c3d17b4cee 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2706,6 +2706,73 @@ qemuSnapshotDeleteExternalPrepareData(virDomainObj *vm, } +/** + * qemuSnapshotDeleteExternalPrepare: + * @vm: domain object + * @snap: snapshot object we are deleting + * @flags: flags passed to virDomainSnapshotDelete + * @externalData: pointer to GSList of qemuSnapshotDeleteExternalData + * @stop_qemu: pointer to boolean indicating QEMU process was started + * + * Validates and prepares data for snapshot @snap we are deleting and + * store it in @externalData. For offline VMs we need to start QEMU + * process in order to delete external snapshots and caller will need + * to stop that process, @stop_qemu will be set to True. + * + * Return 0 on success, -1 on error. + */ +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; @@ -3476,40 +3543,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.41.0

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; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 55 +++++++++++++++++++++++----------------- 1 file changed, 32 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index d47dc72709..8b005cae8f 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2759,34 +2759,43 @@ 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) + /* Leaf non-active snapshot doesn't have overlay files for the disk images + * so there is no need to do any merge and we can just delete the files + * directly. */ + 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.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 a67be1ae49..6dd420a53b 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3229,6 +3229,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, @@ -3268,6 +3303,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

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; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/virdomainmomentobjlist.c | 17 +++++++++++++++++ src/conf/virdomainmomentobjlist.h | 4 ++++ src/libvirt_private.syms | 1 + 3 files changed, 22 insertions(+) diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c index e5cdd9a141..ea9850df8c 100644 --- a/src/conf/virdomainmomentobjlist.c +++ b/src/conf/virdomainmomentobjlist.c @@ -582,3 +582,20 @@ virDomainMomentFindLeaf(virDomainMomentObjList *list) return moment; return NULL; } + + +/* Check if @moment is descendant of @ancestor. */ +bool +virDomainMomentIsAncestor(virDomainMomentObj *moment, + virDomainMomentObj *ancestor) +{ + if (moment == ancestor) + return false; + + for (moment = moment->parent; moment; moment = moment->parent) { + if (moment == ancestor) + return true; + } + + 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 a75156f097..1cf40ba863 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1221,6 +1221,7 @@ virDomainMomentDropChildren; virDomainMomentDropParent; virDomainMomentForEachChild; virDomainMomentForEachDescendant; +virDomainMomentIsAncestor; virDomainMomentMoveChildren; virDomainMomentObjFree; virDomainMomentObjNew; -- 2.41.0

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 | 122 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 122 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 0238ab2249..8d0581d33b 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2588,6 +2588,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 data for which the + deleted storage source is backing store */ qemuBlockJobData *job; bool merge; } qemuSnapshotDeleteExternalData; @@ -2600,6 +2602,7 @@ qemuSnapshotDeleteExternalDataFree(qemuSnapshotDeleteExternalData *data) return; virObjectUnref(data->job); + g_slist_free_full(data->disksWithBacking, g_free); g_free(data); } @@ -2649,6 +2652,84 @@ qemuSnapshotFindParentSnapForDisk(virDomainMomentObj *snap, } +struct _qemuSnapshotDisksWithBackingStoreData { + virStorageSource *diskSrc; + uid_t uid; + gid_t gid; +}; + + +struct _qemuSnapshotDisksWithBackingStoreIterData { + virDomainMomentObj *current; + virStorageSource *diskSrc; + GSList **disksWithBacking; + virQEMUDriverConfig *cfg; +}; + + +static int +qemuSnapshotDiskHasBackingDisk(void *payload, + const char *name G_GNUC_UNUSED, + void *opaque) +{ + virDomainMomentObj *snap = payload; + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); + struct _qemuSnapshotDisksWithBackingStoreIterData *iterdata = opaque; + ssize_t i; + + /* skip snapshots that are within the active snapshot tree as it will be handled + * by qemu */ + if (virDomainMomentIsAncestor(iterdata->current, snap) || iterdata->current == snap) + return 0; + + for (i = 0; i < snapdef->parent.dom->ndisks; i++) { + virDomainDiskDef *disk = snapdef->parent.dom->disks[i]; + uid_t uid; + gid_t gid; + + if (!virStorageSourceIsLocalStorage(disk->src)) + continue; + + qemuDomainGetImageIds(iterdata->cfg, snapdef->parent.dom, disk->src, + NULL, &uid, &gid); + + if (!disk->src->backingStore) + ignore_value(virStorageSourceGetMetadata(disk->src, uid, gid, 1, false)); + + if (virStorageSourceIsSameLocation(disk->src->backingStore, iterdata->diskSrc)) { + struct _qemuSnapshotDisksWithBackingStoreData *data = + g_new0(struct _qemuSnapshotDisksWithBackingStoreData, 1); + + data->diskSrc = disk->src; + data->uid = uid; + data->gid = gid; + + *iterdata->disksWithBacking = g_slist_prepend(*iterdata->disksWithBacking, data); + } + } + + return 0; +} + + +static void +qemuSnapshotGetDisksWithBackingStore(virDomainObj *vm, + virDomainMomentObj *snap, + qemuSnapshotDeleteExternalData *data) +{ + struct _qemuSnapshotDisksWithBackingStoreIterData iterData; + virQEMUDriver *driver = QEMU_DOMAIN_PRIVATE(vm)->driver; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + + iterData.current = virDomainSnapshotGetCurrent(vm->snapshots); + iterData.diskSrc = data->diskSrc; + iterData.disksWithBacking = &data->disksWithBacking; + iterData.cfg = cfg; + + virDomainMomentForEachDescendant(snap, qemuSnapshotDiskHasBackingDisk, &iterData); +} + + /** * qemuSnapshotDeleteExternalPrepareData: * @vm: domain object @@ -2733,6 +2814,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); @@ -3129,6 +3212,43 @@ 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)) { + struct _qemuSnapshotDisksWithBackingStoreData *backingData = 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(backingData->diskSrc->format), + "-b", + data->parentDiskSrc->path, + backingData->diskSrc->path, + NULL))) + continue; + + virCommandSetUID(cmd, backingData->uid); + virCommandSetGID(cmd, backingData->gid); + + ignore_value(virCommandRun(cmd, NULL)); + } +} + + static int qemuSnapshotDiscardExternal(virDomainObj *vm, virDomainMomentObj *snap, @@ -3215,6 +3335,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

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. If user creates new snapshot after reverting to non-leaf snapshot it creates a new branch. Deleting snapshot with multiple children will require block-stream which is not implemented for now. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index dca95b415b..692d88e24d 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3729,6 +3729,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")); @@ -3740,6 +3742,25 @@ 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; + } + + if (snap->nchildren > 1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("deletion of external disk snapshot with multiple children snapshots not supported")); + return -1; + } } return 0; -- 2.41.0