Move the code for finishing a job in the ready state to qemu_block.c. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 121 ++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_block.h | 5 ++ src/qemu/qemu_driver.c | 123 +---------------------------------------- 3 files changed, 127 insertions(+), 122 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 4cca7555f3..721145fa42 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3375,3 +3375,124 @@ qemuBlockCommit(virDomainObj *vm, return -1; } + + +/* Called while holding the VM job lock, to implement a block job + * abort with pivot; this updates the VM definition as appropriate, on + * either success or failure. */ +int +qemuBlockPivot(virDomainObj *vm, + qemuBlockJobData *job, + virDomainDiskDef *disk) +{ + g_autoptr(qemuBlockStorageSourceChainData) chainattachdata = NULL; + int ret = -1; + qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(virJSONValue) bitmapactions = NULL; + g_autoptr(virJSONValue) reopenactions = NULL; + int rc = 0; + + if (job->state != QEMU_BLOCKJOB_STATE_READY) { + virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, + _("block job '%s' not ready for pivot yet"), + job->name); + return -1; + } + + switch ((qemuBlockJobType) job->type) { + case QEMU_BLOCKJOB_TYPE_NONE: + case QEMU_BLOCKJOB_TYPE_LAST: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid job type '%d'"), job->type); + return -1; + + case QEMU_BLOCKJOB_TYPE_PULL: + case QEMU_BLOCKJOB_TYPE_COMMIT: + case QEMU_BLOCKJOB_TYPE_BACKUP: + case QEMU_BLOCKJOB_TYPE_INTERNAL: + case QEMU_BLOCKJOB_TYPE_CREATE: + case QEMU_BLOCKJOB_TYPE_BROKEN: + virReportError(VIR_ERR_OPERATION_INVALID, + _("job type '%s' does not support pivot"), + qemuBlockjobTypeToString(job->type)); + return -1; + + case QEMU_BLOCKJOB_TYPE_COPY: + if (!job->jobflagsmissing) { + bool shallow = job->jobflags & VIR_DOMAIN_BLOCK_COPY_SHALLOW; + bool reuse = job->jobflags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT; + + bitmapactions = virJSONValueNewArray(); + + if (qemuMonitorTransactionBitmapAdd(bitmapactions, + disk->mirror->nodeformat, + "libvirt-tmp-activewrite", + false, + false, + 0) < 0) + return -1; + + /* Open and install the backing chain of 'mirror' late if we can use + * blockdev-snapshot to do it. This is to appease oVirt that wants + * to copy data into the backing chain while the top image is being + * copied shallow */ + if (reuse && shallow && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV_SNAPSHOT_ALLOW_WRITE_ONLY) && + virStorageSourceHasBacking(disk->mirror)) { + + if (!(chainattachdata = qemuBuildStorageSourceChainAttachPrepareBlockdev(disk->mirror->backingStore))) + return -1; + + reopenactions = virJSONValueNewArray(); + + if (qemuMonitorTransactionSnapshotBlockdev(reopenactions, + disk->mirror->backingStore->nodeformat, + disk->mirror->nodeformat)) + return -1; + } + + } + break; + + case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT: + bitmapactions = virJSONValueNewArray(); + + if (qemuMonitorTransactionBitmapAdd(bitmapactions, + job->data.commit.base->nodeformat, + "libvirt-tmp-activewrite", + false, + false, + 0) < 0) + return -1; + + break; + } + + qemuDomainObjEnterMonitor(vm); + + if (chainattachdata) { + if ((rc = qemuBlockStorageSourceChainAttach(priv->mon, chainattachdata)) == 0) { + /* install backing images on success, or unplug them on failure */ + if ((rc = qemuMonitorTransaction(priv->mon, &reopenactions)) != 0) + qemuBlockStorageSourceChainDetach(priv->mon, chainattachdata); + } + } + + if (bitmapactions && rc == 0) + ignore_value(qemuMonitorTransaction(priv->mon, &bitmapactions)); + + if (rc == 0) + ret = qemuMonitorJobComplete(priv->mon, job->name); + + qemuDomainObjExitMonitor(vm); + + /* The pivot failed. The block job in QEMU remains in the synchronised state */ + if (ret < 0) + return -1; + + if (disk && disk->mirror) + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT; + job->state = QEMU_BLOCKJOB_STATE_PIVOTING; + + return ret; +} diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 85b4805d89..52deb15a3d 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -285,3 +285,8 @@ qemuBlockCommit(virDomainObj *vm, virStorageSource *top_parent, unsigned long bandwidth, unsigned int flags); + +int +qemuBlockPivot(virDomainObj *vm, + qemuBlockJobData *job, + virDomainDiskDef *disk); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2f05da3d8c..d8db7188f7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14001,127 +14001,6 @@ qemuDomainOpenChannel(virDomainPtr dom, } -/* Called while holding the VM job lock, to implement a block job - * abort with pivot; this updates the VM definition as appropriate, on - * either success or failure. */ -static int -qemuDomainBlockPivot(virDomainObj *vm, - qemuBlockJobData *job, - virDomainDiskDef *disk) -{ - g_autoptr(qemuBlockStorageSourceChainData) chainattachdata = NULL; - int ret = -1; - qemuDomainObjPrivate *priv = vm->privateData; - g_autoptr(virJSONValue) bitmapactions = NULL; - g_autoptr(virJSONValue) reopenactions = NULL; - int rc = 0; - - if (job->state != QEMU_BLOCKJOB_STATE_READY) { - virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, - _("block job '%s' not ready for pivot yet"), - job->name); - return -1; - } - - switch ((qemuBlockJobType) job->type) { - case QEMU_BLOCKJOB_TYPE_NONE: - case QEMU_BLOCKJOB_TYPE_LAST: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("invalid job type '%d'"), job->type); - return -1; - - case QEMU_BLOCKJOB_TYPE_PULL: - case QEMU_BLOCKJOB_TYPE_COMMIT: - case QEMU_BLOCKJOB_TYPE_BACKUP: - case QEMU_BLOCKJOB_TYPE_INTERNAL: - case QEMU_BLOCKJOB_TYPE_CREATE: - case QEMU_BLOCKJOB_TYPE_BROKEN: - virReportError(VIR_ERR_OPERATION_INVALID, - _("job type '%s' does not support pivot"), - qemuBlockjobTypeToString(job->type)); - return -1; - - case QEMU_BLOCKJOB_TYPE_COPY: - if (!job->jobflagsmissing) { - bool shallow = job->jobflags & VIR_DOMAIN_BLOCK_COPY_SHALLOW; - bool reuse = job->jobflags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT; - - bitmapactions = virJSONValueNewArray(); - - if (qemuMonitorTransactionBitmapAdd(bitmapactions, - disk->mirror->nodeformat, - "libvirt-tmp-activewrite", - false, - false, - 0) < 0) - return -1; - - /* Open and install the backing chain of 'mirror' late if we can use - * blockdev-snapshot to do it. This is to appease oVirt that wants - * to copy data into the backing chain while the top image is being - * copied shallow */ - if (reuse && shallow && - virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV_SNAPSHOT_ALLOW_WRITE_ONLY) && - virStorageSourceHasBacking(disk->mirror)) { - - if (!(chainattachdata = qemuBuildStorageSourceChainAttachPrepareBlockdev(disk->mirror->backingStore))) - return -1; - - reopenactions = virJSONValueNewArray(); - - if (qemuMonitorTransactionSnapshotBlockdev(reopenactions, - disk->mirror->backingStore->nodeformat, - disk->mirror->nodeformat)) - return -1; - } - - } - break; - - case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT: - bitmapactions = virJSONValueNewArray(); - - if (qemuMonitorTransactionBitmapAdd(bitmapactions, - job->data.commit.base->nodeformat, - "libvirt-tmp-activewrite", - false, - false, - 0) < 0) - return -1; - - break; - } - - qemuDomainObjEnterMonitor(vm); - - if (chainattachdata) { - if ((rc = qemuBlockStorageSourceChainAttach(priv->mon, chainattachdata)) == 0) { - /* install backing images on success, or unplug them on failure */ - if ((rc = qemuMonitorTransaction(priv->mon, &reopenactions)) != 0) - qemuBlockStorageSourceChainDetach(priv->mon, chainattachdata); - } - } - - if (bitmapactions && rc == 0) - ignore_value(qemuMonitorTransaction(priv->mon, &bitmapactions)); - - if (rc == 0) - ret = qemuMonitorJobComplete(priv->mon, job->name); - - qemuDomainObjExitMonitor(vm); - - /* The pivot failed. The block job in QEMU remains in the synchronised state */ - if (ret < 0) - return -1; - - if (disk && disk->mirror) - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT; - job->state = QEMU_BLOCKJOB_STATE_PIVOTING; - - return ret; -} - - /* bandwidth in MiB/s per public API. Caller must lock vm beforehand, * and not access it afterwards. */ static int @@ -14284,7 +14163,7 @@ qemuDomainBlockJobAbort(virDomainPtr dom, qemuBlockJobSyncBegin(job); if (pivot) { - if ((ret = qemuDomainBlockPivot(vm, job, disk)) < 0) + if ((ret = qemuBlockPivot(vm, job, disk)) < 0) goto endjob; } else { qemuDomainObjEnterMonitor(vm); -- 2.38.1

Extract code that deletes single snapshot to separate function. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 47 ++++++++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index e9bb5abee2..47239e9e9c 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2281,6 +2281,36 @@ qemuSnapshotChildrenReparent(void *payload, } +static int +qemuSnapshotDeleteSingle(virDomainObj *vm, + virDomainMomentObj *snap, + bool metadata_only) +{ + qemuDomainObjPrivate *priv = vm->privateData; + virQEMUDriver *driver = priv->driver; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + + if (snap->nchildren) { + virQEMUMomentReparent rep; + + rep.dir = cfg->snapshotDir; + rep.parent = snap->parent; + rep.vm = vm; + rep.err = 0; + rep.xmlopt = driver->xmlopt; + rep.writeMetadata = qemuDomainSnapshotWriteMetadata; + virDomainMomentForEachChild(snap, + qemuSnapshotChildrenReparent, + &rep); + if (rep.err < 0) + return -1; + virDomainMomentMoveChildren(snap, snap->parent); + } + + return qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); +} + + int qemuSnapshotDelete(virDomainObj *vm, virDomainSnapshotPtr snapshot, @@ -2290,7 +2320,6 @@ qemuSnapshotDelete(virDomainObj *vm, int ret = -1; virDomainMomentObj *snap = NULL; virQEMUMomentRemove rem; - virQEMUMomentReparent rep; bool metadata_only = !!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY); int external = 0; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); @@ -2358,21 +2387,7 @@ qemuSnapshotDelete(virDomainObj *vm, ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); } } else { - if (snap->nchildren) { - rep.dir = cfg->snapshotDir; - rep.parent = snap->parent; - rep.vm = vm; - rep.err = 0; - rep.xmlopt = driver->xmlopt; - rep.writeMetadata = qemuDomainSnapshotWriteMetadata; - virDomainMomentForEachChild(snap, - qemuSnapshotChildrenReparent, - &rep); - if (rep.err < 0) - goto endjob; - virDomainMomentMoveChildren(snap, snap->parent); - } - ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); + ret = qemuSnapshotDeleteSingle(vm, snap, metadata_only); } endjob: -- 2.38.1

Extract code that deletes children of specific snapshot to separate function. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 108 ++++++++++++++++++++++++--------------- 1 file changed, 67 insertions(+), 41 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 47239e9e9c..3d5467457b 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2311,18 +2311,76 @@ qemuSnapshotDeleteSingle(virDomainObj *vm, } -int -qemuSnapshotDelete(virDomainObj *vm, - virDomainSnapshotPtr snapshot, - unsigned int flags) +/** + * qemuSnapshotDeleteChildren: + * @vm: domain object + * @snap: snapshot object + * @metadata_only: boolean + * @flags: bitwise-OR of virDomainSnapshotDeleteFlags + * + * Delete children snapshots of snapshot proved by @snap. Based on what @flags + * are provided it will delete children snapshots including @snap or only + * children snapshots. If @metadata_only is true only libvirt metadata files + * are deleted but the actual snapshots are left intact. + * + * Returns 0 on success, -1 on error. + */ +static int +qemuSnapshotDeleteChildren(virDomainObj *vm, + virDomainMomentObj *snap, + bool metadata_only, + unsigned int flags) { - virQEMUDriver *driver = snapshot->domain->conn->privateData; - int ret = -1; - virDomainMomentObj *snap = NULL; virQEMUMomentRemove rem; + qemuDomainObjPrivate *priv = vm->privateData; + virQEMUDriver *driver = priv->driver; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + + rem.driver = driver; + rem.vm = vm; + rem.metadata_only = metadata_only; + rem.err = 0; + rem.current = virDomainSnapshotGetCurrent(vm->snapshots); + rem.found = false; + rem.momentDiscard = qemuDomainSnapshotDiscard; + virDomainMomentForEachDescendant(snap, qemuDomainMomentDiscardAll, + &rem); + if (rem.err < 0) + return -1; + if (rem.found) { + qemuSnapshotSetCurrent(vm, snap); + + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { + if (qemuDomainSnapshotWriteMetadata(vm, snap, + driver->xmlopt, + cfg->snapshotDir) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to set snapshot '%s' as current"), + snap->def->name); + virDomainSnapshotSetCurrent(vm->snapshots, NULL); + return -1; + } + } + } + + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { + virDomainMomentDropChildren(snap); + return 0; + } + + return qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); +} + + +int +qemuSnapshotDelete(virDomainObj *vm, + virDomainSnapshotPtr snapshot, + unsigned int flags) +{ + int ret = -1; + virDomainMomentObj *snap = NULL; bool metadata_only = !!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY); int external = 0; - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY | @@ -2353,39 +2411,7 @@ qemuSnapshotDelete(virDomainObj *vm, if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { - rem.driver = driver; - rem.vm = vm; - rem.metadata_only = metadata_only; - rem.err = 0; - rem.current = virDomainSnapshotGetCurrent(vm->snapshots); - rem.found = false; - rem.momentDiscard = qemuDomainSnapshotDiscard; - virDomainMomentForEachDescendant(snap, qemuDomainMomentDiscardAll, - &rem); - if (rem.err < 0) - goto endjob; - if (rem.found) { - qemuSnapshotSetCurrent(vm, snap); - - if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { - if (qemuDomainSnapshotWriteMetadata(vm, snap, - driver->xmlopt, - cfg->snapshotDir) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to set snapshot '%s' as current"), - snap->def->name); - virDomainSnapshotSetCurrent(vm->snapshots, NULL); - goto endjob; - } - } - } - - if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { - virDomainMomentDropChildren(snap); - ret = 0; - } else { - ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); - } + ret = qemuSnapshotDeleteChildren(vm, snap, metadata_only, flags); } else { ret = qemuSnapshotDeleteSingle(vm, snap, metadata_only); } -- 2.38.1

This simplifies the code a bit by reusing existing parts that deletes a single snapshot. The drawback of this change is that we will now call the re-parent bits to keep the metadata in sync for every child even though it will get deleted as well. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 69 +++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 33 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 3d5467457b..846f6e9e53 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2311,6 +2311,31 @@ qemuSnapshotDeleteSingle(virDomainObj *vm, } +struct qemuSnapshotDeleteAllData { + virDomainObj *vm; + bool metadata_only; + int error; +}; + + +static int +qemuSnapshotDeleteAll(void *payload, + const char *name G_GNUC_UNUSED, + void *opaque) +{ + int error; + virDomainMomentObj *snap = payload; + struct qemuSnapshotDeleteAllData *data = opaque; + + error = qemuSnapshotDeleteSingle(data->vm, snap, data->metadata_only); + + if (error != 0) + data->error = error; + + return 0; +} + + /** * qemuSnapshotDeleteChildren: * @vm: domain object @@ -2331,44 +2356,22 @@ qemuSnapshotDeleteChildren(virDomainObj *vm, bool metadata_only, unsigned int flags) { - virQEMUMomentRemove rem; - qemuDomainObjPrivate *priv = vm->privateData; - virQEMUDriver *driver = priv->driver; - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + struct qemuSnapshotDeleteAllData data = { + .vm = vm, + .metadata_only = metadata_only, + }; - rem.driver = driver; - rem.vm = vm; - rem.metadata_only = metadata_only; - rem.err = 0; - rem.current = virDomainSnapshotGetCurrent(vm->snapshots); - rem.found = false; - rem.momentDiscard = qemuDomainSnapshotDiscard; - virDomainMomentForEachDescendant(snap, qemuDomainMomentDiscardAll, - &rem); - if (rem.err < 0) - return -1; - if (rem.found) { - qemuSnapshotSetCurrent(vm, snap); + virDomainMomentForEachDescendant(snap, qemuSnapshotDeleteAll, &data); - if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { - if (qemuDomainSnapshotWriteMetadata(vm, snap, - driver->xmlopt, - cfg->snapshotDir) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to set snapshot '%s' as current"), - snap->def->name); - virDomainSnapshotSetCurrent(vm->snapshots, NULL); - return -1; - } - } - } + if (data.error < 0) + return -1; - if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { - virDomainMomentDropChildren(snap); - return 0; + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN && + qemuSnapshotDeleteSingle(vm, snap, metadata_only) < 0) { + return -1; } - return qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); + return 0; } -- 2.38.1

Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 95 +-------------------------------------- src/qemu/qemu_domain.h | 9 ---- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_snapshot.c | 96 +++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_snapshot.h | 4 ++ 5 files changed, 102 insertions(+), 104 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8892f28fce..a6bb4a01df 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -31,6 +31,7 @@ #include "qemu_migration_params.h" #include "qemu_security.h" #include "qemu_slirp.h" +#include "qemu_snapshot.h" #include "qemu_extdevice.h" #include "qemu_blockjob.h" #include "qemu_checkpoint.h" @@ -7139,81 +7140,6 @@ qemuDomainSnapshotForEachQcow2(virQEMUDriver *driver, op, try_all, def->ndisks); } -/* Discard one snapshot (or its metadata), without reparenting any children. */ -int -qemuDomainSnapshotDiscard(virQEMUDriver *driver, - virDomainObj *vm, - virDomainMomentObj *snap, - bool update_parent, - bool metadata_only) -{ - g_autofree char *snapFile = NULL; - qemuDomainObjPrivate *priv; - virDomainMomentObj *parentsnap = NULL; - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - - if (!metadata_only) { - if (!virDomainObjIsActive(vm)) { - size_t i; - /* Ignore any skipped disks */ - - /* Prefer action on the disks in use at the time the snapshot was - * created; but fall back to current definition if dealing with a - * snapshot created prior to libvirt 0.9.5. */ - virDomainDef *def = snap->def->dom; - - if (!def) - def = vm->def; - - for (i = 0; i < def->ndisks; i++) { - if (virDomainDiskTranslateSourcePool(def->disks[i]) < 0) - return -1; - } - - if (qemuDomainSnapshotForEachQcow2(driver, def, snap, "-d", true) < 0) - return -1; - } else { - priv = vm->privateData; - qemuDomainObjEnterMonitor(vm); - /* we continue on even in the face of error */ - qemuMonitorDeleteSnapshot(priv->mon, snap->def->name); - qemuDomainObjExitMonitor(vm); - } - } - - snapFile = g_strdup_printf("%s/%s/%s.xml", cfg->snapshotDir, vm->def->name, - snap->def->name); - - if (snap == virDomainSnapshotGetCurrent(vm->snapshots)) { - virDomainSnapshotSetCurrent(vm->snapshots, NULL); - if (update_parent && snap->def->parent_name) { - parentsnap = virDomainSnapshotFindByName(vm->snapshots, - snap->def->parent_name); - if (!parentsnap) { - VIR_WARN("missing parent snapshot matching name '%s'", - snap->def->parent_name); - } else { - virDomainSnapshotSetCurrent(vm->snapshots, parentsnap); - if (qemuDomainSnapshotWriteMetadata(vm, parentsnap, - driver->xmlopt, - cfg->snapshotDir) < 0) { - VIR_WARN("failed to set parent snapshot '%s' as current", - snap->def->parent_name); - virDomainSnapshotSetCurrent(vm->snapshots, NULL); - } - } - } - } - - if (unlink(snapFile) < 0) - VIR_WARN("Failed to unlink %s", snapFile); - if (update_parent) - virDomainMomentDropParent(snap); - virDomainSnapshotObjListRemove(vm->snapshots, snap); - - return 0; -} - /* Hash iterator callback to discard multiple snapshots. */ int qemuDomainMomentDiscardAll(void *payload, const char *name G_GNUC_UNUSED, @@ -7232,23 +7158,6 @@ int qemuDomainMomentDiscardAll(void *payload, return 0; } -int -qemuDomainSnapshotDiscardAllMetadata(virQEMUDriver *driver, - virDomainObj *vm) -{ - virQEMUMomentRemove rem = { - .driver = driver, - .vm = vm, - .metadata_only = true, - .momentDiscard = qemuDomainSnapshotDiscard, - }; - - virDomainSnapshotForEach(vm->snapshots, qemuDomainMomentDiscardAll, &rem); - virDomainSnapshotObjListRemoveAll(vm->snapshots); - - return rem.err; -} - static void qemuDomainRemoveInactiveCommon(virQEMUDriver *driver, @@ -7261,7 +7170,7 @@ qemuDomainRemoveInactiveCommon(virQEMUDriver *driver, g_autofree char *chkDir = NULL; /* Remove any snapshot metadata prior to removing the domain */ - if (qemuDomainSnapshotDiscardAllMetadata(driver, vm) < 0) { + if (qemuSnapshotDiscardAllMetadata(driver, vm) < 0) { VIR_WARN("unable to remove all snapshots for domain %s", vm->def->name); } else { diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 2f027fad87..e34d52c033 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -681,12 +681,6 @@ int qemuDomainSnapshotForEachQcow2(virQEMUDriver *driver, const char *op, bool try_all); -int qemuDomainSnapshotDiscard(virQEMUDriver *driver, - virDomainObj *vm, - virDomainMomentObj *snap, - bool update_current, - bool metadata_only); - typedef struct _virQEMUMomentRemove virQEMUMomentRemove; struct _virQEMUMomentRemove { virQEMUDriver *driver; @@ -703,9 +697,6 @@ int qemuDomainMomentDiscardAll(void *payload, const char *name, void *data); -int qemuDomainSnapshotDiscardAllMetadata(virQEMUDriver *driver, - virDomainObj *vm); - void qemuDomainRemoveInactive(virQEMUDriver *driver, virDomainObj *vm, virDomainUndefineFlagsValues flags, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 53533062e1..4bd052c4da 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6526,7 +6526,7 @@ qemuDomainUndefineFlags(virDomainPtr dom, nsnapshots); goto endjob; } - if (qemuDomainSnapshotDiscardAllMetadata(driver, vm) < 0) + if (qemuSnapshotDiscardAllMetadata(driver, vm) < 0) goto endjob; } if (!virDomainObjIsActive(vm) && diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 846f6e9e53..8754022aa4 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2281,6 +2281,100 @@ qemuSnapshotChildrenReparent(void *payload, } +/* Discard one snapshot (or its metadata), without reparenting any children. */ +static int +qemuSnapshotDiscard(virQEMUDriver *driver, + virDomainObj *vm, + virDomainMomentObj *snap, + bool update_parent, + bool metadata_only) +{ + g_autofree char *snapFile = NULL; + qemuDomainObjPrivate *priv; + virDomainMomentObj *parentsnap = NULL; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + + if (!metadata_only) { + if (!virDomainObjIsActive(vm)) { + size_t i; + /* Ignore any skipped disks */ + + /* Prefer action on the disks in use at the time the snapshot was + * created; but fall back to current definition if dealing with a + * snapshot created prior to libvirt 0.9.5. */ + virDomainDef *def = snap->def->dom; + + if (!def) + def = vm->def; + + for (i = 0; i < def->ndisks; i++) { + if (virDomainDiskTranslateSourcePool(def->disks[i]) < 0) + return -1; + } + + if (qemuDomainSnapshotForEachQcow2(driver, def, snap, "-d", true) < 0) + return -1; + } else { + priv = vm->privateData; + qemuDomainObjEnterMonitor(vm); + /* we continue on even in the face of error */ + qemuMonitorDeleteSnapshot(priv->mon, snap->def->name); + qemuDomainObjExitMonitor(vm); + } + } + + snapFile = g_strdup_printf("%s/%s/%s.xml", cfg->snapshotDir, vm->def->name, + snap->def->name); + + if (snap == virDomainSnapshotGetCurrent(vm->snapshots)) { + virDomainSnapshotSetCurrent(vm->snapshots, NULL); + if (update_parent && snap->def->parent_name) { + parentsnap = virDomainSnapshotFindByName(vm->snapshots, + snap->def->parent_name); + if (!parentsnap) { + VIR_WARN("missing parent snapshot matching name '%s'", + snap->def->parent_name); + } else { + virDomainSnapshotSetCurrent(vm->snapshots, parentsnap); + if (qemuDomainSnapshotWriteMetadata(vm, parentsnap, + driver->xmlopt, + cfg->snapshotDir) < 0) { + VIR_WARN("failed to set parent snapshot '%s' as current", + snap->def->parent_name); + virDomainSnapshotSetCurrent(vm->snapshots, NULL); + } + } + } + } + + if (unlink(snapFile) < 0) + VIR_WARN("Failed to unlink %s", snapFile); + if (update_parent) + virDomainMomentDropParent(snap); + virDomainSnapshotObjListRemove(vm->snapshots, snap); + + return 0; +} + + +int +qemuSnapshotDiscardAllMetadata(virQEMUDriver *driver, + virDomainObj *vm) +{ + virQEMUMomentRemove rem = { + .driver = driver, + .vm = vm, + .metadata_only = true, + .momentDiscard = qemuSnapshotDiscard, + }; + + virDomainSnapshotForEach(vm->snapshots, qemuDomainMomentDiscardAll, &rem); + virDomainSnapshotObjListRemoveAll(vm->snapshots); + + return rem.err; +} + + static int qemuSnapshotDeleteSingle(virDomainObj *vm, virDomainMomentObj *snap, @@ -2307,7 +2401,7 @@ qemuSnapshotDeleteSingle(virDomainObj *vm, virDomainMomentMoveChildren(snap, snap->parent); } - return qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); + return qemuSnapshotDiscard(driver, vm, snap, true, metadata_only); } diff --git a/src/qemu/qemu_snapshot.h b/src/qemu/qemu_snapshot.h index f69c05f68c..38437a2fd7 100644 --- a/src/qemu/qemu_snapshot.h +++ b/src/qemu/qemu_snapshot.h @@ -49,6 +49,10 @@ qemuSnapshotRevert(virDomainObj *vm, virDomainSnapshotPtr snapshot, unsigned int flags); +int +qemuSnapshotDiscardAllMetadata(virQEMUDriver *driver, + virDomainObj *vm); + int qemuSnapshotDelete(virDomainObj *vm, virDomainSnapshotPtr snapshot, -- 2.38.1

Prepare the validation function for external snapshot delete support. There is one exception when deleting `children-only` snapshots. If the snapshot tree is like this example: snap1 (external) | +- snap2 (internal) | +- snap3 (internal) | +- snap4 (internal) and user calls `snapshot-delete snap1 --children-only` the current snapshot is external but all the children snapshots are internal only and we are able to delete it. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 73 +++++++++++++++++++++++++--------------- 1 file changed, 46 insertions(+), 27 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 11427716e4..a4b45d3ba3 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -100,21 +100,6 @@ qemuSnapObjFromSnapshot(virDomainObj *vm, } -/* Count how many snapshots in a set are external snapshots. */ -static int -qemuSnapshotCountExternal(void *payload, - const char *name G_GNUC_UNUSED, - void *data) -{ - virDomainMomentObj *snap = payload; - int *count = data; - - if (virDomainSnapshotIsExternal(snap)) - (*count)++; - return 0; -} - - int qemuSnapshotFSFreeze(virDomainObj *vm, const char **mountpoints, @@ -2485,26 +2470,60 @@ qemuSnapshotDeleteChildren(virDomainObj *vm, } +typedef struct { + int external; + int internal; +} qemuSnapshotCount; + + +static int +qemuSnapshotCountExternalInternal(void *payload, + const char *name G_GNUC_UNUSED, + void *data) +{ + virDomainMomentObj *snap = payload; + qemuSnapshotCount *count = data; + + if (virDomainSnapshotIsExternal(snap)) { + count->external++; + } else { + count->internal++; + } + + return 0; +} + + static int qemuSnapshotDeleteValidate(virDomainMomentObj *snap, unsigned int flags) { - int external = 0; - - if (!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) && - virDomainSnapshotIsExternal(snap)) - external++; if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | - VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { + qemuSnapshotCount count = { 0 }; + virDomainMomentForEachDescendant(snap, - qemuSnapshotCountExternal, - &external); + qemuSnapshotCountExternalInternal, + &count); - if (external) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("deletion of %d external disk snapshots not " - "supported yet"), external); + if (count.external > 0 && count.internal > 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("deletion of external and internal children disk snapshots not supported")); + return -1; + } + + if (count.external > 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("deletion of external children disk snapshots not supported")); + return -1; + } + } + + if (virDomainSnapshotIsExternal(snap) && + !(flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("deletion of external disk snapshots not supported")); return -1; } -- 2.38.1

Deleting external snapshots will require to run it as async domain job, the same way as we do for snapshot creation. For internal snapshots modify the job mask in order to forbid any other job to be started. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index adcd4eb73a..97df448363 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2361,11 +2361,16 @@ qemuSnapshotDiscard(virQEMUDriver *driver, if (qemuDomainSnapshotForEachQcow2(driver, def, snap, "-d", true) < 0) return -1; } else { + /* Similarly as internal snapshot creation we would use a regular job + * here so set a mask to forbid any other job. */ + qemuDomainObjSetAsyncJobMask(vm, VIR_JOB_NONE); priv = vm->privateData; - qemuDomainObjEnterMonitor(vm); + if (qemuDomainObjEnterMonitorAsync(vm, VIR_ASYNC_JOB_SNAPSHOT) < 0) + return -1; /* we continue on even in the face of error */ qemuMonitorDeleteSnapshot(priv->mon, snap->def->name); qemuDomainObjExitMonitor(vm); + qemuDomainObjSetAsyncJobMask(vm, VIR_JOB_DEFAULT_MASK); } } @@ -2565,8 +2570,11 @@ qemuSnapshotDelete(virDomainObj *vm, VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY | VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY, -1); - if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0) + if (virDomainObjBeginAsyncJob(vm, VIR_ASYNC_JOB_SNAPSHOT, + VIR_DOMAIN_JOB_OPERATION_SNAPSHOT_DELETE, + flags) < 0) { return -1; + } if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) goto endjob; @@ -2584,7 +2592,7 @@ qemuSnapshotDelete(virDomainObj *vm, } endjob: - virDomainObjEndJob(vm); + virDomainObjEndAsyncJob(vm); return ret; } -- 2.38.1

In order to save some CPU cycles we will collect all the necessary data to delete external snapshot before we even start. They will be later used by code that deletes the snapshots and updates metadata when needed. With external snapshots we need data that libvirt gets from running QEMU process so if the VM is not running we need to start paused QEMU process for the snapshot deletion and kill at afterwards. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 163 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 163 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 97df448363..882224b0a7 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2232,6 +2232,134 @@ qemuSnapshotRevert(virDomainObj *vm, } +typedef struct _qemuSnapshotDeleteExternalData { + virDomainMomentObj *parentSnap; + virDomainSnapshotDiskDef *snapDisk; + virDomainDiskDef *domDisk; + virDomainDiskDef *parentDomDisk; + virStorageSource *diskSrc; + virStorageSource *parentDiskSrc; + virStorageSource *prevDiskSrc; + qemuBlockJobData *job; +} qemuSnapshotDeleteExternalData; + +G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuSnapshotDeleteExternalData, g_free); + + +/** + * qemuSnapshotFindParentSnapForDisk: + * @snap: snapshot object that is about to be deleted + * @snapDisk: snapshot disk definition + * + * Find parent snapshot object that contains snapshot of the @snapDisk. + * It may not be the next snapshot in the snapshot tree as the disk in + * question may be skipped by using VIR_DOMAIN_SNAPSHOT_LOCATION_NO. + * + * Returns virDomainMomentObj* on success or NULL if there is no parent + * snapshot containing the @snapDisk. + * */ +static virDomainMomentObj* +qemuSnapshotFindParentSnapForDisk(virDomainMomentObj *snap, + virDomainSnapshotDiskDef *snapDisk) +{ + virDomainMomentObj *parentSnap = snap->parent; + + while (parentSnap) { + ssize_t i; + virDomainSnapshotDef *parentSnapdef = virDomainSnapshotObjGetDef(parentSnap); + + if (!parentSnapdef) + break; + + for (i = 0; i < parentSnapdef->ndisks; i++) { + virDomainSnapshotDiskDef *parentSnapDisk = &(parentSnapdef->disks[i]); + + if (parentSnapDisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_NO && + STREQ(snapDisk->name, parentSnapDisk->name)) { + return parentSnap; + } + } + + parentSnap = parentSnap->parent; + } + + return NULL; +} + + +static GSList* +qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, + virDomainMomentObj *snap) +{ + ssize_t i; + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); + 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_NO) + continue; + + data = g_new0(qemuSnapshotDeleteExternalData, 1); + data->snapDisk = snapDisk; + + data->domDisk = qemuDomainDiskByName(vm->def, snapDisk->name); + if (!data->domDisk) + return NULL; + + data->diskSrc = virStorageSourceChainLookup(data->domDisk->src, NULL, + data->snapDisk->src->path, + NULL, &data->prevDiskSrc); + if (!data->diskSrc) + return NULL; + + if (!virStorageSourceIsSameLocation(data->diskSrc, data->snapDisk->src)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("VM disk source and snapshot disk source are not the same")); + return NULL; + } + + 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 NULL; + } + + if (virDomainObjIsActive(vm)) { + data->parentDiskSrc = data->diskSrc->backingStore; + if (!data->parentDiskSrc) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("failed to find parent disk source in backing chain")); + return NULL; + } + + if (!virStorageSourceIsSameLocation(data->parentDiskSrc, data->parentDomDisk->src)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("snapshot VM disk source and parent disk source are not the same")); + return NULL; + } + } + + 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 NULL; + } + + ret = g_slist_append(ret, g_steal_pointer(&data)); + } + + return g_steal_pointer(&ret); +} + + typedef struct _virQEMUMomentReparent virQEMUMomentReparent; struct _virQEMUMomentReparent { const char *dir; @@ -2565,6 +2693,10 @@ qemuSnapshotDelete(virDomainObj *vm, int ret = -1; virDomainMomentObj *snap = NULL; bool metadata_only = !!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY); + bool stop_qemu = false; + qemuDomainObjPrivate *priv = vm->privateData; + virQEMUDriver *driver = priv->driver; + g_autoslist(qemuSnapshotDeleteExternalData) externalData = NULL; virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY | @@ -2582,6 +2714,32 @@ qemuSnapshotDelete(virDomainObj *vm, if (!metadata_only) { if (qemuSnapshotDeleteValidate(vm, snap, flags) < 0) goto endjob; + + if (virDomainSnapshotIsExternal(snap) && + !(flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY))) { + + externalData = qemuSnapshotDeleteExternalPrepare(vm, snap); + + 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. */ + g_slist_free_full(externalData, g_free); + externalData = qemuSnapshotDeleteExternalPrepare(vm, snap); + } + + if (!externalData) + goto endjob; + } } if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | @@ -2592,6 +2750,11 @@ qemuSnapshotDelete(virDomainObj *vm, } endjob: + if (stop_qemu) { + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN, + VIR_ASYNC_JOB_SNAPSHOT, 0); + } + virDomainObjEndAsyncJob(vm); return ret; -- 2.38.1

When deleting snapshot we are starting block-commit job over all disks that are part of the snapshot. This operation may fail as it writes data changes to the backing qcow2 image so we need to wait for all the disks to finish the operation and wait for correct signal from QEMU. If deleting active snapshot we will get `ready` signal and for inactive snapshots we need to disable autofinalize in order to get `pending` signal. At this point if commit for any disk fails for some reason and we abort the VM is still in consistent state and user can fix the reason why the deletion failed. After that we do `pivot` or `finalize` if it's active snapshot or not to finish the block job. It still may fail but there is nothing else we can do about it. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 266 +++++++++++++++++++++++++++++++++++---- 1 file changed, 245 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 882224b0a7..c493a3e94f 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2394,6 +2394,207 @@ qemuSnapshotChildrenReparent(void *payload, } +/* Deleting external snapshot is started by running qemu block-commit job. + * We need to wait for all block-commit jobs to be 'ready' or 'pending' to + * continue with external snapshot deletion. */ +static int +qemuSnapshotJobIsRunning(qemuBlockjobState state) +{ + switch (state) { + case QEMU_BLOCKJOB_STATE_COMPLETED: + case QEMU_BLOCKJOB_STATE_FAILED: + case QEMU_BLOCKJOB_STATE_CANCELLED: + case QEMU_BLOCKJOB_STATE_READY: + case QEMU_BLOCKJOB_STATE_CONCLUDED: + case QEMU_BLOCKJOB_STATE_PENDING: + return 0; + + case QEMU_BLOCKJOB_STATE_NEW: + case QEMU_BLOCKJOB_STATE_RUNNING: + case QEMU_BLOCKJOB_STATE_ABORTING: + case QEMU_BLOCKJOB_STATE_PIVOTING: + return 1; + + case QEMU_BLOCKJOB_STATE_LAST: + break; + } + + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid block job state")); + return -1; +} + + +/* When finishing or aborting qemu blockjob we only need to know if the + * job is still active or not. */ +static int +qemuSnapshotJobIsActive(qemuBlockjobState state) +{ + switch (state) { + case QEMU_BLOCKJOB_STATE_COMPLETED: + case QEMU_BLOCKJOB_STATE_FAILED: + case QEMU_BLOCKJOB_STATE_CANCELLED: + case QEMU_BLOCKJOB_STATE_CONCLUDED: + return 0; + + case QEMU_BLOCKJOB_STATE_READY: + case QEMU_BLOCKJOB_STATE_NEW: + case QEMU_BLOCKJOB_STATE_RUNNING: + case QEMU_BLOCKJOB_STATE_ABORTING: + case QEMU_BLOCKJOB_STATE_PENDING: + case QEMU_BLOCKJOB_STATE_PIVOTING: + return 1; + + case QEMU_BLOCKJOB_STATE_LAST: + break; + } + + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid block job state")); + return -1; +} + + +/* Wait for qemu blockjob to finish 'block-commit' operation until it is + * ready to be finished by calling 'block-pivot' or 'block-finalize'. */ +static int +qemuSnapshotJobRunning(virDomainObj *vm, + qemuBlockJobData *job) +{ + int rc; + qemuBlockJobUpdate(vm, job, VIR_ASYNC_JOB_SNAPSHOT); + + while ((rc = qemuSnapshotJobIsRunning(job->state)) > 0) { + if (qemuDomainObjWait(vm) < 0) + return -1; + qemuBlockJobUpdate(vm, job, VIR_ASYNC_JOB_SNAPSHOT); + } + + if (rc < 0) + return -1; + + return 0; +} + + +/* Wait for qemu blockjob to be done after 'block-pivot' or 'block-finalize' + * was started. */ +static int +qemuSnapshotJobFinishing(virDomainObj *vm, + qemuBlockJobData *job) +{ + int rc; + qemuBlockJobUpdate(vm, job, VIR_ASYNC_JOB_SNAPSHOT); + + while ((rc = qemuSnapshotJobIsActive(job->state)) > 0) { + if (qemuDomainObjWait(vm) < 0) + return -1; + qemuBlockJobUpdate(vm, job, VIR_ASYNC_JOB_SNAPSHOT); + } + + if (rc < 0) + return -1; + + return 0; +} + + +static int +qemuSnapshotDiscardExternal(virDomainObj *vm, + GSList *externalData) +{ + GSList *cur = NULL; + + for (cur = externalData; cur; cur = g_slist_next(cur)) { + qemuSnapshotDeleteExternalData *data = cur->data; + 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; + } + + 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 (qemuSnapshotJobRunning(vm, data->job) < 0) + goto error; + + if (data->job->state == QEMU_BLOCKJOB_STATE_FAILED) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("block commit failed while deleting disk '%s' snapshot: '%s'"), + data->snapDisk->name, data->job->errmsg); + goto error; + } + } + + for (cur = externalData; cur; cur = g_slist_next(cur)) { + qemuSnapshotDeleteExternalData *data = cur->data; + + if (data->job->state == QEMU_BLOCKJOB_STATE_READY) { + if (qemuBlockPivot(vm, data->job, VIR_ASYNC_JOB_SNAPSHOT, NULL) < 0) + goto error; + } else if (data->job->state == QEMU_BLOCKJOB_STATE_PENDING) { + if (qemuBlockFinalize(vm, data->job, VIR_ASYNC_JOB_SNAPSHOT) < 0) + goto error; + } + + if (qemuSnapshotJobFinishing(vm, data->job) < 0) + goto error; + + if (data->job->state == QEMU_BLOCKJOB_STATE_FAILED) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("finishing block job failed while deleting disk '%s' snapshot: '%s'"), + data->snapDisk->name, data->job->errmsg); + goto error; + } + + qemuBlockJobSyncEnd(vm, data->job, VIR_ASYNC_JOB_SNAPSHOT); + } + + return 0; + + error: + for (cur = externalData; cur; cur = g_slist_next(cur)) { + qemuDomainObjPrivate *priv = vm->privateData; + qemuSnapshotDeleteExternalData *data = cur->data; + + if (!data->job) + continue; + + qemuBlockJobUpdate(vm, data->job, VIR_ASYNC_JOB_SNAPSHOT); + + if (qemuSnapshotJobIsActive(data->job->state)) { + if (qemuDomainObjEnterMonitorAsync(vm, VIR_ASYNC_JOB_SNAPSHOT) == 0) { + ignore_value(qemuMonitorBlockJobCancel(priv->mon, data->job->name, false)); + qemuDomainObjExitMonitor(vm); + + data->job->state = QEMU_BLOCKJOB_STATE_ABORTING; + } + } + + qemuBlockJobSyncEnd(vm, data->job, VIR_ASYNC_JOB_SNAPSHOT); + } + + return -1; +} + + static int qemuSnapshotDiscardMetadata(virDomainObj *vm, virDomainMomentObj *snap, @@ -2460,11 +2661,12 @@ qemuSnapshotDiscardMetadata(virDomainObj *vm, /* Discard one snapshot (or its metadata), without reparenting any children. */ static int -qemuSnapshotDiscard(virQEMUDriver *driver, - virDomainObj *vm, - virDomainMomentObj *snap, - bool update_parent, - bool metadata_only) +qemuSnapshotDiscardImpl(virQEMUDriver *driver, + virDomainObj *vm, + virDomainMomentObj *snap, + GSList *externalData, + bool update_parent, + bool metadata_only) { qemuDomainObjPrivate *priv; @@ -2486,19 +2688,29 @@ qemuSnapshotDiscard(virQEMUDriver *driver, return -1; } - if (qemuDomainSnapshotForEachQcow2(driver, def, snap, "-d", true) < 0) - return -1; + if (virDomainSnapshotIsExternal(snap)) { + if (qemuSnapshotDiscardExternal(vm, externalData) < 0) + return -1; + } else { + if (qemuDomainSnapshotForEachQcow2(driver, def, snap, "-d", true) < 0) + return -1; + } } else { - /* Similarly as internal snapshot creation we would use a regular job - * here so set a mask to forbid any other job. */ - qemuDomainObjSetAsyncJobMask(vm, VIR_JOB_NONE); - priv = vm->privateData; - if (qemuDomainObjEnterMonitorAsync(vm, VIR_ASYNC_JOB_SNAPSHOT) < 0) - return -1; - /* we continue on even in the face of error */ - qemuMonitorDeleteSnapshot(priv->mon, snap->def->name); - qemuDomainObjExitMonitor(vm); - qemuDomainObjSetAsyncJobMask(vm, VIR_JOB_DEFAULT_MASK); + if (virDomainSnapshotIsExternal(snap)) { + if (qemuSnapshotDiscardExternal(vm, externalData) < 0) + return -1; + } else { + /* Similarly as internal snapshot creation we would use a regular job + * here so set a mask to forbid any other job. */ + qemuDomainObjSetAsyncJobMask(vm, VIR_JOB_NONE); + priv = vm->privateData; + if (qemuDomainObjEnterMonitorAsync(vm, VIR_ASYNC_JOB_SNAPSHOT) < 0) + return -1; + /* we continue on even in the face of error */ + qemuMonitorDeleteSnapshot(priv->mon, snap->def->name); + qemuDomainObjExitMonitor(vm); + qemuDomainObjSetAsyncJobMask(vm, VIR_JOB_DEFAULT_MASK); + } } } @@ -2509,6 +2721,17 @@ qemuSnapshotDiscard(virQEMUDriver *driver, } +static int +qemuSnapshotDiscard(virQEMUDriver *driver, + virDomainObj *vm, + virDomainMomentObj *snap, + bool update_parent, + bool metadata_only) +{ + return qemuSnapshotDiscardImpl(driver, vm, snap, NULL, update_parent, metadata_only); +} + + int qemuSnapshotDiscardAllMetadata(virQEMUDriver *driver, virDomainObj *vm) @@ -2530,12 +2753,13 @@ qemuSnapshotDiscardAllMetadata(virQEMUDriver *driver, static int qemuSnapshotDeleteSingle(virDomainObj *vm, virDomainMomentObj *snap, + GSList *externalData, bool metadata_only) { qemuDomainObjPrivate *priv = vm->privateData; virQEMUDriver *driver = priv->driver; - return qemuSnapshotDiscard(driver, vm, snap, true, metadata_only); + return qemuSnapshotDiscardImpl(driver, vm, snap, externalData, true, metadata_only); } @@ -2555,7 +2779,7 @@ qemuSnapshotDeleteAll(void *payload, virDomainMomentObj *snap = payload; struct qemuSnapshotDeleteAllData *data = opaque; - error = qemuSnapshotDeleteSingle(data->vm, snap, data->metadata_only); + error = qemuSnapshotDeleteSingle(data->vm, snap, NULL, data->metadata_only); if (error != 0) data->error = error; @@ -2595,7 +2819,7 @@ qemuSnapshotDeleteChildren(virDomainObj *vm, return -1; if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN && - qemuSnapshotDeleteSingle(vm, snap, metadata_only) < 0) { + qemuSnapshotDeleteSingle(vm, snap, NULL, metadata_only) < 0) { return -1; } @@ -2746,7 +2970,7 @@ qemuSnapshotDelete(virDomainObj *vm, VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { ret = qemuSnapshotDeleteChildren(vm, snap, metadata_only, flags); } else { - ret = qemuSnapshotDeleteSingle(vm, snap, metadata_only); + ret = qemuSnapshotDeleteSingle(vm, snap, externalData, metadata_only); } endjob: -- 2.38.1

With external snapshots we need to modify the metadata bit more then what is required for internal snapshots. Mainly the storage source location changes with every external snapshot. This means that if we delete non-leaf snapshot we need to update all children snapshots and modify the disk sources for all affected disks. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 165 +++++++++++++++++++++++++++++++++++---- 1 file changed, 148 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index c493a3e94f..3fbc3ce65f 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2394,6 +2394,127 @@ qemuSnapshotChildrenReparent(void *payload, } +typedef struct _qemuSnapshotUpdateDisksData qemuSnapshotUpdateDisksData; +struct _qemuSnapshotUpdateDisksData { + virDomainMomentObj *snap; + virDomainObj *vm; + int error; +}; + + +/** + * qemuSnapshotUpdateDisksSingle: + * @snap: snapshot object where we are updating disks + * @def: active or inactive definition from @snap + * @parentDef: parent snapshot object of snapshot that we are deleting + * @snapDisk: snapshot disk definition from snapshot we are deleting + * + * When deleting external snapshots we need to modify remaining metadata + * files stored by libvirt. + * + * The first part updates only metadata for external snapshots where we need + * to update the disk source in the domain definition stored within the + * snapshot metadata. There is no need to do it for internal snapshots as + * they don't create new disk files. + * + * The second part needs to be done for all metadata files. Both internal and + * external snapshot metadata files have in the domain definition backingStore + * that could contain the deleted disk. + * + * Returns 0 on success, -1 on error. + * */ +static int +qemuSnapshotUpdateDisksSingle(virDomainMomentObj *snap, + virDomainDef *def, + virDomainDef *parentDef, + virDomainSnapshotDiskDef *snapDisk) +{ + virDomainDiskDef *disk = NULL; + + if (!(disk = qemuDomainDiskByName(def, snapDisk->name))) + return -1; + + if (virDomainSnapshotIsExternal(snap)) { + virDomainDiskDef *parentDisk = NULL; + + if (!(parentDisk = qemuDomainDiskByName(parentDef, snapDisk->name))) + return -1; + + if (virStorageSourceIsSameLocation(snapDisk->src, disk->src)) { + virObjectUnref(disk->src); + disk->src = virStorageSourceCopy(parentDisk->src, false); + } + } + + if (disk->src->backingStore) { + virStorageSource *cur = disk->src; + virStorageSource *next = disk->src->backingStore; + + while (next) { + if (virStorageSourceIsSameLocation(snapDisk->src, next)) { + cur->backingStore = next->backingStore; + next->backingStore = NULL; + virObjectUnref(next); + break; + } + + cur = next; + next = cur->backingStore; + } + } + + return 0; +} + + +static int +qemuSnapshotDeleteUpdateDisks(void *payload, + const char *name G_GNUC_UNUSED, + void *opaque) +{ + virDomainMomentObj *snap = payload; + qemuSnapshotUpdateDisksData *data = opaque; + qemuDomainObjPrivate *priv = data->vm->privateData; + virQEMUDriver *driver = priv->driver; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(data->snap); + ssize_t i; + + for (i = 0; i < snapdef->ndisks; i++) { + virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]); + + if (snapDisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NO) + continue; + + if (qemuSnapshotUpdateDisksSingle(snap, snap->def->dom, + data->snap->def->dom, snapDisk) < 0) { + data->error = -1; + } + + if (snap->def->inactiveDom) { + virDomainDef *dom = data->snap->def->inactiveDom; + + if (!dom) + dom = data->snap->def->dom; + + if (qemuSnapshotUpdateDisksSingle(snap, snap->def->inactiveDom, + dom, snapDisk) < 0) { + data->error = -1; + } + } + } + + if (qemuDomainSnapshotWriteMetadata(data->vm, + snap, + driver->xmlopt, + cfg->snapshotDir) < 0) { + data->error = -1; + } + + return 0; +} + + /* Deleting external snapshot is started by running qemu block-commit job. * We need to wait for all block-commit jobs to be 'ready' or 'pending' to * continue with external snapshot deletion. */ @@ -2604,24 +2725,34 @@ qemuSnapshotDiscardMetadata(virDomainObj *vm, virQEMUDriver *driver = priv->driver; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autofree char *snapFile = NULL; + int ret = 0; - if (update_parent) { - if (snap->nchildren) { - virQEMUMomentReparent rep; + if (update_parent && snap->nchildren) { + virQEMUMomentReparent rep; + qemuSnapshotUpdateDisksData data; - rep.dir = cfg->snapshotDir; - rep.parent = snap->parent; - rep.vm = vm; - rep.err = 0; - rep.xmlopt = driver->xmlopt; - rep.writeMetadata = qemuDomainSnapshotWriteMetadata; - virDomainMomentForEachChild(snap, - qemuSnapshotChildrenReparent, - &rep); - if (rep.err < 0) - return -1; - virDomainMomentMoveChildren(snap, snap->parent); - } + rep.dir = cfg->snapshotDir; + rep.parent = snap->parent; + rep.vm = vm; + rep.err = 0; + rep.xmlopt = driver->xmlopt; + rep.writeMetadata = qemuDomainSnapshotWriteMetadata; + virDomainMomentForEachChild(snap, + qemuSnapshotChildrenReparent, + &rep); + if (rep.err < 0) + ret = -1; + + data.snap = snap; + data.vm = vm; + data.error = 0; + virDomainMomentForEachDescendant(snap, + qemuSnapshotDeleteUpdateDisks, + &data); + if (data.error < 0) + ret = -1; + + virDomainMomentMoveChildren(snap, snap->parent); } snapFile = g_strdup_printf("%s/%s/%s.xml", cfg->snapshotDir, vm->def->name, @@ -2655,7 +2786,7 @@ qemuSnapshotDiscardMetadata(virDomainObj *vm, virDomainMomentDropParent(snap); virDomainSnapshotObjListRemove(vm->snapshots, snap); - return 0; + return ret; } -- 2.38.1

When deleting external snapshots the operation may fail at any point which could lead to situation that some disks finished the block commit operation but for some disks it failed and the libvirt job ends. In order to make sure that the qcow2 images are in consistent state introduce new element "<invalid/>" that will mark the disk in snapshot metadata as invalid until the snapshot delete is completed successfully. This will prevent deleting snapshot with the invalid disk and in future reverting to snapshot with the invalid disk. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/snapshot_conf.c | 6 ++++ src/conf/snapshot_conf.h | 1 + src/qemu/qemu_snapshot.c | 59 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 4b5b908d66..7517208d79 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -158,6 +158,9 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, return -1; } + if (flags & VIR_DOMAIN_DEF_PARSE_STATUS) + def->invalid = !!virXPathNode("./invalid", ctxt); + if ((cur = virXPathNode("./source", ctxt)) && virDomainStorageSourceParse(cur, ctxt, src, flags, xmlopt) < 0) return -1; @@ -744,6 +747,9 @@ virDomainSnapshotDiskDefFormat(virBuffer *buf, virBufferAsprintf(&attrBuf, " snapshot='%s'", virDomainSnapshotLocationTypeToString(disk->snapshot)); + if (disk->invalid) + virBufferAddLit(&childBuf, "<invalid/>\n"); + if (disk->src->path || disk->src->format != 0) { g_auto(virBuffer) driverAttrBuf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) driverChildBuf = VIR_BUFFER_INIT_CHILD(&childBuf); diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index fec4a5a912..dfab38e7d2 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -52,6 +52,7 @@ typedef struct _virDomainSnapshotDiskDef virDomainSnapshotDiskDef; struct _virDomainSnapshotDiskDef { char *name; /* name matching the <target dev='...' of the domain */ virDomainSnapshotLocation snapshot; + bool invalid; /* details of wrapper external file. src is always non-NULL. * XXX optimize this to allow NULL for internal snapshots? */ diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 3fbc3ce65f..253de1196b 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2302,6 +2302,12 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, if (snapDisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NO) continue; + if (snapDisk->invalid) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("snapshot is in invalid state")); + return NULL; + } + data = g_new0(qemuSnapshotDeleteExternalData, 1); data->snapDisk = snapDisk; @@ -2620,6 +2626,53 @@ qemuSnapshotJobFinishing(virDomainObj *vm, } +/** + * qemuSnapshotSetInvalid: + * @vm: vm object + * @snap: snapshot object that contains parent disk + * @disk: disk from the snapshot we are deleting + * @invalid: boolean to set/unset invalid state + * + * @snap should point to a ancestor snapshot from the snapshot tree that + * affected the @disk which doesn't have to be the direct parent. + * + * When setting snapshot with parent disk as invalid due to snapshot being + * deleted we should not mark the whole snapshot as invalid but only the + * affected disks because the snapshot can contain other disks that we are + * not modifying at the moment. + * + * Return 0 on success, -1 on error. + * */ +static int +qemuSnapshotSetInvalid(virDomainObj *vm, + virDomainMomentObj *snap, + virDomainSnapshotDiskDef *disk, + bool invalid) +{ + ssize_t i; + qemuDomainObjPrivate *priv = vm->privateData; + virQEMUDriver *driver = priv->driver; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + virDomainSnapshotDef *snapdef = NULL; + + if (!snap) + return 0; + + snapdef = virDomainSnapshotObjGetDef(snap); + if (!snapdef) + return 0; + + for (i = 0; i < snapdef->ndisks; i++) { + virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]); + + if (STREQ(snapDisk->name, disk->name)) + snapDisk->invalid = invalid; + } + + return qemuDomainSnapshotWriteMetadata(vm, snap, driver->xmlopt, cfg->snapshotDir); +} + + static int qemuSnapshotDiscardExternal(virDomainObj *vm, GSList *externalData) @@ -2636,6 +2689,9 @@ qemuSnapshotDiscardExternal(virDomainObj *vm, autofinalize = VIR_TRISTATE_BOOL_YES; } + if (qemuSnapshotSetInvalid(vm, data->parentSnap, data->snapDisk, true) < 0) + goto error; + data->job = qemuBlockCommit(vm, data->domDisk, data->parentDiskSrc, @@ -2686,6 +2742,9 @@ qemuSnapshotDiscardExternal(virDomainObj *vm, } qemuBlockJobSyncEnd(vm, data->job, VIR_ASYNC_JOB_SNAPSHOT); + + if (qemuSnapshotSetInvalid(vm, data->parentSnap, data->snapDisk, false) < 0) + goto error; } return 0; -- 2.38.1

If the daemon crashes or is restarted while the snapshot delete is in progress we have to handle it gracefully to not leave any block jobs active. For now we will simply abort the snapshot delete operation so user can start it again. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_process.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 5de55435d2..cc23b4a799 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3677,6 +3677,37 @@ qemuProcessRecoverMigration(virQEMUDriver *driver, } +static void +qemuProcessAbortSnapshotDelete(virDomainObj *vm) +{ + size_t i; + qemuDomainObjPrivate *priv = vm->privateData; + + for (i = 0; i < vm->def->ndisks; i++) { + virDomainDiskDef *disk = vm->def->disks[i]; + g_autoptr(qemuBlockJobData) diskJob = qemuBlockJobDiskGetJob(disk); + + if (!diskJob) + continue; + + if (diskJob->type != QEMU_BLOCKJOB_TYPE_COMMIT && + diskJob->type != QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT) { + continue; + } + + qemuBlockJobSyncBegin(diskJob); + + qemuDomainObjEnterMonitor(vm); + ignore_value(qemuMonitorBlockJobCancel(priv->mon, diskJob->name, false)); + qemuDomainObjExitMonitor(vm); + + diskJob->state = QEMU_BLOCKJOB_STATE_ABORTING; + + qemuBlockJobSyncEnd(vm, diskJob, VIR_ASYNC_JOB_NONE); + } +} + + static int qemuProcessRecoverJob(virQEMUDriver *driver, virDomainObj *vm, @@ -3726,6 +3757,7 @@ qemuProcessRecoverJob(virQEMUDriver *driver, vm->def->name); } } + qemuProcessAbortSnapshotDelete(vm); break; case VIR_ASYNC_JOB_START: -- 2.38.1

Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_snapshot.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 253de1196b..68acf54917 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3089,9 +3089,9 @@ qemuSnapshotDeleteValidate(virDomainObj *vm, } if (virDomainSnapshotIsExternal(snap) && - !(flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { + (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("deletion of external disk snapshots not supported")); + _("deletion of external disk snapshots with children not supported")); return -1; } -- 2.38.1

Now that deletion of external snapshot is implemented document the current virDomainSnapshotDelete supported state. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt-domain-snapshot.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c index 2917b8bd90..ab6a029915 100644 --- a/src/libvirt-domain-snapshot.c +++ b/src/libvirt-domain-snapshot.c @@ -1075,6 +1075,13 @@ virDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, * libvirt metadata to track snapshots, then this flag is silently * ignored. * + * Since libvirt 9.0.0 deletion of external snapshots is supported + * for QEMU driver. Using @flags VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN + * and VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY is not supported with + * external snapshots. In case that daemon process is terminated + * while the snapshot delete is in process the operation will be + * aborted when the daemon starts again. + * * Returns 0 if the selected snapshot(s) were successfully deleted, * -1 on error. * -- 2.38.1

Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- NEWS.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 39f508a6ce..fecca64344 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -17,6 +17,12 @@ v9.0.0 (unreleased) * **New features** + * QEMU: implement external snapshot deletion + + External snapshot deletion is now possible using the existing API + ``virDomainSnapshotDelete()``. Flags that allow deleting children + or children only are not supported. + * **Improvements** * qemu: Prefer PNG for domain screenshots -- 2.38.1