[libvirt] [PATCH 11/11] qemu: Refactor qemuDomainBlockJobAbort()
John Ferlan
jferlan at redhat.com
Wed Apr 8 19:07:53 UTC 2015
On 04/01/2015 01:20 PM, Peter Krempa wrote:
> Change few variable names and refactor the code flow. As an additional
> bonus the function now fails if the event state is not as expected.
> ---
> src/qemu/qemu_driver.c | 108 ++++++++++++++++++++++++-------------------------
> 1 file changed, 52 insertions(+), 56 deletions(-)
>
This didn't 'git am -3' cleanly with 10/11 v2, but did with 10/11 - I
assume that's already handled in your tree..
Reusing 'async' got confusing...
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index e9431ad..ee5bf8b 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -16279,13 +16279,13 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
> {
> virQEMUDriverPtr driver = dom->conn->privateData;
> char *device = NULL;
> - virObjectEventPtr event = NULL;
> - virObjectEventPtr event2 = NULL;
> virDomainDiskDefPtr disk;
> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> bool save = false;
> int idx;
> - bool async;
> + bool modern;
> + bool pivot = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT);
> + bool async = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC);
> virDomainObjPtr vm;
> int ret = -1;
>
> @@ -16298,7 +16298,7 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
> if (virDomainBlockJobAbortEnsureACL(dom->conn, vm->def) < 0)
> goto cleanup;
>
> - if (qemuDomainSupportsBlockJobs(vm, &async) < 0)
> + if (qemuDomainSupportsBlockJobs(vm, &modern) < 0)
> goto cleanup;
>
> if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> @@ -16314,19 +16314,14 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
> goto endjob;
> disk = vm->def->disks[idx];
>
> - if (async && !(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC)) {
> - /* prepare state for event delivery */
> + if (modern && !async) {
> + /* prepare state for event delivery. Since qemuDomainBlockPivot is
> + * synchronous, but the event is delivered asynchronously we need to
> + * wait too */
> disk->blockJobStatus = -1;
> disk->blockJobSync = true;
> }
>
> - if ((flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT) &&
> - !(async && disk->mirror)) {
> - virReportError(VIR_ERR_OPERATION_INVALID,
> - _("pivot of disk '%s' requires an active copy job"),
> - disk->dst);
> - goto endjob;
> - }
Thinking in terms of the change of 'async' to 'modern'...
if (pivot && !(modern && disk->mirror) is no longer necessary?
I didn't get that from the commit message and just want to be sure due
to the "reuse" of async...
Ahh... I see (I think) this check and error is in qemuDomainBlockPivot,
but even that's missing the check for 'modern'.
> if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_NONE &&
> disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> @@ -16335,29 +16330,29 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
> goto endjob;
> }
>
> - if (disk->mirror && (flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT)) {
> - ret = qemuDomainBlockPivot(driver, vm, device, disk);
> - if (ret < 0 && async)
> + if (pivot) {
Not checking for disk->mirror seems to be a theme, but I see
qemuDomainBlockPivot does the check (and hence the aha moment described
just above).
> + if (qemuDomainBlockPivot(driver, vm, device, disk) < 0)
This used to have a condition of "if (ret < 0 && async)", where async is
now known as modern.
Since qemuDomainBlockPivot is synchronous (another new comment), we
don't need to check modern (or the old this qemu supports
QEMU_CAPS_BLOCKJOB_ASYNC), so in reality modern doesn't matter, so OK I
get this adjustment...
> goto endjob;
> - goto waitjob;
> - }
> - if (disk->mirror) {
> - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_ABORT;
> - save = true;
> - }
> + } else {
> + if (disk->mirror) {
> + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_ABORT;
> + save = true;
> + }
>
> - qemuDomainObjEnterMonitor(driver, vm);
> - ret = qemuMonitorBlockJobCancel(qemuDomainGetMonitor(vm), device, async);
> - if (qemuDomainObjExitMonitor(driver, vm) < 0)
> - ret = -1;
> + qemuDomainObjEnterMonitor(driver, vm);
> + ret = qemuMonitorBlockJobCancel(qemuDomainGetMonitor(vm), device, modern);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0) {
> + ret = -1;
> + goto endjob;
> + }
>
> - if (ret < 0) {
> - if (disk->mirror)
> - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
> - goto endjob;
> + if (ret < 0) {
> + if (disk->mirror)
> + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE;
> + goto endjob;
> + }
> }
>
> - waitjob:
> /* If we have made changes to XML due to a copy job, make a best
> * effort to save it now. But we can ignore failure, since there
> * will be further changes when the event marks completion. */
> @@ -16372,33 +16367,38 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
> * while still holding the VM job, to prevent newly scheduled
> * block jobs from confusing us. */
> if (!async) {
This one (non) change of 'async' to 'modern' still has me wondering a
bit... Especially since
So previously "async" was what is now modern, so does this reference
need to change as well?
Now 'async' means 'flags' provided ASYNC compared to previously where it
meant the capability supports async.
The comment before this check describes the synch vs. asynch, but now
there's no else (async)...
Something just doesn't feel right.
> - /* Older qemu that lacked async reporting also lacked
> - * blockcopy and active commit, so we can hardcode the
> - * event to pull, and we know the XML doesn't need
> - * updating. We have to generate two event variants. */
> - int type = VIR_DOMAIN_BLOCK_JOB_TYPE_PULL;
> - int status = VIR_DOMAIN_BLOCK_JOB_CANCELED;
> - event = virDomainEventBlockJobNewFromObj(vm, disk->src->path, type,
> - status);
> - event2 = virDomainEventBlockJob2NewFromObj(vm, disk->dst, type,
> - status);
> - } else if (disk->blockJobSync) {
> - /* XXX If the event reports failure, we should reflect
> - * that back into the return status of this API call. */
> -
> - while (disk->blockJobStatus == -1 && disk->blockJobSync) {
> - if (virCondWait(&disk->blockJobSyncCond, &vm->parent.lock) < 0) {
> - virReportSystemError(errno, "%s",
> - _("Unable to wait on block job sync "
> - "condition"));
> - disk->blockJobSync = false;
> - goto endjob;
> + if (!modern) {
> + /* Older qemu that lacked async reporting also lacked
> + * blockcopy and active commit, so we can hardcode the
> + * event to pull and let qemuBlockJobEventProcess() handle
> + * the rest as usual */
> + disk->blockJobType = VIR_DOMAIN_BLOCK_JOB_TYPE_PULL;
> + disk->blockJobStatus = VIR_DOMAIN_BLOCK_JOB_CANCELED;
> + } else {
This else used to be off the old (!async) (or now modern)...
John
> + while (disk->blockJobStatus == -1 && disk->blockJobSync) {
> + if (virCondWait(&disk->blockJobSyncCond, &vm->parent.lock) < 0) {
> + virReportSystemError(errno, "%s",
> + _("Unable to wait on block job sync "
> + "condition"));
> + disk->blockJobSync = false;
> + goto endjob;
> + }
> }
> }
>
> qemuBlockJobEventProcess(driver, vm, disk,
> disk->blockJobType,
> disk->blockJobStatus);
> +
> + /* adjust the return code */
> + if ((pivot && disk->blockJobStatus != VIR_DOMAIN_BLOCK_JOB_COMPLETED) ||
> + (!pivot && disk->blockJobStatus != VIR_DOMAIN_BLOCK_JOB_CANCELED)) {
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("failed to terminate block job on disk '%s'"),
> + disk->dst);
> + ret = -1;
> + }
> +
> disk->blockJobSync = false;
> }
>
> @@ -16409,10 +16409,6 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
> virObjectUnref(cfg);
> VIR_FREE(device);
> qemuDomObjEndAPI(&vm);
> - if (event)
> - qemuDomainEventQueue(driver, event);
> - if (event2)
> - qemuDomainEventQueue(driver, event2);
> return ret;
> }
>
More information about the libvir-list
mailing list