Re: [libvirt] [PATCH 11/11] qemu: Refactor qemuDomainBlockJobAbort()
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
Re: [libvirt] [PATCH 11/11] qemu: Refactor qemuDomainBlockJobAbort()
On Thu, Apr 09, 2015 at 20:22:43 +1000, Michael Chapman wrote:
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.
Indeed, I'm actually puzzled that it worked when I was testing it and I
don't remember whether I've done changes and forgot to push them or just
forgot to restart the daemon after compiling.
-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 =
Re: [libvirt] [PATCH 11/11] qemu: Refactor qemuDomainBlockJobAbort()
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
[libvirt] [PATCH 11/11] qemu: Refactor qemuDomainBlockJobAbort()
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;
-}
-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) {
-
