[libvirt] [PATCH 00/12] qemu: cleanups and improvements of blockjob handling (blockdev-add saga)

Peter Krempa (12): qemu: blockjob: Remove 'started' from struct _qemuBlockJobData qemu: blockjob: Fix documentation for 'newstate' of _qemuBlockJobData qemu: driver: Don't try to update blockjob status in qemuDomainGetBlockJobInfo qemu: driver: Set mirror state after successful command qemu: Modernize memory cleaning in qemuDomainBlockCopyCommon qemu: Modernize memory cleaning in qemuDomainBlockPullCommon qemu: Modernize memory cleaning in qemuDomainBlockCommit qemu: Remove unecessary error keeping in qemuDomainBlockCopyCommon qemu: Remove unnecessary calls to qemuDomainStorageSourceAccessRevoke qemu: Validate backing store of 'mirror' for block copy qemu: Simplify allowing access to storage file for block copy qemu: blockcopy: sanitize permission handling for 'mirror' src/qemu/qemu_blockjob.h | 3 +- src/qemu/qemu_driver.c | 185 ++++++++++++++++++++------------------- 2 files changed, 97 insertions(+), 91 deletions(-) -- 2.21.0

This field is no longer used. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h index c7325c6daf..31d8151ef5 100644 --- a/src/qemu/qemu_blockjob.h +++ b/src/qemu/qemu_blockjob.h @@ -71,7 +71,6 @@ struct _qemuBlockJobData { virDomainDiskDefPtr disk; /* may be NULL, if blockjob does not correspond to any disk */ - bool started; int type; /* qemuBlockJobType */ int state; /* qemuBlockjobState */ char *errmsg; -- 2.21.0

On Fri, May 17, 2019 at 01:19:47PM +0200, Peter Krempa wrote:
This field is no longer used.
as of commit d1a44634acead75bc48fb9b0f68dc1ebf6c764fd
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.h | 1 - 1 file changed, 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

When used with the new job handler the values will also include some of the non-public values from qemuBlockjobState. Modify the comment to clarify this. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h index 31d8151ef5..772cc5cba1 100644 --- a/src/qemu/qemu_blockjob.h +++ b/src/qemu/qemu_blockjob.h @@ -76,7 +76,7 @@ struct _qemuBlockJobData { char *errmsg; bool synchronous; /* API call is waiting for this job */ - int newstate; /* virConnectDomainEventBlockJobStatus - new state to be processed */ + int newstate; /* qemuBlockjobState, subset of events emitted by qemu */ }; -- 2.21.0

On Fri, May 17, 2019 at 01:19:48PM +0200, Peter Krempa wrote:
When used with the new job handler the values will also include some of the non-public values from qemuBlockjobState. Modify the comment to clarify this.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

All blockjobs get their status updated by events from qemu, so this code no longer makes sense. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0a425b82e5..33ccb4dfba 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17442,20 +17442,6 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, goto endjob; } - /* Snoop block copy operations, so future cancel operations can - * avoid checking if pivot is safe. Save the change to XML, but - * we can ignore failure because it is only an optimization. We - * hold the vm lock, so modifying the in-memory representation is - * safe, even if we are a query rather than a modify job. */ - if (disk->mirror && - rawInfo.ready != 0 && - info->cur == info->end && !disk->mirrorState) { - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY; - ignore_value(virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps)); - virObjectUnref(cfg); - } endjob: qemuDomainObjEndJob(driver, vm); -- 2.21.0

On Fri, May 17, 2019 at 01:19:49PM +0200, Peter Krempa wrote:
All blockjobs get their status updated by events from qemu, so this code no longer makes sense.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 14 -------------- 1 file changed, 14 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

When aborting or pivoting a block job we record which operation we do for the mirror in the virDomainDiskDef structure. As everything is synchronized by a job it's not necessary to modify the state prior to calling the monitor and reseting the state on failure. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 33ccb4dfba..a7dd4c882f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17107,7 +17107,6 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, * XXX If the abort command is synchronous but the qemu event says * that pivot failed, we need to reflect that failure into the * overall return value. */ - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT; qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorDrivePivot(priv->mon, job->name); if (qemuDomainObjExitMonitor(driver, vm) < 0) { @@ -17115,11 +17114,11 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, goto cleanup; } - if (ret < 0) { - /* The pivot failed. The block job in QEMU remains in the synchronised - * phase. Reset the state we changed and return the error to the user */ - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY; - } + /* The pivot failed. The block job in QEMU remains in the synchronised state */ + if (ret < 0) + goto cleanup; + + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT; cleanup: return ret; @@ -17298,9 +17297,6 @@ qemuDomainBlockJobAbort(virDomainPtr dom, if ((ret = qemuDomainBlockPivot(driver, vm, job, disk)) < 0) goto endjob; } else { - if (disk->mirror) - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_ABORT; - qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorBlockJobCancel(qemuDomainGetMonitor(vm), job->name); if (qemuDomainObjExitMonitor(driver, vm) < 0) { @@ -17308,11 +17304,11 @@ qemuDomainBlockJobAbort(virDomainPtr dom, goto endjob; } - if (ret < 0) { - if (disk->mirror) - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; + if (ret < 0) goto endjob; - } + + if (disk->mirror) + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_ABORT; } ignore_value(virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps)); -- 2.21.0

On Fri, May 17, 2019 at 01:19:50PM +0200, Peter Krempa wrote:
When aborting or pivoting a block job we record which operation we do for the mirror in the virDomainDiskDef structure. As everything is synchronized by a job it's not necessary to modify the state prior to calling the monitor and reseting the state on failure.
resetting
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

Use VIR_AUTOFREE, VIR_AUTOUNREF, and VIR_STEAL_PTR. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a7dd4c882f..c5ff61124b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17572,7 +17572,7 @@ static int qemuDomainBlockCopyCommon(virDomainObjPtr vm, virConnectPtr conn, const char *path, - virStorageSourcePtr mirror, + virStorageSourcePtr mirrorsrc, unsigned long long bandwidth, unsigned int granularity, unsigned long long buf_size, @@ -17581,15 +17581,16 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, { virQEMUDriverPtr driver = conn->privateData; qemuDomainObjPrivatePtr priv; - char *device = NULL; + VIR_AUTOFREE(char *) device = NULL; virDomainDiskDefPtr disk = NULL; int ret = -1; bool need_unlink = false; - virQEMUDriverConfigPtr cfg = NULL; + VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); const char *format = NULL; virErrorPtr monitor_error = NULL; bool reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT); qemuBlockJobDataPtr job = NULL; + VIR_AUTOUNREF(virStorageSourcePtr) mirror = mirrorsrc; /* Preliminaries: find the disk we are editing, sanity checks */ virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW | @@ -17597,12 +17598,11 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, VIR_DOMAIN_BLOCK_COPY_TRANSIENT_JOB, -1); priv = vm->privateData; - cfg = virQEMUDriverGetConfig(driver); if (virStorageSourceIsRelative(mirror)) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("absolute path must be used as block copy target")); - goto cleanup; + return -1; } if (bandwidth > LLONG_MAX) { @@ -17610,11 +17610,11 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, _("bandwidth must be less than " "'%llu' bytes/s (%llu MiB/s)"), LLONG_MAX, LLONG_MAX >> 20); - goto cleanup; + return -1; } if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) - goto cleanup; + return -1; if (virDomainObjCheckActive(vm) < 0) goto endjob; @@ -17760,8 +17760,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, qemuBlockJobStarted(job); need_unlink = false; virStorageFileDeinit(mirror); - disk->mirror = mirror; - mirror = NULL; + VIR_STEAL_PTR(disk->mirror, mirror); disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY; if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) @@ -17779,10 +17778,6 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, } qemuBlockJobStartupFinalize(job); - cleanup: - VIR_FREE(device); - virObjectUnref(cfg); - virObjectUnref(mirror); return ret; } -- 2.21.0

On Fri, May 17, 2019 at 01:19:51PM +0200, Peter Krempa wrote:
Use VIR_AUTOFREE, VIR_AUTOUNREF, and VIR_STEAL_PTR.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

Use VIR_AUTOFREE and VIR_AUTOUNREF. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c5ff61124b..c51b0494a0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17136,13 +17136,13 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver, unsigned int flags) { qemuDomainObjPrivatePtr priv = vm->privateData; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - char *device = NULL; + VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); + VIR_AUTOFREE(char *) device = NULL; virDomainDiskDefPtr disk; virStorageSourcePtr baseSource = NULL; unsigned int baseIndex = 0; - char *basePath = NULL; - char *backingPath = NULL; + VIR_AUTOFREE(char *) basePath = NULL; + VIR_AUTOFREE(char *) backingPath = NULL; unsigned long long speed = bandwidth; qemuBlockJobDataPtr job = NULL; int ret = -1; @@ -17235,10 +17235,6 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver, cleanup: qemuBlockJobStartupFinalize(job); - virObjectUnref(cfg); - VIR_FREE(basePath); - VIR_FREE(backingPath); - VIR_FREE(device); virDomainObjEndAPI(&vm); return ret; } -- 2.21.0

On Fri, May 17, 2019 at 01:19:52PM +0200, Peter Krempa wrote:
Use VIR_AUTOFREE and VIR_AUTOUNREF.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

Use VIR_AUTOFREE and VIR_AUTOUNREF. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c51b0494a0..5f9f547118 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17961,10 +17961,10 @@ qemuDomainBlockCommit(virDomainPtr dom, unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; - virQEMUDriverConfigPtr cfg = NULL; + VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = NULL; qemuDomainObjPrivatePtr priv; virDomainObjPtr vm = NULL; - char *device = NULL; + VIR_AUTOFREE(char *) device = NULL; int ret = -1; virDomainDiskDefPtr disk = NULL; virStorageSourcePtr topSource; @@ -17973,9 +17973,9 @@ qemuDomainBlockCommit(virDomainPtr dom, unsigned int baseIndex = 0; virStorageSourcePtr top_parent = NULL; bool clean_access = false; - char *topPath = NULL; - char *basePath = NULL; - char *backingPath = NULL; + VIR_AUTOFREE(char *) topPath = NULL; + VIR_AUTOFREE(char *) basePath = NULL; + VIR_AUTOFREE(char *) backingPath = NULL; unsigned long long speed = bandwidth; qemuBlockJobDataPtr job = NULL; qemuBlockJobType jobtype = QEMU_BLOCKJOB_TYPE_COMMIT; @@ -18174,11 +18174,6 @@ qemuDomainBlockCommit(virDomainPtr dom, qemuDomainObjEndJob(driver, vm); cleanup: - VIR_FREE(topPath); - VIR_FREE(basePath); - VIR_FREE(backingPath); - VIR_FREE(device); - virObjectUnref(cfg); virDomainObjEndAPI(&vm); return ret; } -- 2.21.0

On Fri, May 17, 2019 at 01:19:53PM +0200, Peter Krempa wrote:
Use VIR_AUTOFREE and VIR_AUTOUNREF.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

Since 3decae00e90 qemuDomainStorageSourceAccessRevoke keeps the libvirt error which was set prior to the call around even after the call, thus we don't need to do the same when reverting access in the block copy code. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5f9f547118..d7b849f953 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17583,7 +17583,6 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, bool need_unlink = false; VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); const char *format = NULL; - virErrorPtr monitor_error = NULL; bool reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT); qemuBlockJobDataPtr job = NULL; VIR_AUTOUNREF(virStorageSourcePtr) mirror = mirrorsrc; @@ -17747,7 +17746,6 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; if (ret < 0) { - monitor_error = virSaveLastError(); qemuDomainStorageSourceAccessRevoke(driver, vm, mirror); goto endjob; } @@ -17768,10 +17766,6 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, VIR_WARN("%s", _("unable to remove just-created copy target")); virStorageFileDeinit(mirror); qemuDomainObjEndJob(driver, vm); - if (monitor_error) { - virSetError(monitor_error); - virFreeError(monitor_error); - } qemuBlockJobStartupFinalize(job); return ret; -- 2.21.0

On Fri, May 17, 2019 at 01:19:54PM +0200, Peter Krempa wrote:
Since 3decae00e90 qemuDomainStorageSourceAccessRevoke keeps the libvirt error which was set prior to the call around even after the call, thus we don't need to do the same when reverting access in the block copy code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 6 ------ 1 file changed, 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

Since 3decae00e90 qemuDomainStorageSourceAccessAllow revokes the permissions it granted if it fails halfway, thus we can remove some calls to qemuDomainStorageSourceAccessRevoke which tried to undo this situation. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d7b849f953..66a6eb0483 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15213,10 +15213,8 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, } /* set correct security, cgroup and locking options on the new image */ - if (qemuDomainStorageSourceAccessAllow(driver, vm, dd->src, false, true) < 0) { - qemuDomainStorageSourceAccessRevoke(driver, vm, dd->src); + if (qemuDomainStorageSourceAccessAllow(driver, vm, dd->src, false, true) < 0) goto cleanup; - } dd->prepared = true; @@ -17725,10 +17723,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, qemuSecuritySetImageLabel(driver, vm, mirror, true) < 0) goto endjob; } else { - if (qemuDomainStorageSourceAccessAllow(driver, vm, mirror, false, true) < 0) { - qemuDomainStorageSourceAccessRevoke(driver, vm, mirror); + if (qemuDomainStorageSourceAccessAllow(driver, vm, mirror, false, true) < 0) goto endjob; - } } if (!(job = qemuBlockJobDiskNew(disk, QEMU_BLOCKJOB_TYPE_COPY, device))) -- 2.21.0

On Fri, May 17, 2019 at 01:19:55PM +0200, Peter Krempa wrote:
Since 3decae00e90 qemuDomainStorageSourceAccessAllow revokes the permissions it granted if it fails halfway, thus we can remove some calls to qemuDomainStorageSourceAccessRevoke which tried to undo this situation.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

Since 4e797f1a we parse backingStore of mirror which will later be used with blockdev. Add some validation for the user passed mirror at the current point to make sure it's not used improperly. Validate that it's not used without blockdev and also that it's not passed when not requesting a shallow copy. Also add a chain terminator for a deep copy since we know the resulting mirror will not have chain. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 72 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 66 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 66a6eb0483..a9c41d1592 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17560,6 +17560,63 @@ qemuDomainBlockCopyValidateMirror(virStorageSourcePtr mirror, } +/** + * qemuDomainBlockCopyCommonValidateUserMirrorBackingStore: + * @mirror: target of the block copy + * @flags: block copy API flags + * @blockdev: true if blockdev is used for the VM + * + * Validates whether backingStore of @mirror makes sense according to @flags. + * This makes sure that: + * 1) mirror has a terminator if it isn't supposed to have backing chain + * 2) if shallow copy is requested there is a chain or prepopulated image + * 3) user specified chain is present only when blockdev is used + * 4) if deep copy is requested, there's no chain + */ +static int +qemuDomainBlockCopyCommonValidateUserMirrorBackingStore(virStorageSourcePtr mirror, + unsigned int flags, + bool blockdev) +{ + /* note that if original disk does not have backing chain, shallow is cleared */ + bool shallow = flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW; + bool reuse = flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT; + + if (!mirror->backingStore) { + /* deep copy won't need backing store so we can terminate it */ + if (!shallow && + !(mirror->backingStore = virStorageSourceNew())) + return -1; + + return 0; + } + + /* validate user provided backing store */ + if (virStorageSourceHasBacking(mirror)) { + if (!blockdev) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("backingStore of mirror target is not supported by this qemu")); + return -1; + } + + if (!shallow) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("backingStore of mirror without VIR_DOMAIN_BLOCK_COPY_SHALLOW doesn't make sense")); + return -1; + } + } else { + /* shallow copy without reuse requires some kind of backing data */ + if (!reuse && shallow) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("VIR_DOMAIN_BLOCK_COPY_SHALLOW implies backing chain for mirror")); + return -1; + } + } + + return 0; +} + + /* bandwidth in bytes/s. Caller must lock vm beforehand, and not * access mirror afterwards. */ static int @@ -17574,7 +17631,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, bool keepParentLabel) { virQEMUDriverPtr driver = conn->privateData; - qemuDomainObjPrivatePtr priv; + qemuDomainObjPrivatePtr priv = vm->privateData; VIR_AUTOFREE(char *) device = NULL; virDomainDiskDefPtr disk = NULL; int ret = -1; @@ -17584,14 +17641,13 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, bool reuse = !!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT); qemuBlockJobDataPtr job = NULL; VIR_AUTOUNREF(virStorageSourcePtr) mirror = mirrorsrc; + bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); /* Preliminaries: find the disk we are editing, sanity checks */ virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW | VIR_DOMAIN_BLOCK_COPY_REUSE_EXT | VIR_DOMAIN_BLOCK_COPY_TRANSIENT_JOB, -1); - priv = vm->privateData; - if (virStorageSourceIsRelative(mirror)) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("absolute path must be used as block copy target")); @@ -17640,6 +17696,10 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, if (!virStorageSourceHasBacking(disk->src)) flags &= ~VIR_DOMAIN_BLOCK_COPY_SHALLOW; + if (qemuDomainBlockCopyCommonValidateUserMirrorBackingStore(mirror, flags, + blockdev) < 0) + goto endjob; + /* unless the user provides a pre-created file, shallow copy into a raw * file is not possible */ if ((flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW) && !reuse && @@ -17705,11 +17765,11 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, keepParentLabel) < 0) goto endjob; - /* If reusing an external image that includes a backing file, the pivot may - * result in qemu needing to open the entire backing chain, so we need to - * label the full backing chain of the mirror instead of just the top image */ + /* If reusing an external image that includes a backing file but the user + * did not enumerate the chain in the XML we need to detect the chain */ if (flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT && mirror->format >= VIR_STORAGE_FILE_BACKING && + mirror->backingStore == NULL && qemuDomainDetermineDiskChain(driver, vm, disk, mirror, true) < 0) goto endjob; -- 2.21.0

On Fri, May 17, 2019 at 01:19:56PM +0200, Peter Krempa wrote:
Since 4e797f1a we parse backingStore of mirror which will later be used with blockdev. Add some validation for the user passed mirror at the current point to make sure it's not used improperly.
Validate that it's not used without blockdev and also that it's not passed when not requesting a shallow copy. Also add a chain terminator for a deep copy since we know the resulting mirror will not have chain.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 72 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 66 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

One code path open-coded qemuDomainStorageSourceChainAccessAllow badly and also did not integrate with the locking code. Replace the separate calls with qemuDomainStorageSourceChainAccessAllow which does everything internally. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a9c41d1592..bd04d21907 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17778,9 +17778,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, /* note that we don't really know whether a part of the backing chain * is shared so rolling this back is not as easy. Thus we do it only * if there's a backing chain */ - if (qemuDomainNamespaceSetupDisk(vm, mirror) < 0 || - qemuSetupImageChainCgroup(vm, mirror) < 0 || - qemuSecuritySetImageLabel(driver, vm, mirror, true) < 0) + if (qemuDomainStorageSourceChainAccessAllow(driver, vm, mirror) < 0) goto endjob; } else { if (qemuDomainStorageSourceAccessAllow(driver, vm, mirror, false, true) < 0) -- 2.21.0

On Fri, May 17, 2019 at 01:19:57PM +0200, Peter Krempa wrote:
One code path open-coded qemuDomainStorageSourceChainAccessAllow badly and also did not integrate with the locking code.
Replace the separate calls with qemuDomainStorageSourceChainAccessAllow which does everything internally.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano

At the point when we want to modify the permissions for the 'mirror' we know whether it is supposed to have a backing chain or no. Given that mirror->backingStore is populated only when we'd need to touch it ayways we can use qemuDomainStorageSourceChainAccessAllow even in place of qemuDomainStorageSourceAccessAllow used for other cases to simplify the code. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bd04d21907..89ad6e0606 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17773,17 +17773,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, qemuDomainDetermineDiskChain(driver, vm, disk, mirror, true) < 0) goto endjob; - if (flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT && - virStorageSourceHasBacking(mirror)) { - /* note that we don't really know whether a part of the backing chain - * is shared so rolling this back is not as easy. Thus we do it only - * if there's a backing chain */ - if (qemuDomainStorageSourceChainAccessAllow(driver, vm, mirror) < 0) - goto endjob; - } else { - if (qemuDomainStorageSourceAccessAllow(driver, vm, mirror, false, true) < 0) - goto endjob; - } + if (qemuDomainStorageSourceChainAccessAllow(driver, vm, mirror) < 0) + goto endjob; if (!(job = qemuBlockJobDiskNew(disk, QEMU_BLOCKJOB_TYPE_COPY, device))) goto endjob; @@ -17800,7 +17791,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; if (ret < 0) { - qemuDomainStorageSourceAccessRevoke(driver, vm, mirror); + qemuDomainStorageSourceChainAccessRevoke(driver, vm, mirror); goto endjob; } -- 2.21.0

On Fri, May 17, 2019 at 01:19:58PM +0200, Peter Krempa wrote:
At the point when we want to modify the permissions for the 'mirror' we know whether it is supposed to have a backing chain or no. Given that mirror->backingStore is populated only when we'd need to touch it ayways we can use qemuDomainStorageSourceChainAccessAllow even in place of qemuDomainStorageSourceAccessAllow used for other cases to simplify the code.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko
-
Peter Krempa