This will be used by snapshot delete async domain job. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- include/libvirt/libvirt-domain.h | 1 + tools/virsh-domain.c | 1 + 2 files changed, 2 insertions(+) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 295fd30c93..4b1de1d5b8 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -4172,6 +4172,7 @@ typedef enum { VIR_DOMAIN_JOB_OPERATION_SNAPSHOT_REVERT = 7, /* (Since: 3.3.0) */ VIR_DOMAIN_JOB_OPERATION_DUMP = 8, /* (Since: 3.3.0) */ VIR_DOMAIN_JOB_OPERATION_BACKUP = 9, /* (Since: 6.0.0) */ + VIR_DOMAIN_JOB_OPERATION_SNAPSHOT_DELETE = 10, /* (Since: 9.0.0) */ # ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_JOB_OPERATION_LAST /* (Since: 3.3.0) */ diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 2d162cf8c0..3e94744c95 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6141,6 +6141,7 @@ VIR_ENUM_IMPL(virshDomainJobOperation, N_("Snapshot revert"), N_("Dump"), N_("Backup"), + N_("Snapshot delete"), ); static const char * -- 2.39.0

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 825db3e088..6d31269ddd 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3373,3 +3373,124 @@ qemuBlockCommit(virDomainObj *vm, return ret; } + + +/* 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 331e6ca50b..c35e5e1a31 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13996,127 +13996,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 @@ -14279,7 +14158,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.39.0

This will allow to use it while having async domain job active which we will use when deleting external snapshots. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 4 +++- src/qemu/qemu_block.h | 1 + src/qemu/qemu_driver.c | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index d2a56ba19c..6a68362708 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3402,6 +3402,7 @@ qemuBlockCommit(virDomainObj *vm, int qemuBlockPivot(virDomainObj *vm, qemuBlockJobData *job, + virDomainAsyncJob asyncJob, virDomainDiskDef *disk) { g_autoptr(qemuBlockStorageSourceChainData) chainattachdata = NULL; @@ -3487,7 +3488,8 @@ qemuBlockPivot(virDomainObj *vm, break; } - qemuDomainObjEnterMonitor(vm); + if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0) + return -1; if (chainattachdata) { if ((rc = qemuBlockStorageSourceChainAttach(priv->mon, chainattachdata)) == 0) { diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index a76d9be711..3f2082f102 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -290,4 +290,5 @@ qemuBlockCommit(virDomainObj *vm, int qemuBlockPivot(virDomainObj *vm, qemuBlockJobData *job, + virDomainAsyncJob asyncJob, virDomainDiskDef *disk); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f7a7086b3d..970cc29eaf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14158,7 +14158,7 @@ qemuDomainBlockJobAbort(virDomainPtr dom, qemuBlockJobSyncBegin(job); if (pivot) { - if ((ret = qemuBlockPivot(vm, job, disk)) < 0) + if ((ret = qemuBlockPivot(vm, job, VIR_ASYNC_JOB_NONE, disk)) < 0) goto endjob; } else { qemuDomainObjEnterMonitor(vm); -- 2.39.0

Deleting external snapshots will require configuring autofinalize to synchronize the block jobs for disks withing single snapshot in order to be able safely abort of one of the jobs fails. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 3 ++- src/qemu/qemu_monitor.c | 9 +++++---- src/qemu/qemu_monitor.h | 3 ++- src/qemu/qemu_monitor_json.c | 4 ++-- src/qemu/qemu_monitor_json.h | 3 ++- tests/qemumonitorjsontest.c | 2 +- 6 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 6a68362708..99409a969b 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3364,7 +3364,8 @@ qemuBlockCommit(virDomainObj *vm, job->name, topSource->nodeformat, baseSource->nodeformat, - backingPath, bandwidth); + backingPath, bandwidth, + VIR_TRISTATE_BOOL_YES); qemuDomainObjExitMonitor(vm); diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 771b1b6d59..38f89167e0 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2795,16 +2795,17 @@ qemuMonitorBlockCommit(qemuMonitor *mon, const char *topNode, const char *baseNode, const char *backingName, - unsigned long long bandwidth) + unsigned long long bandwidth, + virTristateBool autofinalize) { - VIR_DEBUG("device=%s, jobname=%s, topNode=%s, baseNode=%s, backingName=%s, bandwidth=%llu", + VIR_DEBUG("device=%s, jobname=%s, topNode=%s, baseNode=%s, backingName=%s, bandwidth=%llu, autofinalize=%d", device, NULLSTR(jobname), NULLSTR(topNode), - NULLSTR(baseNode), NULLSTR(backingName), bandwidth); + NULLSTR(baseNode), NULLSTR(backingName), bandwidth, autofinalize); QEMU_CHECK_MONITOR(mon); return qemuMonitorJSONBlockCommit(mon, device, jobname, topNode, baseNode, - backingName, bandwidth); + backingName, bandwidth, autofinalize); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index f51146a13a..2d16214ba2 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -973,7 +973,8 @@ int qemuMonitorBlockCommit(qemuMonitor *mon, const char *topNode, const char *baseNode, const char *backingName, - unsigned long long bandwidth) + unsigned long long bandwidth, + virTristateBool autofinalize) ATTRIBUTE_NONNULL(2); int qemuMonitorArbitraryCommand(qemuMonitor *mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 4495f8f2cf..db99017555 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4028,11 +4028,11 @@ qemuMonitorJSONBlockCommit(qemuMonitor *mon, const char *topNode, const char *baseNode, const char *backingName, - unsigned long long speed) + unsigned long long speed, + virTristateBool autofinalize) { g_autoptr(virJSONValue) cmd = NULL; g_autoptr(virJSONValue) reply = NULL; - virTristateBool autofinalize = VIR_TRISTATE_BOOL_YES; virTristateBool autodismiss = VIR_TRISTATE_BOOL_NO; cmd = qemuMonitorJSONMakeCommand("block-commit", diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index d774cbaf14..6f376cf9b7 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -315,7 +315,8 @@ qemuMonitorJSONBlockCommit(qemuMonitor *mon, const char *topNode, const char *baseNode, const char *backingName, - unsigned long long bandwidth) + unsigned long long bandwidth, + virTristateBool autofinalize) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index f224ccb364..1db1f2b949 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1211,7 +1211,7 @@ GEN_TEST_FUNC(qemuMonitorJSONRemoveNetdev, "net0") GEN_TEST_FUNC(qemuMonitorJSONDelDevice, "ide0") GEN_TEST_FUNC(qemuMonitorJSONBlockdevMirror, "jobname", true, "vdb", "targetnode", 1024, 1234, 31234, true, true) GEN_TEST_FUNC(qemuMonitorJSONBlockStream, "vdb", "jobname", "backingnode", "backingfilename", 1024) -GEN_TEST_FUNC(qemuMonitorJSONBlockCommit, "vdb", "jobname", "topnode", "basenode", "backingfilename", 1024) +GEN_TEST_FUNC(qemuMonitorJSONBlockCommit, "vdb", "jobname", "topnode", "basenode", "backingfilename", 1024, VIR_TRISTATE_BOOL_YES) GEN_TEST_FUNC(qemuMonitorJSONScreendump, "devicename", 1, NULL, "/foo/bar") GEN_TEST_FUNC(qemuMonitorJSONOpenGraphics, "spice", "spicefd", false) GEN_TEST_FUNC(qemuMonitorJSONNBDServerAdd, "vda", "export", true, "bitmap") -- 2.39.0

Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 32 ++++++++++++++++++++++++++++++++ src/qemu/qemu_block.h | 5 +++++ 2 files changed, 37 insertions(+) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index da3a1e8557..70811aa861 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3525,3 +3525,35 @@ qemuBlockPivot(virDomainObj *vm, return ret; } + + +/** + * qemuBlockFinalize: + * @vm: domain object + * @job: qemu block job data object + * @asyncJob: qemu async job type + * + * When qemu job is started with autofinalize disabled it will wait in pending + * state for block job finalize to be called manually in order to finish the + * job. This is useful when we are running jobs on multiple disks to make + * a synchronization point before we finish. + * + * Return -1 on error, 0 on success. + */ +int +qemuBlockFinalize(virDomainObj *vm, + qemuBlockJobData *job, + virDomainAsyncJob asyncJob) +{ + int ret; + qemuDomainObjPrivate *priv = vm->privateData; + + if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0) + return -1; + + ret = qemuMonitorJobFinalize(priv->mon, job->name); + + qemuDomainObjExitMonitor(vm); + + return ret; +} diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 7b6aec2a7d..c169432d9c 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -293,3 +293,8 @@ qemuBlockPivot(virDomainObj *vm, qemuBlockJobData *job, virDomainAsyncJob asyncJob, virDomainDiskDef *disk); + +int +qemuBlockFinalize(virDomainObj *vm, + qemuBlockJobData *job, + virDomainAsyncJob asyncJob); -- 2.39.0

QEMU emits this signal when the job finished its work and is about to be finalized. If the job is started with autofinalize disabled the job waits for user input to finalize the job. This will be used by snapshot delete code. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_backup.c | 1 + src/qemu/qemu_blockjob.c | 21 ++++++++++++++++++++- src/qemu/qemu_blockjob.h | 1 + 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index c7721812a5..6a5aacd9f6 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -1035,6 +1035,7 @@ qemuBackupNotifyBlockjobEnd(virDomainObj *vm, 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: case QEMU_BLOCKJOB_STATE_LAST: default: diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index a7aa7b3940..cb2d05d71d 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -55,6 +55,7 @@ VIR_ENUM_IMPL(qemuBlockjobState, "running", "concluded", "aborting", + "pending", "pivoting"); VIR_ENUM_IMPL(qemuBlockjob, @@ -531,6 +532,8 @@ qemuBlockJobRefreshJobs(virDomainObj *vm) if (job->state == QEMU_BLOCKJOB_STATE_NEW || job->state == QEMU_BLOCKJOB_STATE_RUNNING) job->newstate = newstate; + } else if (newstate == QEMU_BLOCKJOB_STATE_PENDING) { + job->newstate = newstate; } /* don't update the job otherwise */ } @@ -1563,6 +1566,19 @@ qemuBlockJobEventProcess(virQEMUDriver *driver, job->newstate = -1; break; + case QEMU_BLOCKJOB_STATE_PENDING: + /* Similarly as for 'ready' state we should handle it only when + * previous state was 'new' or 'running' as there are other cases + * when it can be emitted by QEMU. Currently we need this only when + * deleting non-active external snapshots. */ + if (job->state == QEMU_BLOCKJOB_STATE_NEW || + job->state == QEMU_BLOCKJOB_STATE_RUNNING) { + job->state = job->newstate; + qemuDomainSaveStatus(vm); + } + job->newstate = -1; + break; + case QEMU_BLOCKJOB_STATE_NEW: case QEMU_BLOCKJOB_STATE_RUNNING: case QEMU_BLOCKJOB_STATE_LAST: @@ -1684,13 +1700,16 @@ qemuBlockjobConvertMonitorStatus(int monitorstatus) ret = QEMU_BLOCKJOB_STATE_CONCLUDED; break; + case QEMU_MONITOR_JOB_STATUS_PENDING: + ret = QEMU_BLOCKJOB_STATE_PENDING; + break; + case QEMU_MONITOR_JOB_STATUS_UNKNOWN: case QEMU_MONITOR_JOB_STATUS_CREATED: case QEMU_MONITOR_JOB_STATUS_RUNNING: case QEMU_MONITOR_JOB_STATUS_PAUSED: case QEMU_MONITOR_JOB_STATUS_STANDBY: case QEMU_MONITOR_JOB_STATUS_WAITING: - case QEMU_MONITOR_JOB_STATUS_PENDING: case QEMU_MONITOR_JOB_STATUS_ABORTING: case QEMU_MONITOR_JOB_STATUS_UNDEFINED: case QEMU_MONITOR_JOB_STATUS_NULL: diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h index 741d8df6c5..e9b283da20 100644 --- a/src/qemu/qemu_blockjob.h +++ b/src/qemu/qemu_blockjob.h @@ -41,6 +41,7 @@ typedef enum { QEMU_BLOCKJOB_STATE_CONCLUDED, /* job has finished, but it's unknown whether it has failed or not */ QEMU_BLOCKJOB_STATE_ABORTING, + QEMU_BLOCKJOB_STATE_PENDING, QEMU_BLOCKJOB_STATE_PIVOTING, QEMU_BLOCKJOB_STATE_LAST } qemuBlockjobState; -- 2.39.0

Move code around to make it clear what is called when deleting single snapshot or children snapshots. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 37 ++++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 8b5bcd9770..4cc74fb092 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2350,25 +2350,28 @@ qemuSnapshotDelete(virDomainObj *vm, } } } - } 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); - } - if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { - virDomainMomentDropChildren(snap); - ret = 0; + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { + virDomainMomentDropChildren(snap); + ret = 0; + } else { + 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); } -- 2.39.0

Extract code that deletes children of specific snapshot to separate function. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 109 ++++++++++++++++++++++++--------------- 1 file changed, 68 insertions(+), 41 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index e85655924a..e65eef8a0d 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: if true only snapshots metadata are deleted + * @children_only: if true only snapshot children are deleted + * + * Delete children snapshots of snapshot provided by @snap. If @metadata_only + * is true only snapshot metadata files are delete, disk data are left intact. + * If @children_only is true it will delete only children snapshots of @snap + * and leave @snap intact. + * + * Returns 0 on success, -1 on error. + */ +static int +qemuSnapshotDeleteChildren(virDomainObj *vm, + virDomainMomentObj *snap, + bool metadata_only, + bool children_only) { - 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 (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 (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,8 @@ 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); - } + bool children_only = !!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY); + ret = qemuSnapshotDeleteChildren(vm, snap, metadata_only, children_only); } else { ret = qemuSnapshotDeleteSingle(vm, snap, metadata_only); } -- 2.39.0

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 5c05032ce3..73dc13e178 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" @@ -7140,81 +7141,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, @@ -7233,23 +7159,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, @@ -7262,7 +7171,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 a2ebd8093a..3ea48c9049 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6520,7 +6520,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 37a52028a5..3eb0ec2998 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.39.0

Extract the code deleting external snapshot metadata to separate function. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 88 +++++++++++++++++++++++----------------- 1 file changed, 50 insertions(+), 38 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index e7893f285a..359f0a3671 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2281,47 +2281,15 @@ 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) +qemuSnapshotDiscardMetadata(virDomainObj *vm, + virDomainMomentObj *snap, + bool update_parent) { + qemuDomainObjPrivate *priv = vm->privateData; + virQEMUDriver *driver = priv->driver; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); 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); - } - } if (update_parent) { if (snap->nchildren) { @@ -2348,6 +2316,7 @@ qemuSnapshotDiscard(virQEMUDriver *driver, if (snap == virDomainSnapshotGetCurrent(vm->snapshots)) { virDomainSnapshotSetCurrent(vm->snapshots, NULL); if (update_parent && snap->def->parent_name) { + virDomainMomentObj *parentsnap = NULL; parentsnap = virDomainSnapshotFindByName(vm->snapshots, snap->def->parent_name); if (!parentsnap) { @@ -2376,6 +2345,49 @@ qemuSnapshotDiscard(virQEMUDriver *driver, } +/* 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) +{ + 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 { + qemuDomainObjEnterMonitor(vm); + /* we continue on even in the face of error */ + qemuMonitorDeleteSnapshot(qemuDomainGetMonitor(vm), snap->def->name); + qemuDomainObjExitMonitor(vm); + } + } + + if (qemuSnapshotDiscardMetadata(vm, snap, update_parent) < 0) + return -1; + + return 0; +} + + int qemuSnapshotDiscardAllMetadata(virQEMUDriver *driver, virDomainObj *vm) -- 2.39.0

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 2e0e4896f5..c23f629e9f 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, @@ -2482,26 +2467,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.39.0

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> Reviewed-by: Peter Krempa <pkrempa@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 30137a2b8f..3fd348efd3 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2359,10 +2359,15 @@ qemuSnapshotDiscard(virQEMUDriver *driver, if (qemuDomainSnapshotForEachQcow2(driver, def, snap, "-d", true) < 0) return -1; } else { - qemuDomainObjEnterMonitor(vm); + /* 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); + if (qemuDomainObjEnterMonitorAsync(vm, VIR_ASYNC_JOB_SNAPSHOT) < 0) + return -1; /* we continue on even in the face of error */ qemuMonitorDeleteSnapshot(qemuDomainGetMonitor(vm), snap->def->name); qemuDomainObjExitMonitor(vm); + qemuDomainObjSetAsyncJobMask(vm, VIR_JOB_DEFAULT_MASK); } } @@ -2563,8 +2568,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; @@ -2583,7 +2591,7 @@ qemuSnapshotDelete(virDomainObj *vm, } endjob: - virDomainObjEndJob(vm); + virDomainObjEndAsyncJob(vm); return ret; } -- 2.39.0

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> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_snapshot.c | 256 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 236 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index c7136efe93..26f962ce82 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2410,6 +2410,199 @@ 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 +qemuSnapshotDeleteBlockJobIsRunning(qemuBlockjobState state) +{ + switch (state) { + 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_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: + case QEMU_BLOCKJOB_STATE_LAST: + break; + } + + return 0; +} + + +/* When finishing or aborting qemu blockjob we only need to know if the + * job is still active or not. */ +static int +qemuSnapshotDeleteBlockJobIsActive(qemuBlockjobState state) +{ + switch (state) { + 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_COMPLETED: + case QEMU_BLOCKJOB_STATE_FAILED: + case QEMU_BLOCKJOB_STATE_CANCELLED: + case QEMU_BLOCKJOB_STATE_CONCLUDED: + case QEMU_BLOCKJOB_STATE_LAST: + break; + } + + return 0; +} + + +/* 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 +qemuSnapshotDeleteBlockJobRunning(virDomainObj *vm, + qemuBlockJobData *job) +{ + int rc; + qemuBlockJobUpdate(vm, job, VIR_ASYNC_JOB_SNAPSHOT); + + while ((rc = qemuSnapshotDeleteBlockJobIsRunning(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 +qemuSnapshotDeleteBlockJobFinishing(virDomainObj *vm, + qemuBlockJobData *job) +{ + int rc; + qemuBlockJobUpdate(vm, job, VIR_ASYNC_JOB_SNAPSHOT); + + while ((rc = qemuSnapshotDeleteBlockJobIsActive(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 (qemuSnapshotDeleteBlockJobRunning(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 (qemuSnapshotDeleteBlockJobFinishing(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 (qemuSnapshotDeleteBlockJobIsActive(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, @@ -2476,11 +2669,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) { if (!metadata_only) { if (!virDomainObjIsActive(vm)) { @@ -2500,18 +2694,28 @@ 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); - if (qemuDomainObjEnterMonitorAsync(vm, VIR_ASYNC_JOB_SNAPSHOT) < 0) - return -1; - /* we continue on even in the face of error */ - qemuMonitorDeleteSnapshot(qemuDomainGetMonitor(vm), 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); + if (qemuDomainObjEnterMonitorAsync(vm, VIR_ASYNC_JOB_SNAPSHOT) < 0) + return -1; + /* we continue on even in the face of error */ + qemuMonitorDeleteSnapshot(qemuDomainGetMonitor(vm), snap->def->name); + qemuDomainObjExitMonitor(vm); + qemuDomainObjSetAsyncJobMask(vm, VIR_JOB_DEFAULT_MASK); + } } } @@ -2522,6 +2726,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) @@ -2543,12 +2758,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); } @@ -2568,7 +2784,7 @@ qemuSnapshotDeleteAllHelper(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; @@ -2608,7 +2824,7 @@ qemuSnapshotDeleteChildren(virDomainObj *vm, return -1; if (!children_only && - qemuSnapshotDeleteSingle(vm, snap, metadata_only) < 0) { + qemuSnapshotDeleteSingle(vm, snap, NULL, metadata_only) < 0) { return -1; } @@ -2763,7 +2979,7 @@ qemuSnapshotDelete(virDomainObj *vm, bool children_only = !!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY); ret = qemuSnapshotDeleteChildren(vm, snap, metadata_only, children_only); } else { - ret = qemuSnapshotDeleteSingle(vm, snap, metadata_only); + ret = qemuSnapshotDeleteSingle(vm, snap, externalData, metadata_only); } endjob: -- 2.39.0

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 "<snapshotDeleteInProgress/>" 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 | 8 ++++++ src/conf/snapshot_conf.h | 1 + src/qemu/qemu_snapshot.c | 60 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 4b5b908d66..9bf3c78353 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -158,6 +158,11 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node, return -1; } + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { + def->snapshotDeleteInProgress = !!virXPathNode("./snapshotDeleteInProgress", + ctxt); + } + if ((cur = virXPathNode("./source", ctxt)) && virDomainStorageSourceParse(cur, ctxt, src, flags, xmlopt) < 0) return -1; @@ -744,6 +749,9 @@ virDomainSnapshotDiskDefFormat(virBuffer *buf, virBufferAsprintf(&attrBuf, " snapshot='%s'", virDomainSnapshotLocationTypeToString(disk->snapshot)); + if (disk->snapshotDeleteInProgress) + virBufferAddLit(&childBuf, "<snapshotDeleteInProgress/>\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..96c77ef42b 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 snapshotDeleteInProgress; /* 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 06cc6ef8c0..189fe98299 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2316,6 +2316,13 @@ qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, if (snapDisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NO) continue; + if (snapDisk->snapshotDeleteInProgress) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("snapshot disk '%s' was target of not completed snapshot delete"), + snapDisk->name); + return NULL; + } + data = g_new0(qemuSnapshotDeleteExternalData, 1); data->snapDisk = snapDisk; @@ -2628,6 +2635,53 @@ qemuSnapshotDeleteBlockJobFinishing(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->snapshotDeleteInProgress = invalid; + } + + return qemuDomainSnapshotWriteMetadata(vm, snap, driver->xmlopt, cfg->snapshotDir); +} + + static int qemuSnapshotDiscardExternal(virDomainObj *vm, GSList *externalData) @@ -2644,6 +2698,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, @@ -2694,6 +2751,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.39.0

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. We need to refuse deleting external snapshots if there is already another active job as we would have to figure out which jobs we can abort. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_process.c | 37 +++++++++++++++++++++++++++++++++++++ src/qemu/qemu_snapshot.c | 15 +++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 49ae7b688b..cc65e6befa 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3692,6 +3692,42 @@ qemuProcessRecoverMigration(virQEMUDriver *driver, } +static void +qemuProcessAbortSnapshotDelete(virDomainObj *vm, + virDomainJobObj *job) +{ + size_t i; + qemuDomainObjPrivate *priv = vm->privateData; + qemuDomainJobPrivate *jobPriv = job->privateData; + + if (!jobPriv->snapshotDelete) + return; + + 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, @@ -3741,6 +3777,7 @@ qemuProcessRecoverJob(virQEMUDriver *driver, vm->def->name); } } + qemuProcessAbortSnapshotDelete(vm, job); break; case VIR_ASYNC_JOB_START: diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 189fe98299..348d3260c8 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3095,6 +3095,13 @@ qemuSnapshotDeleteValidate(virDomainObj *vm, } } + if (virDomainSnapshotIsExternal(snap) && + qemuDomainHasBlockjob(vm, false)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("cannot delete external snapshots when there is another active block job")); + return -1; + } + if (virDomainSnapshotIsExternal(snap) && !(flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -3158,6 +3165,14 @@ qemuSnapshotDelete(virDomainObj *vm, * running to get everything we need. */ delData = g_steal_pointer(&externalData); externalData = qemuSnapshotDeleteExternalPrepare(vm, snap); + } else { + qemuDomainJobPrivate *jobPriv = vm->job->privateData; + + /* If the VM is running we need to indicate that the async snapshot + * job is snapshot delete job. */ + jobPriv->snapshotDelete = true; + + qemuDomainSaveStatus(vm); } if (!externalData) -- 2.39.0

Now that deletion of external snapshot is implemented document the current virDomainSnapshotDelete supported state. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@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.39.0