On Mon, Apr 13, 2015 at 21:08:27 -0400, John Ferlan wrote:
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?
The mirror state was changed to VIR_DOMAIN_DISK_MIRROR_STATE_ABORT prior to the monitor call. If the call fails, we did not issue the cancel and thus need to revert to a safe state for a potential new operation.
ACK - in general - just making sure something wasn't missed.
I've pushed the series, thanks.
John
Peter