Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_block.c | 197 +++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_block.h | 9 ++ src/qemu/qemu_driver.c | 181 +------------------------------------ 3 files changed, 207 insertions(+), 180 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index b82e3311e1..c0c4088cbf 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3201,3 +3201,200 @@ qemuBlockExportAddNBD(virDomainObj *vm, return qemuMonitorBlockExportAdd(priv->mon, &nbdprops); } + + +int +qemuBlockCommit(virDomainObj *vm, + virQEMUDriver *driver, + const char *path, + const char *base, + const char *top, + unsigned long bandwidth, + unsigned int flags) +{ + qemuDomainObjPrivate *priv = vm->privateData; + int rc = -1; + virDomainDiskDef *disk = NULL; + virStorageSource *topSource; + virStorageSource *baseSource = NULL; + virStorageSource *top_parent = NULL; + 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 (!(disk = qemuDomainDiskByName(vm->def, path))) + return -1; + + 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 (!top || STREQ(top, disk->dst)) + topSource = disk->src; + else if (!(topSource = virStorageSourceChainLookup(disk->src, NULL, top, + disk->dst, &top_parent))) + 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, path); + return -1; + } + + if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) + baseSource = topSource->backingStore; + else if (!(baseSource = virStorageSourceChainLookup(disk->src, topSource, + base, disk->dst, NULL))) + 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'"), + base, topSource->path, 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..be81ec3c7f 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, + virQEMUDriver *driver, + const char *path, + const char *base, + const char *top, + unsigned long bandwidth, + unsigned int flags); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 707f4cc1bb..d353b6df93 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15375,18 +15375,8 @@ qemuDomainBlockCommit(virDomainPtr dom, 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 | @@ -15396,7 +15386,6 @@ qemuDomainBlockCommit(virDomainPtr dom, if (!(vm = qemuDomainObjFromDomain(dom))) goto cleanup; - priv = vm->privateData; if (virDomainBlockCommitEnsureACL(dom->conn, vm->def) < 0) goto cleanup; @@ -15404,176 +15393,8 @@ qemuDomainBlockCommit(virDomainPtr dom, if (qemuDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0) goto cleanup; - if (virDomainObjCheckActive(vm) < 0) - goto endjob; + ret = qemuBlockCommit(vm, driver, path, base, top, bandwidth, flags); - /* 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); - - 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); qemuDomainObjEndJob(vm); cleanup: -- 2.37.2

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

Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_block.c | 78 ++++++++++++++++++++++++++----------------- src/qemu/qemu_block.h | 10 ++++++ 2 files changed, 57 insertions(+), 31 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 5b34461853..902ec7b2a5 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3204,28 +3204,22 @@ qemuBlockExportAddNBD(virDomainObj *vm, int -qemuBlockCommit(virDomainObj *vm, - virQEMUDriver *driver, - const char *path, - const char *base, - const char *top, - unsigned long bandwidth, - unsigned int flags) +qemuBlockCommitImpl(virDomainObj *vm, + virQEMUDriver *driver, + virDomainDiskDef *disk, + virStorageSource *baseSource, + virStorageSource *topSource, + virStorageSource *top_parent, + unsigned long bandwidth, + unsigned int flags) { qemuDomainObjPrivate *priv = vm->privateData; int rc = -1; - virDomainDiskDef *disk = NULL; - virStorageSource *topSource; - virStorageSource *baseSource = NULL; - virStorageSource *top_parent = NULL; 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) { @@ -3237,9 +3231,6 @@ qemuBlockCommit(virDomainObj *vm, bandwidth <<= 20; } - if (!(disk = qemuDomainDiskByName(vm->def, path))) - return -1; - if (!qemuDomainDiskBlockJobIsSupported(disk)) return -1; @@ -3256,12 +3247,6 @@ qemuBlockCommit(virDomainObj *vm, if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0) return -1; - if (!top || STREQ(top, disk->dst)) - topSource = disk->src; - else if (!(topSource = virStorageSourceChainLookup(disk->src, NULL, top, - disk->dst, &top_parent))) - return -1; - if (topSource == disk->src) { /* XXX Should we auto-pivot when COMMIT_ACTIVE is not specified? */ if (!(flags & VIR_DOMAIN_BLOCK_COMMIT_ACTIVE)) { @@ -3280,22 +3265,16 @@ qemuBlockCommit(virDomainObj *vm, if (!virStorageSourceHasBacking(topSource)) { virReportError(VIR_ERR_INVALID_ARG, _("top '%s' in chain for '%s' has no backing file"), - topSource->path, path); + topSource->path, disk->src->path); return -1; } - if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) - baseSource = topSource->backingStore; - else if (!(baseSource = virStorageSourceChainLookup(disk->src, topSource, - base, disk->dst, NULL))) - 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'"), - base, topSource->path, path); + baseSource->path, topSource->path, disk->src->path); return -1; } @@ -3400,6 +3379,43 @@ qemuBlockCommit(virDomainObj *vm, } +int +qemuBlockCommit(virDomainObj *vm, + virQEMUDriver *driver, + const char *path, + const char *base, + const char *top, + unsigned long bandwidth, + unsigned int flags) +{ + virDomainDiskDef *disk = NULL; + virStorageSource *topSource; + virStorageSource *baseSource = NULL; + virStorageSource *top_parent = NULL; + + if (virDomainObjCheckActive(vm) < 0) + return -1; + + if (!(disk = qemuDomainDiskByName(vm->def, path))) + return -1; + + if (!top || STREQ(top, disk->dst)) + topSource = disk->src; + else if (!(topSource = virStorageSourceChainLookup(disk->src, NULL, top, + disk->dst, &top_parent))) + return -1; + + if (!base && (flags & VIR_DOMAIN_BLOCK_COMMIT_SHALLOW)) + baseSource = topSource->backingStore; + else if (!(baseSource = virStorageSourceChainLookup(disk->src, topSource, + base, disk->dst, NULL))) + return -1; + + return qemuBlockCommitImpl(vm, driver, disk, baseSource, topSource, + top_parent, bandwidth, flags); +} + + /* Called while holding the VM job lock, to implement a block job * abort with pivot; this updates the VM definition as appropriate, on * either success or failure. */ diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index d4b564e177..35f7718697 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -277,6 +277,16 @@ qemuBlockExportAddNBD(virDomainObj *vm, bool writable, const char *bitmap); +int +qemuBlockCommitImpl(virDomainObj *vm, + virQEMUDriver *driver, + virDomainDiskDef *disk, + virStorageSource *baseSource, + virStorageSource *topSource, + virStorageSource *top_parent, + unsigned long bandwidth, + unsigned int flags); + int qemuBlockCommit(virDomainObj *vm, virQEMUDriver *driver, -- 2.37.2

This will allow starting synchronous block commit job that will be used by snapshot delete code. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_block.c | 6 +++++- src/qemu/qemu_block.h | 1 + 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 902ec7b2a5..bc46966d22 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3211,6 +3211,7 @@ qemuBlockCommitImpl(virDomainObj *vm, virStorageSource *topSource, virStorageSource *top_parent, unsigned long bandwidth, + bool sync, unsigned int flags) { qemuDomainObjPrivate *priv = vm->privateData; @@ -3338,6 +3339,9 @@ qemuBlockCommitImpl(virDomainObj *vm, !(backingPath = qemuBlockGetBackingStoreString(baseSource, false))) goto error; + if (sync) + qemuBlockJobSyncBegin(job); + qemuDomainObjEnterMonitor(vm); rc = qemuMonitorBlockCommit(priv->mon, @@ -3412,7 +3416,7 @@ qemuBlockCommit(virDomainObj *vm, return -1; return qemuBlockCommitImpl(vm, driver, disk, baseSource, topSource, - top_parent, bandwidth, flags); + top_parent, bandwidth, false, flags); } diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 35f7718697..29b5ac9ea1 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -285,6 +285,7 @@ qemuBlockCommitImpl(virDomainObj *vm, virStorageSource *topSource, virStorageSource *top_parent, unsigned long bandwidth, + bool sync, unsigned int flags); int -- 2.37.2

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 +++++ 4 files changed, 44 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index c2808c75a3..6279e75c68 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2930,6 +2930,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 63269e15bc..dc35aa52c3 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 70fba50e6c..9d514bfacc 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4410,6 +4410,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 a53e6423df..37f76bcc0c 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -373,6 +373,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) -- 2.37.2

Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_block.c | 6 ++++-- src/qemu/qemu_block.h | 1 + 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 +- 7 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index bc46966d22..ce26afe611 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3212,6 +3212,7 @@ qemuBlockCommitImpl(virDomainObj *vm, virStorageSource *top_parent, unsigned long bandwidth, bool sync, + virTristateBool autofinalize, unsigned int flags) { qemuDomainObjPrivate *priv = vm->privateData; @@ -3349,7 +3350,7 @@ qemuBlockCommitImpl(virDomainObj *vm, job->name, topSource->nodeformat, baseSource->nodeformat, - backingPath, bandwidth); + backingPath, bandwidth, autofinalize); qemuDomainObjExitMonitor(vm); @@ -3416,7 +3417,8 @@ qemuBlockCommit(virDomainObj *vm, return -1; return qemuBlockCommitImpl(vm, driver, disk, baseSource, topSource, - top_parent, bandwidth, false, flags); + top_parent, bandwidth, false, + VIR_TRISTATE_BOOL_YES, flags); } diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 29b5ac9ea1..85dd9c086c 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -286,6 +286,7 @@ qemuBlockCommitImpl(virDomainObj *vm, virStorageSource *top_parent, unsigned long bandwidth, bool sync, + virTristateBool autofinalize, unsigned int flags); int diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 6279e75c68..670a601cd3 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 dc35aa52c3..67353241e5 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -974,7 +974,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 9d514bfacc..fe0b88e8c5 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4035,11 +4035,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 37f76bcc0c..ba7b70bd57 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -312,7 +312,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 b508f63aea..3c61b7ddf7 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1213,7 +1213,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, "/foo/bar") GEN_TEST_FUNC(qemuMonitorJSONOpenGraphics, "spice", "spicefd", false) GEN_TEST_FUNC(qemuMonitorJSONNBDServerAdd, "vda", "export", true, "bitmap") -- 2.37.2

Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_block.c | 16 ++++++++++++++++ src/qemu/qemu_block.h | 4 ++++ 2 files changed, 20 insertions(+) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index ce26afe611..92b0fc9812 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3541,3 +3541,19 @@ qemuBlockPivot(virDomainObj *vm, return ret; } + +int +qemuBlockFinalize(virDomainObj *vm, + qemuBlockJobData *job) +{ + int ret; + qemuDomainObjPrivate *priv = vm->privateData; + + qemuDomainObjEnterMonitor(vm); + + 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 85dd9c086c..82389d3a58 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -302,3 +302,7 @@ int qemuBlockPivot(virDomainObj *vm, qemuBlockJobData *job, virDomainDiskDef *disk); + +int +qemuBlockFinalize(virDomainObj *vm, + qemuBlockJobData *job); -- 2.37.2

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 | 13 ++++++++++++- src/qemu/qemu_blockjob.h | 1 + 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index 5280186970..40254c65cd 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..e13e915057 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,11 @@ qemuBlockJobEventProcess(virQEMUDriver *driver, job->newstate = -1; break; + case QEMU_BLOCKJOB_STATE_PENDING: + job->state = job->newstate; + qemuDomainSaveStatus(vm); + break; + case QEMU_BLOCKJOB_STATE_NEW: case QEMU_BLOCKJOB_STATE_RUNNING: case QEMU_BLOCKJOB_STATE_LAST: @@ -1684,13 +1692,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.37.2

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; --- 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 6033deafed..c1c67ac445 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.37.2

Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 46 ++++++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index c1c67ac445..b5e6a87566 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2281,6 +2281,35 @@ qemuSnapshotChildrenReparent(void *payload, } +static int +qemuSnapshotDeleteSingle(virDomainObj *vm, + virDomainMomentObj *snap, + virQEMUDriver *driver, + bool metadata_only) +{ + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + + if (snap->nchildren) { + virQEMUMomentReparent rep; + + rep.dir = cfg->snapshotDir; + rep.parent = snap->parent; + rep.vm = vm; + rep.err = 0; + rep.xmlopt = driver->xmlopt; + rep.writeMetadata = qemuDomainSnapshotWriteMetadata; + virDomainMomentForEachChild(snap, + qemuSnapshotChildrenReparent, + &rep); + if (rep.err < 0) + return -1; + virDomainMomentMoveChildren(snap, snap->parent); + } + + return qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); +} + + int qemuSnapshotDelete(virDomainObj *vm, virDomainSnapshotPtr snapshot, @@ -2290,7 +2319,6 @@ qemuSnapshotDelete(virDomainObj *vm, int ret = -1; virDomainMomentObj *snap = NULL; virQEMUMomentRemove rem; - virQEMUMomentReparent rep; bool metadata_only = !!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY); int external = 0; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); @@ -2358,21 +2386,7 @@ qemuSnapshotDelete(virDomainObj *vm, ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); } } else { - if (snap->nchildren) { - rep.dir = cfg->snapshotDir; - rep.parent = snap->parent; - rep.vm = vm; - rep.err = 0; - rep.xmlopt = driver->xmlopt; - rep.writeMetadata = qemuDomainSnapshotWriteMetadata; - virDomainMomentForEachChild(snap, - qemuSnapshotChildrenReparent, - &rep); - if (rep.err < 0) - goto endjob; - virDomainMomentMoveChildren(snap, snap->parent); - } - ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); + ret = qemuSnapshotDeleteSingle(vm, snap, driver, metadata_only); } endjob: -- 2.37.2

On Tue, Aug 23, 2022 at 18:32:13 +0200, Pavel Hrdina wrote: 'driver' can be extracted from the the private data of 'vm', removing the need to pass it in. Doing that will also remove the need to pass it in via 'struct qemuSnapshotDeleteAllData' in upcoming patch. You actually may be better off refactoring qemuDomainSnapshotDiscard first to remove the argument.

Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 94 ++++++++++++++++++++++------------------ 1 file changed, 53 insertions(+), 41 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index b5e6a87566..d40d75e75d 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2310,18 +2310,62 @@ qemuSnapshotDeleteSingle(virDomainObj *vm, } -int -qemuSnapshotDelete(virDomainObj *vm, - virDomainSnapshotPtr snapshot, - unsigned int flags) +static int +qemuSnapshotDeleteChildren(virDomainObj *vm, + virDomainMomentObj *snap, + virQEMUDriver *driver, + bool metadata_only, + unsigned int flags) { - virQEMUDriver *driver = snapshot->domain->conn->privateData; - int ret = -1; - virDomainMomentObj *snap = NULL; virQEMUMomentRemove rem; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + + rem.driver = driver; + rem.vm = vm; + rem.metadata_only = metadata_only; + rem.err = 0; + rem.current = virDomainSnapshotGetCurrent(vm->snapshots); + rem.found = false; + rem.momentDiscard = qemuDomainSnapshotDiscard; + virDomainMomentForEachDescendant(snap, qemuDomainMomentDiscardAll, + &rem); + if (rem.err < 0) + return -1; + if (rem.found) { + qemuSnapshotSetCurrent(vm, snap); + + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { + if (qemuDomainSnapshotWriteMetadata(vm, snap, + driver->xmlopt, + cfg->snapshotDir) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to set snapshot '%s' as current"), + snap->def->name); + virDomainSnapshotSetCurrent(vm->snapshots, NULL); + return -1; + } + } + } + + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { + virDomainMomentDropChildren(snap); + return 0; + } + + return qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); +} + + +int +qemuSnapshotDelete(virDomainObj *vm, + virDomainSnapshotPtr snapshot, + unsigned int flags) +{ + virQEMUDriver *driver = snapshot->domain->conn->privateData; + 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 | @@ -2352,39 +2396,7 @@ qemuSnapshotDelete(virDomainObj *vm, if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { - rem.driver = driver; - rem.vm = vm; - rem.metadata_only = metadata_only; - rem.err = 0; - rem.current = virDomainSnapshotGetCurrent(vm->snapshots); - rem.found = false; - rem.momentDiscard = qemuDomainSnapshotDiscard; - virDomainMomentForEachDescendant(snap, qemuDomainMomentDiscardAll, - &rem); - if (rem.err < 0) - goto endjob; - if (rem.found) { - qemuSnapshotSetCurrent(vm, snap); - - if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { - if (qemuDomainSnapshotWriteMetadata(vm, snap, - driver->xmlopt, - cfg->snapshotDir) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to set snapshot '%s' as current"), - snap->def->name); - virDomainSnapshotSetCurrent(vm->snapshots, NULL); - goto endjob; - } - } - } - - if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { - virDomainMomentDropChildren(snap); - ret = 0; - } else { - ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); - } + ret = qemuSnapshotDeleteChildren(vm, snap, driver, metadata_only, flags); } else { ret = qemuSnapshotDeleteSingle(vm, snap, driver, metadata_only); } -- 2.37.2

On Tue, Aug 23, 2022 at 18:32:15 +0200, Pavel Hrdina wrote: You should mention that the tradeoff is that some steps will get executed multiple times. So 'data->error' is initialized to '0' in the caller. By the logic above you'll never write to it if 'error' will actually carry a non-zero value. Since it's used as a glorified boolean (having only '0' or '-1' stored in it) I think you can drop the '&& data->error != 0) altogether and always update it when an error occured. [1]. Also I'd go with c99 style init ... ... to make this more obvious. Preferrably check that qemuSnapshotDeleteSingle returns -1 and return failure here instead ... So that we have the usual control flow.

On Tue, Aug 23, 2022 at 18:32:17 +0200, Pavel Hrdina wrote: This function is very misleading now. Specifically it does _NOT_ delete the metadata of @snap but rather modifies the metadata to update the parent appropriately. The metadata of @snap is still deleted in qemuSnapshotDiscard. The non-existant commit message and function header also doesn't help in understanding what this is actually supposed to do.

On Tue, Aug 23, 2022 at 18:32:18 +0200, Pavel Hrdina wrote: Okay, I now understand why you've needed to extrac the code, but it doesn't make it less confusing for the future in any way. You need to pick a better name and add comment for the function. Since you need to move the function please put it in the correct place at the time you've extracted it. Additionally as mentioned @driver can be fetched from @vm so you can make the header simpler. Hmm, so you are planning on moving also ... ... this code into that function? That will make sense at the end, but I think you approached it from the wrong end. I'd start with extracting the metadata deletion first and then move the reparenting of children into it later.

Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 65 ++++++++++++++++++++-------------------- 1 file changed, 32 insertions(+), 33 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 787186605f..591d6db39b 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2288,6 +2288,8 @@ qemuSnapshotDiscardMetadata(virDomainObj *vm, bool update_parent) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + g_autofree char *snapFile = NULL; + virDomainMomentObj *parentsnap = NULL; if (update_parent && snap->nchildren) { virQEMUMomentReparent rep; @@ -2306,6 +2308,36 @@ qemuSnapshotDiscardMetadata(virDomainObj *vm, virDomainMomentMoveChildren(snap, snap->parent); } + 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; } @@ -2318,10 +2350,7 @@ qemuSnapshotDiscard(virQEMUDriver *driver, 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)) { @@ -2355,36 +2384,6 @@ qemuSnapshotDiscard(virQEMUDriver *driver, if (qemuSnapshotDiscardMetadata(vm, snap, driver, update_parent) < 0) return -1; - 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; } -- 2.37.2

Signed-off-by: Pavel Hrdina <phrdina(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 591d6db39b..42bc410078 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2469,6 +2469,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, @@ -2478,7 +2505,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 | @@ -2491,20 +2517,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.37.2

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(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 72 +++++++++++++++++++++++++--------------- 1 file changed, 45 insertions(+), 27 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 42bc410078..da9b4c30f0 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, @@ -2469,26 +2454,59 @@ 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++; + qemuSnapshotCount count = { 0 }; if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | - VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { 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.37.2

In order to save some CPU cycles we will collect all the necessary data to delete external snapshot before we even start. They will be later used by code that deletes the snapshots and updates metadata when needed. With external snapshots we need data that libvirt gets from running QEMU process so if the VM is not running we need to start paused QEMU process for the snapshot deletion and kill at afterwards. Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 144 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 142 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index da9b4c30f0..dade5dcea4 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2232,6 +2232,120 @@ qemuSnapshotRevert(virDomainObj *vm, } +typedef struct { + virDomainMomentObj *parentSnap; + virDomainSnapshotDiskDef *snapDisk; + virDomainDiskDef *domDisk; + virDomainDiskDef *parentDomDisk; + virStorageSource *diskSrc; + virStorageSource *parentDiskSrc; + virStorageSource *prevDiskSrc; + qemuBlockJobData *job; +} qemuSnapshotDeleteExternalData; + + +static virDomainMomentObj* +qemuSnapshotFindParentSnapForDisk(virDomainMomentObj *snap, + virDomainSnapshotDiskDef *snapDisk) +{ + virDomainMomentObj *parentSnap = snap->parent; + + while (parentSnap) { + ssize_t i; + virDomainSnapshotDef *parentSnapdef = virDomainSnapshotObjGetDef(parentSnap); + + if (!parentSnapdef) + break; + + for (i = 0; i < parentSnapdef->ndisks; i++) { + virDomainSnapshotDiskDef *parentSnapDisk = &(parentSnapdef->disks[i]); + + if (parentSnapDisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_NO && + STREQ(snapDisk->name, parentSnapDisk->name)) { + return parentSnap; + } + } + + parentSnap = parentSnap->parent; + } + + return NULL; +} + + +static GPtrArray* +qemuSnapshotDeleteExternalPrepare(virDomainObj *vm, + virDomainMomentObj *snap) +{ + ssize_t i; + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(snap); + g_autoptr(GPtrArray) ret = g_ptr_array_new_full(0, g_free); + + for (i = 0; i < snapdef->ndisks; i++) { + g_autofree qemuSnapshotDeleteExternalData *data = NULL; + virDomainSnapshotDiskDef *snapDisk = &(snapdef->disks[i]); + + if (snapDisk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NO) + continue; + + data = g_new0(qemuSnapshotDeleteExternalData, 1); + data->snapDisk = snapDisk; + + data->domDisk = qemuDomainDiskByName(vm->def, snapDisk->name); + if (!data->domDisk) + return NULL; + + data->diskSrc = virStorageSourceChainLookup(data->domDisk->src, NULL, + data->snapDisk->src->path, + NULL, &data->prevDiskSrc); + if (!data->diskSrc) + return NULL; + + if (!virStorageSourceIsSameLocation(data->diskSrc, data->snapDisk->src)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("VM disk source and snapshot disk source are not the same")); + return NULL; + } + + data->parentDomDisk = virDomainDiskByTarget(snapdef->parent.dom, + data->snapDisk->name); + if (!data->parentDomDisk) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("failed to find disk '%s' in snapshot VM XML"), + snapDisk->name); + return NULL; + } + + data->parentDiskSrc = data->diskSrc->backingStore; + if (!data->parentDiskSrc) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("failed to find parent disk source in backing chain")); + return NULL; + } + + if (!virStorageSourceIsSameLocation(data->parentDiskSrc, data->parentDomDisk->src)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("snapshot VM disk source and parent disk source are not the same")); + return NULL; + } + + data->parentSnap = qemuSnapshotFindParentSnapForDisk(snap, data->snapDisk); + + if (data->parentSnap && !virDomainSnapshotIsExternal(data->parentSnap)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("parent snapshot '%s' for disk '%s' is internal snapshot"), + snapDisk->name, + data->parentSnap->def->name); + return NULL; + } + + g_ptr_array_add(ret, g_steal_pointer(&data)); + } + + return g_steal_pointer(&ret); +} + + typedef struct _virQEMUMomentReparent virQEMUMomentReparent; struct _virQEMUMomentReparent { const char *dir; @@ -2504,9 +2618,9 @@ qemuSnapshotDeleteValidate(virDomainMomentObj *snap, } 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 snapshot with internal children disk snapshots not supported")); return -1; } @@ -2523,6 +2637,8 @@ qemuSnapshotDelete(virDomainObj *vm, int ret = -1; virDomainMomentObj *snap = NULL; bool metadata_only = !!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY); + bool stop_qemu = false; + g_autoptr(GPtrArray) externalData = NULL; virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY | @@ -2537,6 +2653,25 @@ qemuSnapshotDelete(virDomainObj *vm, if (!metadata_only) { if (qemuSnapshotDeleteValidate(snap, flags) < 0) goto endjob; + + if (virDomainSnapshotIsExternal(snap) && + !(flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY))) { + if (!virDomainObjIsActive(vm)) { + if (qemuProcessStart(NULL, driver, vm, NULL, VIR_ASYNC_JOB_NONE, + NULL, -1, NULL, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_CREATE, + VIR_QEMU_PROCESS_START_PAUSED) < 0) { + goto endjob; + } + + stop_qemu = true; + } + + externalData = qemuSnapshotDeleteExternalPrepare(vm, snap); + if (!externalData) + goto endjob; + } } if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | @@ -2547,6 +2682,11 @@ qemuSnapshotDelete(virDomainObj *vm, } endjob: + if (stop_qemu) { + qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN, + VIR_ASYNC_JOB_NONE, 0); + } + qemuDomainObjEndJob(vm); return ret; -- 2.37.2

On Tue, Sep 06, 2022 at 12:19:38PM +0200, Peter Krempa wrote: There is no need to access these randomly but G(S)List doesn't work that nicely with g_autoptr and dynamically allocated elements. For this case we would have to use g_list_free_full(). Yeah I don't like this ordering as well, I'll check what parts of the qemuSnapshotDeleteExternalPrepare() can be done without qemu running. Good point, will look into it. Pavel

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(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 163 +++++++++++++++++++++++++++++++++++---- 1 file changed, 147 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index dade5dcea4..64ee395230 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2380,6 +2380,114 @@ qemuSnapshotChildrenReparent(void *payload, } +static int +qemuSnapshotJobRunning(virDomainObj *vm, + qemuBlockJobData *job) +{ + qemuBlockJobUpdate(vm, job, VIR_ASYNC_JOB_NONE); + + while (job->state != QEMU_BLOCKJOB_STATE_READY && + job->state != QEMU_BLOCKJOB_STATE_PENDING && + job->state != QEMU_BLOCKJOB_STATE_FAILED && + job->state != QEMU_BLOCKJOB_STATE_CANCELLED && + job->state != QEMU_BLOCKJOB_STATE_COMPLETED) { + if (qemuDomainObjWait(vm) < 0) + return -1; + qemuBlockJobUpdate(vm, job, VIR_ASYNC_JOB_NONE); + } + + return 0; +} + + +static int +qemuSnapshotJobFinishing(virDomainObj *vm, + qemuBlockJobData *job) +{ + qemuBlockJobUpdate(vm, job, VIR_ASYNC_JOB_NONE); + + while (job->state != QEMU_BLOCKJOB_STATE_READY && + job->state != QEMU_BLOCKJOB_STATE_FAILED && + job->state != QEMU_BLOCKJOB_STATE_CANCELLED && + job->state != QEMU_BLOCKJOB_STATE_COMPLETED) { + if (qemuDomainObjWait(vm) < 0) + return -1; + qemuBlockJobUpdate(vm, job, VIR_ASYNC_JOB_NONE); + } + + return 0; +} + + +static int +qemuSnapshotDiscardExternal(virDomainObj *vm, + virQEMUDriver *driver, + GPtrArray *externalData) +{ + ssize_t i; + + for (i = 0; i < externalData->len; i++) { + qemuSnapshotDeleteExternalData *data = g_ptr_array_index(externalData, i); + 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; + } + + if (qemuBlockCommitImpl(vm, driver, + data->domDisk, + data->parentDiskSrc, + data->diskSrc, + data->prevDiskSrc, + 0, true, autofinalize, commitFlags) < 0) { + return -1; + } + + data->job = qemuBlockJobDiskGetJob(data->domDisk); + if (!data->job) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("disk '%s' does not have an active block job"), + data->domDisk->dst); + return -1; + } + } + + for (i = 0; i < externalData->len; i++) { + qemuSnapshotDeleteExternalData *data = g_ptr_array_index(externalData, i); + + if (qemuSnapshotJobRunning(vm, data->job) < 0) + return -1; + + if (data->job->state == QEMU_BLOCKJOB_STATE_FAILED) + return -1; + } + + for (i = 0; i < externalData->len; i++) { + qemuSnapshotDeleteExternalData *data = g_ptr_array_index(externalData, i); + + if (data->job->state == QEMU_BLOCKJOB_STATE_READY) { + if (qemuBlockPivot(vm, data->job, NULL) < 0) + return -1; + } else if (data->job->state == QEMU_BLOCKJOB_STATE_PENDING) { + if (qemuBlockFinalize(vm, data->job) < 0) + return -1; + } + + if (qemuSnapshotJobFinishing(vm, data->job) < 0) + return -1; + + if (data->job->state == QEMU_BLOCKJOB_STATE_FAILED) + return -1; + + qemuBlockJobSyncEnd(vm, data->job, VIR_ASYNC_JOB_NONE); + } + + return 0; +} + + static int qemuSnapshotDiscardMetadata(virDomainObj *vm, virDomainMomentObj *snap, @@ -2443,11 +2551,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, + GPtrArray *externalData, + bool update_parent, + bool metadata_only) { qemuDomainObjPrivate *priv; @@ -2469,14 +2578,24 @@ qemuSnapshotDiscard(virQEMUDriver *driver, return -1; } - if (qemuDomainSnapshotForEachQcow2(driver, def, snap, "-d", true) < 0) - return -1; + if (virDomainSnapshotIsExternal(snap)) { + if (qemuSnapshotDiscardExternal(vm, driver, externalData) < 0) + return -1; + } else { + 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 (virDomainSnapshotIsExternal(snap)) { + if (qemuSnapshotDiscardExternal(vm, driver, externalData) < 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); + } } } @@ -2487,6 +2606,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) @@ -2509,9 +2639,10 @@ static int qemuSnapshotDeleteSingle(virDomainObj *vm, virDomainMomentObj *snap, virQEMUDriver *driver, + GPtrArray *externalData, bool metadata_only) { - return qemuSnapshotDiscard(driver, vm, snap, true, metadata_only); + return qemuSnapshotDiscardImpl(driver, vm, snap, externalData, true, metadata_only); } @@ -2532,7 +2663,7 @@ qemuSnapshotDeleteAll(void *payload, virDomainMomentObj *snap = payload; struct qemuSnapshotDeleteAllData *data = opaque; - error = qemuSnapshotDeleteSingle(data->vm, snap, data->driver, + error = qemuSnapshotDeleteSingle(data->vm, snap, data->driver, NULL, data->metadata_only); if (error != 0 && data->error != 0) @@ -2561,7 +2692,7 @@ qemuSnapshotDeleteChildren(virDomainObj *vm, return -1; if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) { - return qemuSnapshotDeleteSingle(vm, snap, driver, metadata_only); + return qemuSnapshotDeleteSingle(vm, snap, driver, NULL, metadata_only); } return 0; @@ -2678,7 +2809,7 @@ qemuSnapshotDelete(virDomainObj *vm, VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { ret = qemuSnapshotDeleteChildren(vm, snap, driver, metadata_only, flags); } else { - ret = qemuSnapshotDeleteSingle(vm, snap, driver, metadata_only); + ret = qemuSnapshotDeleteSingle(vm, snap, driver, externalData, metadata_only); } endjob: -- 2.37.2

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(a)redhat.com&gt; --- src/qemu/qemu_snapshot.c | 116 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 64ee395230..37ae3f04d0 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2380,6 +2380,109 @@ qemuSnapshotChildrenReparent(void *payload, } +typedef struct _qemuSnapshotUpdateDisksData qemuSnapshotUpdateDisksData; +struct _qemuSnapshotUpdateDisksData { + virDomainMomentObj *snap; + virDomainObj *vm; + virQEMUDriver *driver; + int error; + int (*writeMetadata)(virDomainObj *, virDomainMomentObj *, + virDomainXMLOption *, const char *); +}; + + +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; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(data->driver); + virDomainSnapshotDef *snapdef = virDomainSnapshotObjGetDef(data->snap); + ssize_t i; + + if (data->error < 0) + return 0; + + 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; + return 0; + } + + 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; + return 0; + } + } + } + + data->error = data->writeMetadata(data->vm, + snap, + data->driver->xmlopt, + cfg->snapshotDir); + return 0; +} + + static int qemuSnapshotJobRunning(virDomainObj *vm, qemuBlockJobData *job) @@ -2500,6 +2603,7 @@ qemuSnapshotDiscardMetadata(virDomainObj *vm, if (update_parent && snap->nchildren) { virQEMUMomentReparent rep; + qemuSnapshotUpdateDisksData data; rep.dir = cfg->snapshotDir; rep.parent = snap->parent; @@ -2512,6 +2616,18 @@ qemuSnapshotDiscardMetadata(virDomainObj *vm, &rep); if (rep.err < 0) return -1; + + data.snap = snap; + data.driver = driver; + data.vm = vm; + data.error = 0; + data.writeMetadata = qemuDomainSnapshotWriteMetadata; + virDomainMomentForEachDescendant(snap, + qemuSnapshotDeleteUpdateDisks, + &data); + if (data.error < 0) + return -1; + virDomainMomentMoveChildren(snap, snap->parent); } -- 2.37.2

On Wed, Sep 07, 2022 at 03:06:51PM +0200, Peter Krempa wrote: Needs to be documented as pointed elsewhere. The first part is relevant only to external snapshots because when you delete one the whole image disappears and you need to update other metadata files to not point to the deleted file anymore. So for example: test.qcow2 | +-test.snap1 | +-test.snap2 There the snap2 metadata file will have in the domain def for the state before snapshot 'test.snap1' as a disk source. If you delete the snap1 we need to change the source to 'test.qcow2' in the snap2 metadata. But the next part that modifies the backing chain in the domain definition is relevant to internal snapshot metadata files as well. Pavel This removes source we are deleting from the backing chain. Could probably rename next -> cur and cur -> prev.

On Tue, Aug 23, 2022 at 18:32:26 +0200, Pavel Hrdina wrote: You shouldn't allow parsing this unless a snapshot is being redefined/reloaded from disk. Once again please make sure to document non-obvious behaviour. In this patch it's specifically where the disks of the _parent_ snapshot are marked as invalid. I've overlooked that first.