Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
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.2On Tue, Aug 23, 2022 at 18:32:04 +0200, Pavel Hrdina wrote: Commit message could be a bit more verbose. > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > --- > 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(-) There's nothing wrong with this commit itself but for the next commit I'm going to suggest that all the bits for extracting the virStorageSource objects based on the names are put back (or kept) into qemuDomainBlockCommit, as that bit is definitely not resuable. In the end I'd suggest that qemuBlockCommit will actually be what you have for qemuBlockCommitImpl and all the checks are kept in qemuDomainBlockCommit. With that it might make sense to do that in a single step rather than moving the code back and forth.
© 2016 - 2026 Red Hat, Inc.