On 04/09/2015 12:45 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.
> ---
>
> Notes:
> Version 3:
> - fixed error reporting code and success code propagation from pivot
>
> src/qemu/qemu_driver.c | 107 +++++++++++++++++++++++--------------------------
> 1 file changed, 51 insertions(+), 56 deletions(-)
>
ACK 1-6...
Just one question below...
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index eebed55..8d4aa97 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -16273,13 +16273,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;
>
> @@ -16292,7 +16292,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)
> @@ -16308,19 +16308,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;
> - }
> if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_NONE &&
> disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY) {
> virReportError(VIR_ERR_OPERATION_INVALID,
> @@ -16329,31 +16324,31 @@ 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) {
> + if ((ret = qemuDomainBlockPivot(driver, vm, device, disk)) < 0) {
> disk->blockJobSync = false;
> 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;
> + }
should this just fall through now? Asked differently - should
disk->mirrorState change before goto endjob if ExitMonitor fails?
Would "if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0)" do the
trick?
ACK - in general - just making sure something wasn't missed.
John
>
> - 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. */
> @@ -16368,33 +16363,37 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
> * while still holding the VM job, to prevent newly scheduled
> * block jobs from confusing us. */
> if (!async) {
> - /* 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 {
> + 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 we've got an explicit failure */
> + if (disk->blockJobStatus == VIR_DOMAIN_BLOCK_JOB_FAILED) {
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("failed to terminate block job on disk '%s'"),
> + disk->dst);
> + ret = -1;
> + }
> +
> disk->blockJobSync = false;
> }
>
> @@ -16405,10 +16404,6 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
> virObjectUnref(cfg);
> VIR_FREE(device);
> qemuDomObjEndAPI(&vm);
> - if (event)
> - qemuDomainEventQueue(driver, event);
> - if (event2)
> - qemuDomainEventQueue(driver, event2);
> return ret;
> }
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list