[libvirt] [PATCH 0/5] Block job handling related fixes (blockdev-add saga)
Fix the generated documentation and clean up a few corner cases in qemu's block job handling. Peter Krempa (5): lib: Fix docs generated for enum virDomainBlockJobType qemu: blockjob: Make sure that internal states are not reported as event qemu: blockjob: Mark job as started only when it's new qemu: Clear block copy mirror state explicitly qemu: Don't double-free disk->mirror if block commit initialization fails include/libvirt/libvirt-domain.h | 3 ++- src/qemu/qemu_blockjob.c | 7 ++++++- src/qemu/qemu_driver.c | 34 ++++++++++++++------------------ 3 files changed, 23 insertions(+), 21 deletions(-) -- 2.20.1
Mixing documentation strings trailing the enum value and preceeding the enum value ends in a big mixup. Fix docs string for VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN so that it's not squished together with the next one. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- include/libvirt/libvirt-domain.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 6125d45008..0388911ded 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2375,7 +2375,8 @@ int virDomainSetPerfEvents(virDomainPtr dom, * Describes various possible block jobs. */ typedef enum { - VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN = 0, /* Placeholder */ + /* Placeholder */ + VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN = 0, /* Block Pull (virDomainBlockPull, or virDomainBlockRebase without * flags), job ends on completion */ -- 2.20.1
While the callers should make sure that they don't call qemuBlockJobEmitEvents for any internal state or job, let's add checks that prevents us from emitting wrong events altogether. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 9b638b7ef6..ba98ef2259 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -214,6 +214,10 @@ qemuBlockJobEmitEvents(virQEMUDriverPtr driver, virObjectEventPtr event = NULL; virObjectEventPtr event2 = NULL; + if (type >= VIR_DOMAIN_BLOCK_JOB_TYPE_LAST || + status >= VIR_DOMAIN_BLOCK_JOB_LAST) + return; + if (virStorageSourceIsLocalStorage(disk->src) && !virStorageSourceIsEmpty(disk->src)) { event = virDomainEventBlockJobNewFromObj(vm, virDomainDiskGetSource(disk), -- 2.20.1
Switching a block job to some states (e.g. QEMU_BLOCKJOB_STATE_READY) might not require a job, thus if it will become ready asynchronously we should not overwrite the state any more. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index ba98ef2259..1b7a372434 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -149,7 +149,8 @@ qemuBlockJobDiskGetJob(virDomainDiskDefPtr disk) void qemuBlockJobStarted(qemuBlockJobDataPtr job) { - job->state = QEMU_BLOCKJOB_STATE_RUNNING; + if (job->state == QEMU_BLOCKJOB_STATE_NEW) + job->state = QEMU_BLOCKJOB_STATE_RUNNING; } -- 2.20.1
While this should not be necessary as we clear it in the event handler, let's be sure and clear it prior to starting the job. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 90319261ff..d8f667e9aa 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17854,6 +17854,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, if (!(job = qemuBlockJobDiskNew(disk, QEMU_BLOCKJOB_TYPE_COPY, device))) goto endjob; + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; + /* Actually start the mirroring */ qemuDomainObjEnterMonitor(driver, vm); /* qemuMonitorDriveMirror needs to honor the REUSE_EXT flag as specified -- 2.20.1
disk->mirror would not be cleared while the local pointer was freed in qemuDomainBlockCommit if qemuDomainObjExitMonitor or qemuBlockJobDiskNew would return a failure. Since block job handling is executed in the separate handler which needs a qemu job, we don't need to pre-set the mirror state prior to starting the job. Similarly the block copy job does not do that. Move the setting of the data after starting the job so that we avoid this problem. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d8f667e9aa..5dd18954b9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18182,6 +18182,8 @@ qemuDomainBlockCommit(virDomainPtr dom, disk->dst); goto endjob; } + + jobtype = QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT; } else if (flags & VIR_DOMAIN_BLOCK_COMMIT_ACTIVE) { virReportError(VIR_ERR_INVALID_ARG, _("active commit requested but '%s' is not active"), @@ -18254,22 +18256,16 @@ qemuDomainBlockCommit(virDomainPtr dom, qemuDomainDiskChainElementPrepare(driver, vm, top_parent, false, false) < 0)) goto endjob; + if (!(job = qemuBlockJobDiskNew(disk, jobtype, device))) + goto endjob; + + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; + /* Start the commit operation. Pass the user's original spelling, * if any, through to qemu, since qemu may behave differently * depending on whether the input was specified as relative or * absolute (that is, our absolute top_canon may do the wrong - * thing if the user specified a relative name). Be prepared for - * a ready event to occur while locks are dropped. */ - if (mirror) { - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; - disk->mirror = mirror; - disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT; - jobtype = QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT; - } - - if (!(job = qemuBlockJobDiskNew(disk, jobtype, device))) - goto endjob; - + * thing if the user specified a relative name). */ qemuDomainObjEnterMonitor(driver, vm); basePath = qemuMonitorDiskNameLookup(priv->mon, device, disk->src, baseSource); @@ -18279,17 +18275,15 @@ qemuDomainBlockCommit(virDomainPtr dom, ret = qemuMonitorBlockCommit(priv->mon, device, topPath, basePath, backingPath, speed); - if (qemuDomainObjExitMonitor(driver, vm) < 0) { + if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0) { ret = -1; goto endjob; } - if (ret == 0) { - qemuBlockJobStarted(job); - mirror = NULL; - } else { - disk->mirror = NULL; - disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; + qemuBlockJobStarted(job); + if (mirror) { + VIR_STEAL_PTR(disk->mirror, mirror); + disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT; } if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) -- 2.20.1
On 1/24/19 4:58 AM, Peter Krempa wrote:
Fix the generated documentation and clean up a few corner cases in qemu's block job handling.
Peter Krempa (5): lib: Fix docs generated for enum virDomainBlockJobType qemu: blockjob: Make sure that internal states are not reported as event qemu: blockjob: Mark job as started only when it's new qemu: Clear block copy mirror state explicitly qemu: Don't double-free disk->mirror if block commit initialization fails
include/libvirt/libvirt-domain.h | 3 ++- src/qemu/qemu_blockjob.c | 7 ++++++- src/qemu/qemu_driver.c | 34 ++++++++++++++------------------ 3 files changed, 23 insertions(+), 21 deletions(-)
NIT, w/r/t patch2 - maybe a code comment regarding internal states would help someone that trips across that code and wonders what the check is for. yes it's in the commit message, but not everyone goes back to the commit that added the check to see why it was done that way. Reviewed-by: John Ferlan <jferlan@redhat.com> (series) John
participants (2)
-
John Ferlan -
Peter Krempa