Re: [libvirt] [PATCHv3 7/7] qemu: Refactor qemuDomainBlockJobAbort()

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