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

This will allow to use it while having async domain job active which we will use when deleting external snapshots. At the same time we will need to have the block job started as synchronous. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 23 ++++++++++++++++++++++- src/qemu/qemu_block.h | 1 + src/qemu/qemu_driver.c | 3 ++- 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 6d31269ddd..d2a56ba19c 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3198,6 +3198,22 @@ qemuBlockExportAddNBD(virDomainObj *vm, } +/** + * qemuBlockCommit: + * @vm: domain object + * @disk: disk object where we are about to block commit + * @baseSource: disk source within backing chain to commit data into + * @topSource: disk source within backing chain with data we will commit + * @top_parent: disk source that has @topSource as backing disk + * @bandwidth: bandwidth limit, flags determine the unit + * @asyncJob: qemu async job type + * @flags: bitwise-OR of virDomainBlockCommitFlags + * + * Starts a block commit job for @disk. If @asyncJob is different then + * VIR_ASYNC_JOB_NONE the job will be started as synchronous. + * + * Returns -1 on error, 0 on success. + */ int qemuBlockCommit(virDomainObj *vm, virDomainDiskDef *disk, @@ -3205,6 +3221,7 @@ qemuBlockCommit(virDomainObj *vm, virStorageSource *topSource, virStorageSource *top_parent, unsigned long bandwidth, + virDomainAsyncJob asyncJob, unsigned int flags) { qemuDomainObjPrivate *priv = vm->privateData; @@ -3336,7 +3353,11 @@ qemuBlockCommit(virDomainObj *vm, !(backingPath = qemuBlockGetBackingStoreString(baseSource, false))) goto cleanup; - qemuDomainObjEnterMonitor(vm); + if (asyncJob != VIR_ASYNC_JOB_NONE) + qemuBlockJobSyncBegin(job); + + if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0) + goto cleanup; ret = qemuMonitorBlockCommit(priv->mon, qemuDomainDiskGetTopNodename(disk), diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 52deb15a3d..a76d9be711 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -284,6 +284,7 @@ qemuBlockCommit(virDomainObj *vm, virStorageSource *topSource, virStorageSource *top_parent, unsigned long bandwidth, + virDomainAsyncJob asyncJob, unsigned int flags); int diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d8db7188f7..5ee6e2698d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15030,7 +15030,8 @@ qemuDomainBlockCommit(virDomainPtr dom, base, disk->dst, NULL))) goto endjob; - ret = qemuBlockCommit(vm, disk, baseSource, topSource, top_parent, bandwidth, flags); + ret = qemuBlockCommit(vm, disk, baseSource, topSource, top_parent, + bandwidth, VIR_ASYNC_JOB_NONE, flags); endjob: virDomainObjEndJob(vm); -- 2.39.0

Upcoming snapshot deletion code will require that multiple commit jobs are finished in sync. To allow aborting then if one fails we will need to use manual finalization of the jobs. This commit implements the monitor code for `job-finalize`. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor.c | 12 ++++++++++++ src/qemu/qemu_monitor.h | 5 +++++ src/qemu/qemu_monitor_json.c | 22 ++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 +++++ tests/qemumonitorjsontest.c | 2 ++ 5 files changed, 46 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 734364e070..0354832fa5 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2932,6 +2932,18 @@ qemuMonitorJobDismiss(qemuMonitor *mon, } +int +qemuMonitorJobFinalize(qemuMonitor *mon, + const char *jobname) +{ + VIR_DEBUG("jobname=%s", jobname); + + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONJobFinalize(mon, jobname); +} + + int qemuMonitorJobComplete(qemuMonitor *mon, const char *jobname) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 906a919f52..eebac918c1 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1029,6 +1029,11 @@ int qemuMonitorJobDismiss(qemuMonitor *mon, const char *jobname) ATTRIBUTE_NONNULL(2); +int +qemuMonitorJobFinalize(qemuMonitor *mon, + const char *jobname) + ATTRIBUTE_NONNULL(2); + int qemuMonitorJobComplete(qemuMonitor *mon, const char *jobname) ATTRIBUTE_NONNULL(2); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 9822097bd7..baf01a7206 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4405,6 +4405,28 @@ qemuMonitorJSONJobDismiss(qemuMonitor *mon, } +int +qemuMonitorJSONJobFinalize(qemuMonitor *mon, + const char *jobname) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("job-finalize", + "s:id", jobname, + NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + return -1; + + if (qemuMonitorJSONBlockJobError(cmd, reply, jobname) < 0) + return -1; + + return 0; +} + + int qemuMonitorJSONJobComplete(qemuMonitor *mon, const char *jobname) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 484cb09830..073a0579b5 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -377,6 +377,11 @@ qemuMonitorJSONJobDismiss(qemuMonitor *mon, const char *jobname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int +qemuMonitorJSONJobFinalize(qemuMonitor *mon, + const char *jobname) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + int qemuMonitorJSONJobComplete(qemuMonitor *mon, const char *jobname) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 59f7322711..f224ccb364 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1223,6 +1223,7 @@ GEN_TEST_FUNC(qemuMonitorJSONBlockdevMediumInsert, "foodev", "newnode") GEN_TEST_FUNC(qemuMonitorJSONBitmapRemove, "foodev", "newnode") GEN_TEST_FUNC(qemuMonitorJSONJobDismiss, "jobname") GEN_TEST_FUNC(qemuMonitorJSONJobComplete, "jobname") +GEN_TEST_FUNC(qemuMonitorJSONJobFinalize, "jobname") GEN_TEST_FUNC(qemuMonitorJSONBlockJobCancel, "jobname", true) GEN_TEST_FUNC(qemuMonitorJSONSetAction, QEMU_MONITOR_ACTION_SHUTDOWN_PAUSE, @@ -2921,6 +2922,7 @@ mymain(void) DO_TEST_GEN(qemuMonitorJSONBitmapRemove); DO_TEST_GEN(qemuMonitorJSONJobDismiss); DO_TEST_GEN(qemuMonitorJSONJobComplete); + DO_TEST_GEN(qemuMonitorJSONJobFinalize); DO_TEST_GEN(qemuMonitorJSONBlockJobCancel); DO_TEST_GEN(qemuMonitorJSONSetAction); DO_TEST_GEN(qemuMonitorJSONSetLaunchSecurityState); -- 2.39.0

External snapshots will use this to synchronize qemu block jobs. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 9 ++++++++- src/qemu/qemu_block.h | 1 + src/qemu/qemu_driver.c | 3 ++- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 99409a969b..da3a1e8557 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3207,11 +3207,17 @@ qemuBlockExportAddNBD(virDomainObj *vm, * @top_parent: disk source that has @topSource as backing disk * @bandwidth: bandwidth limit, flags determine the unit * @asyncJob: qemu async job type + * @autofinalize: virTristateBool controlling qemu block job finalization * @flags: bitwise-OR of virDomainBlockCommitFlags * * Starts a block commit job for @disk. If @asyncJob is different then * VIR_ASYNC_JOB_NONE the job will be started as synchronous. * + * The @autofinalize argument controls if the qemu block job will be automatically + * finalized. This is used when deleting external snapshots where we need to + * disable automatic finalization for some use-case. The default value passed + * to this argument should be VIR_TRISTATE_BOOL_YES. + * * Returns -1 on error, 0 on success. */ int @@ -3222,6 +3228,7 @@ qemuBlockCommit(virDomainObj *vm, virStorageSource *top_parent, unsigned long bandwidth, virDomainAsyncJob asyncJob, + virTristateBool autofinalize, unsigned int flags) { qemuDomainObjPrivate *priv = vm->privateData; @@ -3365,7 +3372,7 @@ qemuBlockCommit(virDomainObj *vm, topSource->nodeformat, baseSource->nodeformat, backingPath, bandwidth, - VIR_TRISTATE_BOOL_YES); + autofinalize); qemuDomainObjExitMonitor(vm); diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 3f2082f102..7b6aec2a7d 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -285,6 +285,7 @@ qemuBlockCommit(virDomainObj *vm, virStorageSource *top_parent, unsigned long bandwidth, virDomainAsyncJob asyncJob, + virTristateBool autofinalize, unsigned int flags); int diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index acd67b1f5a..12fca22616 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15031,7 +15031,8 @@ qemuDomainBlockCommit(virDomainPtr dom, goto endjob; ret = qemuBlockCommit(vm, disk, baseSource, topSource, top_parent, - bandwidth, VIR_ASYNC_JOB_NONE, flags); + bandwidth, VIR_ASYNC_JOB_NONE, VIR_TRISTATE_BOOL_YES, + flags); endjob: virDomainObjEndJob(vm); -- 2.39.0

The created job will be needed by external snapshot delete code so rework qemuBlockCommit to return that pointer. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_block.c | 57 ++++++++++++++++++++++-------------------- src/qemu/qemu_block.h | 2 +- src/qemu/qemu_driver.c | 5 +++- 3 files changed, 35 insertions(+), 29 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 70811aa861..7ea42961b6 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3218,9 +3218,10 @@ qemuBlockExportAddNBD(virDomainObj *vm, * disable automatic finalization for some use-case. The default value passed * to this argument should be VIR_TRISTATE_BOOL_YES. * - * Returns -1 on error, 0 on success. + * Returns qemuBlockJobData pointer on success, NULL on error. Caller is responsible + * to call virObjectUnref on the pointer. */ -int +qemuBlockJobData * qemuBlockCommit(virDomainObj *vm, virDomainDiskDef *disk, virStorageSource *baseSource, @@ -3233,14 +3234,15 @@ qemuBlockCommit(virDomainObj *vm, { qemuDomainObjPrivate *priv = vm->privateData; virQEMUDriver *driver = priv->driver; - int ret = -1; + int rc = -1; bool clean_access = false; g_autofree char *backingPath = NULL; qemuBlockJobData *job = NULL; + qemuBlockJobData *ret = NULL; g_autoptr(virStorageSource) mirror = NULL; if (virDomainObjCheckActive(vm) < 0) - return -1; + return NULL; /* Convert bandwidth MiB to bytes, if necessary */ if (!(flags & VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES)) { @@ -3248,26 +3250,26 @@ qemuBlockCommit(virDomainObj *vm, virReportError(VIR_ERR_OVERFLOW, _("bandwidth must be less than %llu"), LLONG_MAX >> 20); - return -1; + return NULL; } bandwidth <<= 20; } if (!qemuDomainDiskBlockJobIsSupported(disk)) - return -1; + return NULL; if (virStorageSourceIsEmpty(disk->src)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk %s has no source file to be committed"), disk->dst); - return -1; + return NULL; } if (qemuDomainDiskBlockJobIsActive(disk)) - return -1; + return NULL; if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0) - return -1; + return NULL; if (topSource == disk->src) { /* XXX Should we auto-pivot when COMMIT_ACTIVE is not specified? */ @@ -3275,20 +3277,20 @@ qemuBlockCommit(virDomainObj *vm, virReportError(VIR_ERR_INVALID_ARG, _("commit of '%s' active layer requires active flag"), disk->dst); - return -1; + return NULL; } } else if (flags & VIR_DOMAIN_BLOCK_COMMIT_ACTIVE) { virReportError(VIR_ERR_INVALID_ARG, _("active commit requested but '%s' is not active"), topSource->path); - return -1; + return NULL; } if (!virStorageSourceHasBacking(topSource)) { virReportError(VIR_ERR_INVALID_ARG, _("top '%s' in chain for '%s' has no backing file"), topSource->path, disk->src->path); - return -1; + return NULL; } if ((flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) && @@ -3297,33 +3299,33 @@ qemuBlockCommit(virDomainObj *vm, _("base '%s' is not immediately below '%s' in chain " "for '%s'"), baseSource->path, topSource->path, disk->src->path); - return -1; + return NULL; } /* For an active commit, clone enough of the base to act as the mirror */ if (topSource == disk->src) { if (!(mirror = virStorageSourceCopy(baseSource, false))) - return -1; + return NULL; if (virStorageSourceInitChainElement(mirror, disk->src, true) < 0) - return -1; + return NULL; } if (flags & VIR_DOMAIN_BLOCK_COMMIT_RELATIVE && topSource != disk->src) { if (top_parent && qemuBlockUpdateRelativeBacking(vm, top_parent, disk->src) < 0) - return -1; + return NULL; if (virStorageSourceGetRelativeBackingPath(topSource, baseSource, &backingPath) < 0) - return -1; + return NULL; if (!backingPath) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("can't keep relative backing relationship")); - return -1; + return NULL; } } @@ -3366,17 +3368,17 @@ qemuBlockCommit(virDomainObj *vm, if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0) goto cleanup; - ret = qemuMonitorBlockCommit(priv->mon, - qemuDomainDiskGetTopNodename(disk), - job->name, - topSource->nodeformat, - baseSource->nodeformat, - backingPath, bandwidth, - autofinalize); + rc = qemuMonitorBlockCommit(priv->mon, + qemuDomainDiskGetTopNodename(disk), + job->name, + topSource->nodeformat, + baseSource->nodeformat, + backingPath, bandwidth, + autofinalize); qemuDomainObjExitMonitor(vm); - if (ret < 0) + if (rc < 0) goto cleanup; if (mirror) { @@ -3384,9 +3386,10 @@ qemuBlockCommit(virDomainObj *vm, disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT; } qemuBlockJobStarted(job, vm); + ret = virObjectRef(job); cleanup: - if (ret < 0 && clean_access) { + if (rc < 0 && clean_access) { virErrorPtr orig_err; virErrorPreserveLast(&orig_err); /* Revert access to read-only, if possible. */ diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index c169432d9c..a8079c2207 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -277,7 +277,7 @@ qemuBlockExportAddNBD(virDomainObj *vm, bool writable, const char *bitmap); -int +qemuBlockJobData * qemuBlockCommit(virDomainObj *vm, virDomainDiskDef *disk, virStorageSource *baseSource, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 12fca22616..53533062e1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14999,6 +14999,7 @@ qemuDomainBlockCommit(virDomainPtr dom, virStorageSource *topSource; virStorageSource *baseSource = NULL; virStorageSource *top_parent = NULL; + g_autoptr(qemuBlockJobData) job = NULL; virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW | VIR_DOMAIN_BLOCK_COMMIT_ACTIVE | @@ -15030,9 +15031,11 @@ qemuDomainBlockCommit(virDomainPtr dom, base, disk->dst, NULL))) goto endjob; - ret = qemuBlockCommit(vm, disk, baseSource, topSource, top_parent, + job = qemuBlockCommit(vm, disk, baseSource, topSource, top_parent, bandwidth, VIR_ASYNC_JOB_NONE, VIR_TRISTATE_BOOL_YES, flags); + if (job) + ret = 0; endjob: virDomainObjEndJob(vm); -- 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> --- 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

Looks up disk storage source within storage source chain using storage source object instead of path to make it work with all disk types. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt_private.syms | 1 + src/storage_file/storage_source.c | 39 +++++++++++++++++++++++++++++++ src/storage_file/storage_source.h | 6 +++++ 3 files changed, 46 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ae746a2d51..6ec405ef67 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1778,6 +1778,7 @@ virStorageFileProbeGetMetadata; # storage_file/storage_source.h virStorageSourceAccess; virStorageSourceChainLookup; +virStorageSourceChainLookupBySource; virStorageSourceChown; virStorageSourceCreate; virStorageSourceDeinit; diff --git a/src/storage_file/storage_source.c b/src/storage_file/storage_source.c index ab0cdf2b12..a15d766bec 100644 --- a/src/storage_file/storage_source.c +++ b/src/storage_file/storage_source.c @@ -322,6 +322,45 @@ virStorageSourceChainLookup(virStorageSource *chain, } +/** + * virStorageSourceChainLookupBySource: + * @chain: chain top to look in + * @base: stourage source to look for in @chain + * @parent: Filled with parent virStorageSource of the returned value if non-NULL. + * + * Looks up a storage source definition corresponding to @base in @chain. + * + * Returns virStorageSource withing chain or NULL if not found. + */ +virStorageSource * +virStorageSourceChainLookupBySource(virStorageSource *chain, + virStorageSource *base, + virStorageSource **parent) +{ + virStorageSource *prev = NULL; + + if (parent) + *parent = NULL; + + while (virStorageSourceIsBacking(chain)) { + if (virStorageSourceIsSameLocation(chain, base)) + break; + + prev = chain; + chain = chain->backingStore; + } + + if (!virStorageSourceIsBacking(chain)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("could not find base disk source in disk source chain")); + return NULL; + } + + *parent = prev; + return chain; +} + + static virStorageSource * virStorageSourceNewFromBackingRelative(virStorageSource *parent, const char *rel) diff --git a/src/storage_file/storage_source.h b/src/storage_file/storage_source.h index 0ae06f4e7d..63fefb6919 100644 --- a/src/storage_file/storage_source.h +++ b/src/storage_file/storage_source.h @@ -47,6 +47,12 @@ virStorageSourceChainLookup(virStorageSource *chain, virStorageSource **parent) ATTRIBUTE_NONNULL(1); +virStorageSource * +virStorageSourceChainLookupBySource(virStorageSource *chain, + virStorageSource *base, + virStorageSource **parent) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + int virStorageSourceUpdatePhysicalSize(virStorageSource *src, int fd, -- 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 d7983c134f..e9bb5abee2 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 47239e9e9c..a3ff195685 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 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 ce25d17c99..307a3deeb1 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 471bd184d4..3051e5c8df 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 cb4e0dcb78..d7da317e22 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 31e7087081..fe279a2374 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> --- 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 fe050b7fc4..6ca61301a7 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

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> Reviewed-by: Peter Krempa <pkrempa@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 6ca61301a7..094d56b13d 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2410,6 +2410,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. */ @@ -2612,24 +2733,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, @@ -2663,7 +2794,7 @@ qemuSnapshotDiscardMetadata(virDomainObj *vm, virDomainMomentDropParent(snap); virDomainSnapshotObjListRemove(vm->snapshots, snap); - return 0; + return ret; } -- 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 | 32 ++++++++++++++++++++++++++++++++ src/qemu/qemu_snapshot.c | 7 +++++++ 2 files changed, 39 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index b6adcf2f2a..09ab231aaa 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: diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 0ed8a82aa9..f5b88daa5d 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", -- 2.39.0

Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@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 f5b88daa5d..4f83eb6bf9 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -3103,9 +3103,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.39.0

Signed-off-by: Pavel Hrdina <phrdina@redhat.com> Reviewed-by: Peter Krempa <pkrempa@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.39.0