This will be used by snapshot delete async domain job. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- 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.38.1

Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_block.c | 179 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_block.h | 9 +++ src/qemu/qemu_driver.c | 162 +------------------------------------ 3 files changed, 189 insertions(+), 161 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 8a6f601b29..4cca7555f3 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3196,3 +3196,182 @@ qemuBlockExportAddNBD(virDomainObj *vm, return qemuMonitorBlockExportAdd(priv->mon, &nbdprops); } + + +int +qemuBlockCommit(virDomainObj *vm, + virDomainDiskDef *disk, + virStorageSource *baseSource, + virStorageSource *topSource, + virStorageSource *top_parent, + unsigned long bandwidth, + unsigned int flags) +{ + qemuDomainObjPrivate *priv = vm->privateData; + virQEMUDriver *driver = priv->driver; + int rc = -1; + bool clean_access = false; + g_autofree char *backingPath = NULL; + qemuBlockJobData *job = NULL; + g_autoptr(virStorageSource) mirror = NULL; + + if (virDomainObjCheckActive(vm) < 0) + return -1; + + /* Convert bandwidth MiB to bytes, if necessary */ + if (!(flags & VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES)) { + if (bandwidth > LLONG_MAX >> 20) { + virReportError(VIR_ERR_OVERFLOW, + _("bandwidth must be less than %llu"), + LLONG_MAX >> 20); + return -1; + } + bandwidth <<= 20; + } + + if (!qemuDomainDiskBlockJobIsSupported(disk)) + return -1; + + if (virStorageSourceIsEmpty(disk->src)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk %s has no source file to be committed"), + disk->dst); + return -1; + } + + if (qemuDomainDiskBlockJobIsActive(disk)) + return -1; + + if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0) + return -1; + + if (topSource == disk->src) { + /* XXX Should we auto-pivot when COMMIT_ACTIVE is not specified? */ + if (!(flags & VIR_DOMAIN_BLOCK_COMMIT_ACTIVE)) { + virReportError(VIR_ERR_INVALID_ARG, + _("commit of '%s' active layer requires active flag"), + disk->dst); + return -1; + } + } 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; + } + + 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; + } + + if ((flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) && + baseSource != topSource->backingStore) { + virReportError(VIR_ERR_INVALID_ARG, + _("base '%s' is not immediately below '%s' in chain " + "for '%s'"), + baseSource->path, topSource->path, disk->src->path); + return -1; + } + + /* 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; + if (virStorageSourceInitChainElement(mirror, + disk->src, + true) < 0) + return -1; + } + + if (flags & VIR_DOMAIN_BLOCK_COMMIT_RELATIVE && + topSource != disk->src) { + if (top_parent && + qemuBlockUpdateRelativeBacking(vm, top_parent, disk->src) < 0) + return -1; + + if (virStorageSourceGetRelativeBackingPath(topSource, baseSource, + &backingPath) < 0) + return -1; + + if (!backingPath) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("can't keep relative backing relationship")); + return -1; + } + } + + /* For the commit to succeed, we must allow qemu to open both the + * 'base' image and the parent of 'top' as read/write; 'top' might + * not have a parent, or might already be read-write. XXX It + * would also be nice to revert 'base' to read-only, as well as + * revoke access to files removed from the chain, when the commit + * operation succeeds, but doing that requires tracking the + * operation in XML across libvirtd restarts. */ + clean_access = true; + if (qemuDomainStorageSourceAccessAllow(driver, vm, baseSource, + false, false, false) < 0) + goto error; + + if (top_parent && top_parent != disk->src) { + /* While top_parent is topmost image, we don't need to remember its + * owner as it will be overwritten upon finishing the commit. Hence, + * pass chainTop = false. */ + if (qemuDomainStorageSourceAccessAllow(driver, vm, top_parent, + false, false, false) < 0) + goto error; + } + + if (!(job = qemuBlockJobDiskNewCommit(vm, disk, top_parent, topSource, + baseSource, + flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE, + flags))) + goto error; + + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; + + if (!backingPath && top_parent && + !(backingPath = qemuBlockGetBackingStoreString(baseSource, false))) + goto error; + + qemuDomainObjEnterMonitor(vm); + + rc = qemuMonitorBlockCommit(priv->mon, + qemuDomainDiskGetTopNodename(disk), + job->name, + topSource->nodeformat, + baseSource->nodeformat, + backingPath, bandwidth); + + qemuDomainObjExitMonitor(vm); + + if (rc < 0) + goto error; + + if (mirror) { + disk->mirror = g_steal_pointer(&mirror); + disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT; + } + qemuBlockJobStarted(job, vm); + + return 0; + + error: + if (clean_access) { + virErrorPtr orig_err; + virErrorPreserveLast(&orig_err); + /* Revert access to read-only, if possible. */ + qemuDomainStorageSourceAccessAllow(driver, vm, baseSource, + true, false, false); + if (top_parent && top_parent != disk->src) + qemuDomainStorageSourceAccessAllow(driver, vm, top_parent, + true, false, false); + + virErrorRestore(&orig_err); + } + qemuBlockJobStartupFinalize(vm, job); + + return -1; +} diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 8a3a10344e..85b4805d89 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -276,3 +276,12 @@ qemuBlockExportAddNBD(virDomainObj *vm, const char *exportname, bool writable, const char *bitmap); + +int +qemuBlockCommit(virDomainObj *vm, + virDomainDiskDef *disk, + virStorageSource *baseSource, + virStorageSource *topSource, + virStorageSource *top_parent, + unsigned long bandwidth, + unsigned int flags); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d509582719..2f05da3d8c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15114,19 +15114,12 @@ qemuDomainBlockCommit(virDomainPtr dom, unsigned long bandwidth, unsigned int flags) { - virQEMUDriver *driver = dom->conn->privateData; - qemuDomainObjPrivate *priv; virDomainObj *vm = NULL; int ret = -1; virDomainDiskDef *disk = NULL; virStorageSource *topSource; virStorageSource *baseSource = NULL; virStorageSource *top_parent = NULL; - bool clean_access = false; - g_autofree char *backingPath = NULL; - unsigned long long speed = bandwidth; - qemuBlockJobData *job = NULL; - g_autoptr(virStorageSource) mirror = NULL; virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW | VIR_DOMAIN_BLOCK_COMMIT_ACTIVE | @@ -15136,7 +15129,6 @@ qemuDomainBlockCommit(virDomainPtr dom, if (!(vm = qemuDomainObjFromDomain(dom))) goto cleanup; - priv = vm->privateData; if (virDomainBlockCommitEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -15144,176 +15136,24 @@ qemuDomainBlockCommit(virDomainPtr dom, if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0) goto cleanup; - if (virDomainObjCheckActive(vm) < 0) - goto endjob; - - /* Convert bandwidth MiB to bytes, if necessary */ - if (!(flags & VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES)) { - if (speed > LLONG_MAX >> 20) { - virReportError(VIR_ERR_OVERFLOW, - _("bandwidth must be less than %llu"), - LLONG_MAX >> 20); - goto endjob; - } - speed <<= 20; - } - if (!(disk = qemuDomainDiskByName(vm->def, path))) goto endjob; - if (!qemuDomainDiskBlockJobIsSupported(disk)) - goto endjob; - - if (virStorageSourceIsEmpty(disk->src)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("disk %s has no source file to be committed"), - disk->dst); - goto endjob; - } - - if (qemuDomainDiskBlockJobIsActive(disk)) - goto endjob; - - if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0) - goto endjob; - if (!top || STREQ(top, disk->dst)) topSource = disk->src; else if (!(topSource = virStorageSourceChainLookup(disk->src, NULL, top, disk->dst, &top_parent))) goto endjob; - if (topSource == disk->src) { - /* XXX Should we auto-pivot when COMMIT_ACTIVE is not specified? */ - if (!(flags & VIR_DOMAIN_BLOCK_COMMIT_ACTIVE)) { - virReportError(VIR_ERR_INVALID_ARG, - _("commit of '%s' active layer requires active flag"), - disk->dst); - goto endjob; - } - } else if (flags & VIR_DOMAIN_BLOCK_COMMIT_ACTIVE) { - virReportError(VIR_ERR_INVALID_ARG, - _("active commit requested but '%s' is not active"), - topSource->path); - goto endjob; - } - - if (!virStorageSourceHasBacking(topSource)) { - virReportError(VIR_ERR_INVALID_ARG, - _("top '%s' in chain for '%s' has no backing file"), - topSource->path, path); - goto endjob; - } - if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) baseSource = topSource->backingStore; else if (!(baseSource = virStorageSourceChainLookup(disk->src, topSource, base, disk->dst, NULL))) goto endjob; - if ((flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW) && - baseSource != topSource->backingStore) { - virReportError(VIR_ERR_INVALID_ARG, - _("base '%s' is not immediately below '%s' in chain " - "for '%s'"), - base, topSource->path, path); - goto endjob; - } - - /* For an active commit, clone enough of the base to act as the mirror */ - if (topSource == disk->src) { - if (!(mirror = virStorageSourceCopy(baseSource, false))) - goto endjob; - if (virStorageSourceInitChainElement(mirror, - disk->src, - true) < 0) - goto endjob; - } - - if (flags & VIR_DOMAIN_BLOCK_COMMIT_RELATIVE && - topSource != disk->src) { - if (top_parent && - qemuBlockUpdateRelativeBacking(vm, top_parent, disk->src) < 0) - goto endjob; - - if (virStorageSourceGetRelativeBackingPath(topSource, baseSource, - &backingPath) < 0) - goto endjob; - - if (!backingPath) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("can't keep relative backing relationship")); - goto endjob; - } - } - - /* For the commit to succeed, we must allow qemu to open both the - * 'base' image and the parent of 'top' as read/write; 'top' might - * not have a parent, or might already be read-write. XXX It - * would also be nice to revert 'base' to read-only, as well as - * revoke access to files removed from the chain, when the commit - * operation succeeds, but doing that requires tracking the - * operation in XML across libvirtd restarts. */ - clean_access = true; - if (qemuDomainStorageSourceAccessAllow(driver, vm, baseSource, - false, false, false) < 0) - goto endjob; - - if (top_parent && top_parent != disk->src) { - /* While top_parent is topmost image, we don't need to remember its - * owner as it will be overwritten upon finishing the commit. Hence, - * pass chainTop = false. */ - if (qemuDomainStorageSourceAccessAllow(driver, vm, top_parent, - false, false, false) < 0) - goto endjob; - } - - if (!(job = qemuBlockJobDiskNewCommit(vm, disk, top_parent, topSource, - baseSource, - flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE, - flags))) - goto endjob; - - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; - - if (!backingPath && top_parent && - !(backingPath = qemuBlockGetBackingStoreString(baseSource, false))) - goto endjob; - - qemuDomainObjEnterMonitor(vm); - - ret = qemuMonitorBlockCommit(priv->mon, - qemuDomainDiskGetTopNodename(disk), - job->name, - topSource->nodeformat, - baseSource->nodeformat, - backingPath, speed); - - qemuDomainObjExitMonitor(vm); - - if (ret < 0) - goto endjob; - - if (mirror) { - disk->mirror = g_steal_pointer(&mirror); - disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT; - } - qemuBlockJobStarted(job, vm); + ret = qemuBlockCommit(vm, disk, baseSource, topSource, top_parent, bandwidth, flags); endjob: - if (ret < 0 && clean_access) { - virErrorPtr orig_err; - virErrorPreserveLast(&orig_err); - /* Revert access to read-only, if possible. */ - qemuDomainStorageSourceAccessAllow(driver, vm, baseSource, - true, false, false); - if (top_parent && top_parent != disk->src) - qemuDomainStorageSourceAccessAllow(driver, vm, top_parent, - true, false, false); - - virErrorRestore(&orig_err); - } - qemuBlockJobStartupFinalize(vm, job); virDomainObjEndJob(vm); cleanup: -- 2.38.1

On Thu, Dec 08, 2022 at 14:30:38 +0100, Pavel Hrdina wrote: [...] [1] [2] [...] [...] So before this patch qemuBlockJobStartupFinalize() gets called both on error and success code paths. After this patch [1] the success code path doesn't call it any more, just the error code path [2]. Thus the reference to 'job' is leaked in the new code. The new function must also call qemuBlockJobStartupFinalize() in both cases.

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(a)redhat.com&gt; --- src/qemu/qemu_block.c | 7 ++++++- src/qemu/qemu_block.h | 1 + src/qemu/qemu_driver.c | 3 ++- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 721145fa42..999e43c630 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3205,6 +3205,7 @@ qemuBlockCommit(virDomainObj *vm, virStorageSource *topSource, virStorageSource *top_parent, unsigned long bandwidth, + virDomainAsyncJob asyncJob, unsigned int flags) { qemuDomainObjPrivate *priv = vm->privateData; @@ -3336,7 +3337,11 @@ qemuBlockCommit(virDomainObj *vm, !(backingPath = qemuBlockGetBackingStoreString(baseSource, false))) goto error; - qemuDomainObjEnterMonitor(vm); + if (asyncJob != VIR_ASYNC_JOB_NONE) + qemuBlockJobSyncBegin(job); + + if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0) + goto error; rc = 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.38.1

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(a)redhat.com&gt; --- 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 999e43c630..8a633d4423 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3388,6 +3388,7 @@ qemuBlockCommit(virDomainObj *vm, int qemuBlockPivot(virDomainObj *vm, qemuBlockJobData *job, + virDomainAsyncJob asyncJob, virDomainDiskDef *disk) { g_autoptr(qemuBlockStorageSourceChainData) chainattachdata = NULL; @@ -3473,7 +3474,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 5ee6e2698d..acd67b1f5a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14163,7 +14163,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.38.1

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(a)redhat.com&gt; --- 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.38.1

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(a)redhat.com&gt; --- 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 8a633d4423..5600abf6aa 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3348,7 +3348,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 0354832fa5..43525def0c 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 eebac918c1..d09439911f 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 baf01a7206..9aee84084e 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 073a0579b5..a03caf7740 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.38.1

External snapshots will use this to synchronize qemu block jobs. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_block.c | 7 ++++++- src/qemu/qemu_block.h | 1 + src/qemu/qemu_driver.c | 3 ++- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 5600abf6aa..2abddf904f 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3198,6 +3198,10 @@ qemuBlockExportAddNBD(virDomainObj *vm, } +/* 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.*/ int qemuBlockCommit(virDomainObj *vm, virDomainDiskDef *disk, @@ -3206,6 +3210,7 @@ qemuBlockCommit(virDomainObj *vm, virStorageSource *top_parent, unsigned long bandwidth, virDomainAsyncJob asyncJob, + virTristateBool autofinalize, unsigned int flags) { qemuDomainObjPrivate *priv = vm->privateData; @@ -3349,7 +3354,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.38.1

Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_block.c | 19 +++++++++++++++++++ src/qemu/qemu_block.h | 5 +++++ 2 files changed, 24 insertions(+) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 2abddf904f..24c82377b0 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3509,3 +3509,22 @@ qemuBlockPivot(virDomainObj *vm, return ret; } + + +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.38.1

The created job will be needed by external snapshot delete code so rework qemuBlockCommit to return that pointer. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_block.c | 42 +++++++++++++++++++++++------------------- src/qemu/qemu_block.h | 2 +- src/qemu/qemu_driver.c | 5 ++++- 3 files changed, 28 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 24c82377b0..0a0d942e71 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3201,8 +3201,12 @@ qemuBlockExportAddNBD(virDomainObj *vm, /* 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.*/ -int + * to this argument should be VIR_TRISTATE_BOOL_YES. + * + * Returns qemuBlockJobData pointer on success, NULL on error. Caller is responsible + * to call virObjectUnref on the pointer. + */ +qemuBlockJobData * qemuBlockCommit(virDomainObj *vm, virDomainDiskDef *disk, virStorageSource *baseSource, @@ -3222,7 +3226,7 @@ qemuBlockCommit(virDomainObj *vm, 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)) { @@ -3230,26 +3234,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? */ @@ -3257,20 +3261,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) && @@ -3279,33 +3283,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; } } @@ -3367,7 +3371,7 @@ qemuBlockCommit(virDomainObj *vm, } qemuBlockJobStarted(job, vm); - return 0; + return job; error: if (clean_access) { @@ -3384,7 +3388,7 @@ qemuBlockCommit(virDomainObj *vm, } qemuBlockJobStartupFinalize(vm, job); - return -1; + return NULL; } 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.38.1

On Tue, Dec 13, 2022 at 10:06:41 +0100, Peter Krempa wrote: Once you fix patch 2/30 to call qemuBlockJobStartupFinalize in both cases, this patch will also need to call it directly to finalize the job once the pointer is handed over. And I mean explicitly calling qemuBlockJobStartupFinalize even in the success code path even when for now it just unrefs the object. With that change: Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Tue, Dec 13, 2022 at 18:12:38 +0100, Pavel Hrdina wrote: Thinking about it a bit more I think it's okay to use virObjectUnref without an explicit call to qemuBlockJobStartupFinalize() in cases where you are 100% sure that the job was started. Thus in the patch makes qemuBlockCommit() return the job pointer you will no longer call qemuBlockJobStartupFinalize() and the caller will have to unref the returned job. The same thing will need to happen here then. Originally I wanted to insist that all of the job cleanup goes through qemuBlockJobStartupFinalize() but that actually doesn't make much sense.

QEMU emits this signal when autofinalize is disabled and QEMU is waiting for the caller to start the finalization manually. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_backup.c | 1 + src/qemu/qemu_blockjob.c | 20 +++++++++++++++++++- src/qemu/qemu_blockjob.h | 1 + 3 files changed, 21 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..71dddc256c 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,18 @@ 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 when aborting job it can + * reset the internally set job state */ + 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 +1699,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.38.1

Move code around to make it clear what is called when deleting single snapshot or children snapshots. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 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.38.1

On Thu, Dec 08, 2022 at 14:30:50 +0100, Pavel Hrdina wrote: 'boolean' is quite obvious from the function prototype, but it's not that obvious what it actually means. @metadata_only: If true, delete only snapshot metadata, leave data intact. In fact the only thing you use from @flags is VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY. Additionally you already pass 'metadata_only' as boolean which is originally also part of 'flags. Preferrably extract VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY as a boolean too and pass it to this function instead of flags. s/proved/provided/ ? With the above: Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Thu, Dec 08, 2022 at 14:30:51 +0100, Pavel Hrdina wrote: Since this itself doesn't delete all snapshots please call it e.g: qemuSnapshotDeleteAllHelper Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

Previously the reparent happened before the actual snapshot deletion. This change moves the code closer to the rest of the code handling snapshot metadata when deletion happens. Following patch will extract it into separate function Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 8754022aa4..5418496c1e 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2323,6 +2323,25 @@ qemuSnapshotDiscard(virQEMUDriver *driver, } } + if (update_parent) { + if (snap->nchildren) { + virQEMUMomentReparent rep; + + rep.dir = cfg->snapshotDir; + rep.parent = snap->parent; + rep.vm = vm; + rep.err = 0; + rep.xmlopt = driver->xmlopt; + rep.writeMetadata = qemuDomainSnapshotWriteMetadata; + virDomainMomentForEachChild(snap, + qemuSnapshotChildrenReparent, + &rep); + if (rep.err < 0) + return -1; + virDomainMomentMoveChildren(snap, snap->parent); + } + } + snapFile = g_strdup_printf("%s/%s/%s.xml", cfg->snapshotDir, vm->def->name, snap->def->name); @@ -2382,24 +2401,6 @@ qemuSnapshotDeleteSingle(virDomainObj *vm, { qemuDomainObjPrivate *priv = vm->privateData; virQEMUDriver *driver = priv->driver; - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - - if (snap->nchildren) { - virQEMUMomentReparent rep; - - rep.dir = cfg->snapshotDir; - rep.parent = snap->parent; - rep.vm = vm; - rep.err = 0; - rep.xmlopt = driver->xmlopt; - rep.writeMetadata = qemuDomainSnapshotWriteMetadata; - virDomainMomentForEachChild(snap, - qemuSnapshotChildrenReparent, - &rep); - if (rep.err < 0) - return -1; - virDomainMomentMoveChildren(snap, snap->parent); - } return qemuSnapshotDiscard(driver, vm, snap, true, metadata_only); } -- 2.38.1

Extract the code deleting external snapshot metadata to separate function. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 91 +++++++++++++++++++++++----------------- 1 file changed, 53 insertions(+), 38 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 5418496c1e..4ea3f578e9 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,52 @@ 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) +{ + qemuDomainObjPrivate *priv; + + 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 (qemuSnapshotDiscardMetadata(vm, snap, update_parent) < 0) + return -1; + + return 0; +} + + int qemuSnapshotDiscardAllMetadata(virQEMUDriver *driver, virDomainObj *vm) -- 2.38.1

Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 42 ++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 4ea3f578e9..11427716e4 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2485,6 +2485,33 @@ qemuSnapshotDeleteChildren(virDomainObj *vm, } +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)) + virDomainMomentForEachDescendant(snap, + qemuSnapshotCountExternal, + &external); + + if (external) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("deletion of %d external disk snapshots not " + "supported yet"), external); + return -1; + } + + return 0; +} + + int qemuSnapshotDelete(virDomainObj *vm, virDomainSnapshotPtr snapshot, @@ -2493,7 +2520,6 @@ qemuSnapshotDelete(virDomainObj *vm, int ret = -1; virDomainMomentObj *snap = NULL; bool metadata_only = !!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY); - int external = 0; virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY | @@ -2506,20 +2532,8 @@ qemuSnapshotDelete(virDomainObj *vm, goto endjob; if (!metadata_only) { - if (!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) && - virDomainSnapshotIsExternal(snap)) - external++; - if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | - VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) - virDomainMomentForEachDescendant(snap, - qemuSnapshotCountExternal, - &external); - if (external) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("deletion of %d external disk snapshots not " - "supported yet"), external); + if (qemuSnapshotDeleteValidate(snap, flags) < 0) goto endjob; - } } if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | -- 2.38.1

Deleting internal snapshot when the currently active disk image is different then where the internal snapshot was taken doesn't work correctly. This applies to a running VM only as we are using QMP command and talking to the QEMU process that is using different disk. This works correctly when the VM is shut of as in this case we spawn qemu-img process to delete the snapshot. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index a4b45d3ba3..adcd4eb73a 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2495,9 +2495,30 @@ qemuSnapshotCountExternalInternal(void *payload, static int -qemuSnapshotDeleteValidate(virDomainMomentObj *snap, +qemuSnapshotDeleteValidate(virDomainObj *vm, + virDomainMomentObj *snap, unsigned int flags) { + if (!virDomainSnapshotIsExternal(snap) && + virDomainObjIsActive(vm)) { + ssize_t i; + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); + + for (i = 0; i < snapdef->ndisks; i++) { + virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]); + virDomainDiskDef *vmdisk = NULL; + virDomainDiskDef *disk = NULL; + + vmdisk = qemuDomainDiskByName(vm->def, snapDisk->name); + disk = qemuDomainDiskByName(snapdef->parent.dom, snapDisk->name); + + if (!virStorageSourceIsSameLocation(vmdisk->src, disk->src)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("deleting internal snapshot for non-active disk is not supported")); + return -1; + } + } + } if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { @@ -2551,7 +2572,7 @@ qemuSnapshotDelete(virDomainObj *vm, goto endjob; if (!metadata_only) { - if (qemuSnapshotDeleteValidate(snap, flags) < 0) + if (qemuSnapshotDeleteValidate(vm, snap, flags) < 0) goto endjob; } -- 2.38.1

On Tue, Dec 13, 2022 at 03:26:52PM +0100, Peter Krempa wrote: I get the point but to me it feels it still doesn't describe the issue properly. Take this situation, you have VM, first snapshot is internal and second one is external and active and the VM is running. In this situation you are not able to delete the internal snapshot because the HMP commands will not work properly but will not produce any error as well. VM | +- snap1 (internal) | + snap2 (external)(active) So the triggers only if the snapshot that we are trying to delete is internal, the VM is running and the currently active disk in VM is different than where the internal snapshot was taken. How about: "disk image '%s' for internal snapshot '%s' is not the same as disk image currently used by VM" Pavel

On Thu, Dec 08, 2022 at 14:30:59 +0100, Pavel Hrdina wrote: Please annotate which 'source' belongs to which 'disk def' and such. You can also theoretically reorder them. So here you are looking up based on 'src->path' which works properly really only for file-based/local storage. For anything else it will break. Since you have a virStorageSource to compare to you should probably create a helper which walks the backing chain and uses virStorageSourceIsSameLocation internally instead virStorageSourceChainLookup which should really be used only for user-provided data as the third argument (for backwards compatibility with old-style API args). Note that active definitions contain also a "terminator" virStorageSource put into the 'backingStore' object, which is a valid pointer. You probably want to use 'virStorageSourceIsBacking' here. The usual way to work with singly-linked lists is to use the prepend operation ... ... and reverse it before returning. Preferrably don't reuse 'externalData'. You can e.g. instead steal the value into a locally scoped temporary autofreed variable and then re-create it. If you deal with the lookup properly you can add: Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Thu, Dec 08, 2022 at 14:31:00 +0100, Pavel Hrdina wrote: This is more considering snapshot deletion blockjobs... so perhaps: qemuSnapshotDeleteBlockjobIsRunning ? Can this happen? Same naming patern as above. -||- And both functions above have confusing naming too. [1] So 'externalData' is passed here from the caller, and the caller simply calls the equivalent of 'g_free' on the individual struct. Now this leaks the 'job' field allocated in [1] since you are still holding the reference. And in this code path too.

On Thu, Dec 08, 2022 at 14:31:02 +0100, Pavel Hrdina wrote: You want VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE as this parser doesn't work with VIR_DOMAIN_DEF_PARSE_* flags. In fact the only reason this works is that VIR_DOMAIN_DEF_PARSE_STATUS and VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE have the same value. Based on my coment below ... the 'invalid' name of this flag might be a bit misleading. I suppose we want to say that there's another operation in progress, but I don't have currently ideas for a better name. [...] This error is not really actionable. You want to explain that it's most likely a previous operation that failed and needs to be re-tried. I think you also want to mark the inactiveDef if present.

On Thu, Dec 08, 2022 at 14:31:03 +0100, Pavel Hrdina wrote: So assume that you have a VM where you've e.g. added a new disk after it had some snapshots. Now you have a running blockjob on the new disk. Now you try delete a snapshot. The un-related job could get restarted as this job doesn't distinguish between those situations. This is as a regular block-job doesn't even register as an async job. The simplest fix will be to call qemuDomainHasBlockjob and refuse the whole deletion. Once you add that to the appropriate place: Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;

On Thu, Dec 08, 2022 at 14:31:05 +0100, Pavel Hrdina wrote: This doesn't look like the best place for this kind of documentation but I don't have a better suggestion. Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;