On Wed, 1 Apr 2015, 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(-)
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;
- }
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) {
+ if (qemuDomainBlockPivot(driver, vm, device, disk) < 0)
goto endjob;
- goto waitjob;
- }
This still needs to assign to ret here, otherwise we won't return 0 on
success.
- 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) {
- /* 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 ((pivot && disk->blockJobStatus != VIR_DOMAIN_BLOCK_JOB_COMPLETED) ||
+ (!pivot && disk->blockJobStatus != VIR_DOMAIN_BLOCK_JOB_CANCELED)) {
I'm wondering whether it is simpler to examine disk->mirrorState here
rather than disk->blockJobStatus. The second test doesn't look right: QEMU
signals COMPLETED if a drive mirror is canceled after it's fully synced.
+ 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;
}
- Michael
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list