[libvirt] [PATCH 00/11] qemu: Refactor the block job code
At the expense of adding 113 lines of code, kill the ugly qemuBlockJobImpl method and spread it's guts into separate functions. This series additionally fixes a issue with failed drive pivot and the abort function now returns errors if the returned event contained failure. Peter Krempa (11): qemu: monitor: Extract handling of JSON block job error codes qemu: domain: Introduce helper to retrieve domain monitor object qemu: domain: Add helper to check block job support qemu: blockjob: Use the new helpers in qemuDomainGetBlockJobInfo qemu: blockjob: Split qemuDomainBlockJobSetSpeed from qemuDomainBlockJobImpl qemu: Clean up old leftovers in qemuMonitorDrivePivot qemu: blockPivot: Don't pause the VM any more since we don't use drive-reopen qemu: blockjob: Separate qemuDomainBlockJobAbort from qemuDomainBlockJobImpl qemu: blockPull: Refactor the rest of qemuDomainBlockJobImpl qemu: drivePivot: Fix assumption when 'block-job-complete' fails qemu: Refactor qemuDomainBlockJobAbort() src/qemu/qemu_domain.c | 43 +++++ src/qemu/qemu_domain.h | 4 + src/qemu/qemu_driver.c | 437 +++++++++++++++++++++---------------------- src/qemu/qemu_migration.c | 8 +- src/qemu/qemu_monitor.c | 88 ++++++--- src/qemu/qemu_monitor.h | 35 ++-- src/qemu/qemu_monitor_json.c | 188 +++++++++++-------- src/qemu/qemu_monitor_json.h | 28 ++- tests/qemumonitorjsontest.c | 2 +- 9 files changed, 473 insertions(+), 360 deletions(-) -- 2.2.2
My intention is to split qemuMonitorJSONBlockJob() into simpler separate functions for every block job type. Since the error handling code is the same for all block jobs, this patch extracts the code into a separate function that will later be reused in more places. --- src/qemu/qemu_monitor_json.c | 64 +++++++++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index aa844ca..edd0a3f 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4252,6 +4252,39 @@ qemuMonitorJSONBlockJobInfo(qemuMonitorPtr mon, } +static int +qemuMonitorJSONBlockJobError(virJSONValuePtr reply, + const char *cmd_name, + const char *device) +{ + if (!virJSONValueObjectHasKey(reply, "error")) + return 0; + + if (qemuMonitorJSONHasError(reply, "DeviceNotActive")) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("No active operation on device: %s"), device); + } else if (qemuMonitorJSONHasError(reply, "DeviceInUse")) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Device %s in use"), device); + } else if (qemuMonitorJSONHasError(reply, "NotSupported")) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Operation is not supported for device: %s"), device); + } else if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Command '%s' is not found"), cmd_name); + } else { + virJSONValuePtr error = virJSONValueObjectGet(reply, "error"); + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected error: (%s) '%s'"), + NULLSTR(virJSONValueObjectGetString(error, "class")), + NULLSTR(virJSONValueObjectGetString(error, "desc"))); + } + + return -1; +} + + /* speed is in bytes/sec */ int qemuMonitorJSONBlockJob(qemuMonitorPtr mon, @@ -4322,34 +4355,15 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon, if (!cmd) return -1; - ret = qemuMonitorJSONCommand(mon, cmd, &reply); + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; - if (ret == 0 && virJSONValueObjectHasKey(reply, "error")) { - ret = -1; - if (qemuMonitorJSONHasError(reply, "DeviceNotActive")) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("No active operation on device: %s"), - device); - } else if (qemuMonitorJSONHasError(reply, "DeviceInUse")) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("Device %s in use"), device); - } else if (qemuMonitorJSONHasError(reply, "NotSupported")) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("Operation is not supported for device: %s"), - device); - } else if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("Command '%s' is not found"), cmd_name); - } else { - virJSONValuePtr error = virJSONValueObjectGet(reply, "error"); + if (qemuMonitorJSONBlockJobError(reply, cmd_name, device) < 0) + goto cleanup; - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unexpected error: (%s) '%s'"), - NULLSTR(virJSONValueObjectGetString(error, "class")), - NULLSTR(virJSONValueObjectGetString(error, "desc"))); - } - } + ret = 0; + cleanup: virJSONValueFree(cmd); virJSONValueFree(reply); return ret; -- 2.2.2
On 04/01/2015 01:20 PM, Peter Krempa wrote:
My intention is to split qemuMonitorJSONBlockJob() into simpler separate functions for every block job type. Since the error handling code is the same for all block jobs, this patch extracts the code into a separate function that will later be reused in more places. --- src/qemu/qemu_monitor_json.c | 64 +++++++++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 25 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index aa844ca..edd0a3f 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4252,6 +4252,39 @@ qemuMonitorJSONBlockJobInfo(qemuMonitorPtr mon, }
+static int +qemuMonitorJSONBlockJobError(virJSONValuePtr reply, + const char *cmd_name, + const char *device) +{ + if (!virJSONValueObjectHasKey(reply, "error")) + return 0; + + if (qemuMonitorJSONHasError(reply, "DeviceNotActive")) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("No active operation on device: %s"), device); + } else if (qemuMonitorJSONHasError(reply, "DeviceInUse")) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Device %s in use"), device); + } else if (qemuMonitorJSONHasError(reply, "NotSupported")) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Operation is not supported for device: %s"), device); + } else if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Command '%s' is not found"), cmd_name); + } else { + virJSONValuePtr error = virJSONValueObjectGet(reply, "error");
Because you touched it - Coverity whines that 'error' is not checked for NULL: (8) Event returned_null: "virJSONValueObjectGet" returns null (checked 96 out of 99 times). [details] (16) Event var_assigned: Assigning: "error" = null return value from "virJSONValueObjectGet". Also see events: (17) Event dereference: Dereferencing a pointer that might be null "error" when calling "virJSONValueObjectGetString". [details] There's many examples (96) of checking... ACK with the check John
+ + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected error: (%s) '%s'"), + NULLSTR(virJSONValueObjectGetString(error, "class")), + NULLSTR(virJSONValueObjectGetString(error, "desc"))); + } + + return -1; +} + + /* speed is in bytes/sec */ int qemuMonitorJSONBlockJob(qemuMonitorPtr mon, @@ -4322,34 +4355,15 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon, if (!cmd) return -1;
- ret = qemuMonitorJSONCommand(mon, cmd, &reply); + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup;
- if (ret == 0 && virJSONValueObjectHasKey(reply, "error")) { - ret = -1; - if (qemuMonitorJSONHasError(reply, "DeviceNotActive")) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("No active operation on device: %s"), - device); - } else if (qemuMonitorJSONHasError(reply, "DeviceInUse")) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("Device %s in use"), device); - } else if (qemuMonitorJSONHasError(reply, "NotSupported")) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("Operation is not supported for device: %s"), - device); - } else if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("Command '%s' is not found"), cmd_name); - } else { - virJSONValuePtr error = virJSONValueObjectGet(reply, "error"); + if (qemuMonitorJSONBlockJobError(reply, cmd_name, device) < 0) + goto cleanup;
- virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unexpected error: (%s) '%s'"), - NULLSTR(virJSONValueObjectGetString(error, "class")), - NULLSTR(virJSONValueObjectGetString(error, "desc"))); - } - } + ret = 0;
+ cleanup: virJSONValueFree(cmd); virJSONValueFree(reply); return ret;
On Wed, Apr 08, 2015 at 10:25:36 -0400, John Ferlan wrote:
On 04/01/2015 01:20 PM, Peter Krempa wrote:
My intention is to split qemuMonitorJSONBlockJob() into simpler separate functions for every block job type. Since the error handling code is the same for all block jobs, this patch extracts the code into a separate function that will later be reused in more places. --- src/qemu/qemu_monitor_json.c | 64 +++++++++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 25 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index aa844ca..edd0a3f 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4252,6 +4252,39 @@ qemuMonitorJSONBlockJobInfo(qemuMonitorPtr mon, }
+static int +qemuMonitorJSONBlockJobError(virJSONValuePtr reply, + const char *cmd_name, + const char *device) +{ + if (!virJSONValueObjectHasKey(reply, "error")) + return 0; + + if (qemuMonitorJSONHasError(reply, "DeviceNotActive")) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("No active operation on device: %s"), device); + } else if (qemuMonitorJSONHasError(reply, "DeviceInUse")) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Device %s in use"), device); + } else if (qemuMonitorJSONHasError(reply, "NotSupported")) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Operation is not supported for device: %s"), device); + } else if (qemuMonitorJSONHasError(reply, "CommandNotFound")) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Command '%s' is not found"), cmd_name); + } else { + virJSONValuePtr error = virJSONValueObjectGet(reply, "error");
Because you touched it - Coverity whines that 'error' is not checked for NULL:
(8) Event returned_null: "virJSONValueObjectGet" returns null (checked 96 out of 99 times). [details] (16) Event var_assigned: Assigning: "error" = null return value from "virJSONValueObjectGet". Also see events: (17) Event dereference: Dereferencing a pointer that might be null "error" when calling "virJSONValueObjectGetString". [details]
I hate coverity for this. That's a false positive due to the fact that we check right away that the reply object contains the error subobject.
There's many examples (96) of checking...
Many examples, but here it's useless. The check will make it ugly.
ACK with the check
I'll send a new version that will have different control flow. If we are going to uglyfy this function for coverity, we might as well as fix it so that the error object is not looked up several times which will then make the coverity error disappear. The downside will be that it will not be straight move of code. Peter
On 04/09/2015 05:26 AM, Peter Krempa wrote:
Because you touched it - Coverity whines that 'error' is not checked for NULL:
(8) Event returned_null: "virJSONValueObjectGet" returns null (checked 96 out of 99 times). [details] (16) Event var_assigned: Assigning: "error" = null return value from "virJSONValueObjectGet". Also see events: (17) Event dereference: Dereferencing a pointer that might be null "error" when calling "virJSONValueObjectGetString". [details]
I hate coverity for this. That's a false positive due to the fact that we check right away that the reply object contains the error subobject.
How about a "sa_assert(event);" Coverity is only complaining because so many places check "if (error) ? xxx : yyy" And yes it does work, I just checked John
There's many examples (96) of checking...
Many examples, but here it's useless. The check will make it ugly.
ACK with the check
I'll send a new version that will have different control flow. If we are going to uglyfy this function for coverity, we might as well as fix it so that the error object is not looked up several times which will then make the coverity error disappear.
The downside will be that it will not be straight move of code.
Peter
On Thu, Apr 09, 2015 at 05:31:08 -0400, John Ferlan wrote:
On 04/09/2015 05:26 AM, Peter Krempa wrote:
Because you touched it - Coverity whines that 'error' is not checked for NULL:
(8) Event returned_null: "virJSONValueObjectGet" returns null (checked 96 out of 99 times). [details] (16) Event var_assigned: Assigning: "error" = null return value from "virJSONValueObjectGet". Also see events: (17) Event dereference: Dereferencing a pointer that might be null "error" when calling "virJSONValueObjectGetString". [details]
I hate coverity for this. That's a false positive due to the fact that we check right away that the reply object contains the error subobject.
How about a "sa_assert(event);"
I hate those to be honest.
Coverity is only complaining because so many places check "if (error) ? xxx : yyy"
And yes it does work, I just checked
John
I've refactored a few other places, they should work nice together and get rid of the error. I'll post v2 of the patches that can't be pushed separately after I finish rebasing. Peter
In some cases where the function does not need to access the private data this helper may be used to retrieve the monitor object. --- src/qemu/qemu_domain.c | 13 +++++++++++++ src/qemu/qemu_domain.h | 2 ++ 2 files changed, 15 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 758fcd9..707ef8b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3025,3 +3025,16 @@ qemuDomainMemoryDeviceAlignSize(virDomainMemoryDefPtr mem) { mem->size = VIR_ROUND_UP(mem->size, 1024); } + + +/** + * qemuDomainGetMonitor: + * @vm: domain object + * + * Returns the monitor pointer corresponding to the domain object @vm. + */ +qemuMonitorPtr +qemuDomainGetMonitor(virDomainObjPtr vm) +{ + return ((qemuDomainObjPrivatePtr) vm->privateData)->mon; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index b854b54..33dac39 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -248,6 +248,8 @@ void qemuDomainObjDiscardAsyncJob(virQEMUDriverPtr driver, virDomainObjPtr obj); void qemuDomainObjReleaseAsyncJob(virDomainObjPtr obj); +qemuMonitorPtr qemuDomainGetMonitor(virDomainObjPtr vm) + ATTRIBUTE_NONNULL(1); void qemuDomainObjEnterMonitor(virQEMUDriverPtr driver, virDomainObjPtr obj) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -- 2.2.2
On 04/01/2015 01:20 PM, Peter Krempa wrote:
In some cases where the function does not need to access the private data this helper may be used to retrieve the monitor object. --- src/qemu/qemu_domain.c | 13 +++++++++++++ src/qemu/qemu_domain.h | 2 ++ 2 files changed, 15 insertions(+)
ACK John
We need to check that qemu supports block jobs in multiple places. Add a helper to do the check. --- src/qemu/qemu_domain.c | 30 ++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 2 ++ 2 files changed, 32 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 707ef8b..3fb497f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3038,3 +3038,33 @@ qemuDomainGetMonitor(virDomainObjPtr vm) { return ((qemuDomainObjPrivatePtr) vm->privateData)->mon; } + + +/** + * qemuDomainSupportsBlockJobs: + * @vm: domain object + * @modern: pointer to bool that returns whether modern block jobs are supported + * + * Returns -1 in case when qemu does not support block jobs at all. Otherwise + * returns 0 and optionally fills @modern to denote that modern (async) block + * jobs are supported. + */ +int +qemuDomainSupportsBlockJobs(virDomainObjPtr vm, + bool *modern) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + bool async = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC); + bool sync = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_SYNC); + + if (!sync && !async) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("block jobs not supported with this QEMU binary")); + return -1; + } + + if (modern) + *modern = async; + + return 0; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 33dac39..ec76e91 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -424,6 +424,8 @@ int qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); +int qemuDomainSupportsBlockJobs(virDomainObjPtr vm, bool *modern) + ATTRIBUTE_NONNULL(1); bool qemuDomainDiskBlockJobIsActive(virDomainDiskDefPtr disk); void qemuDomObjEndAPI(virDomainObjPtr *vm); -- 2.2.2
On 04/01/2015 01:20 PM, Peter Krempa wrote:
We need to check that qemu supports block jobs in multiple places. Add a helper to do the check. --- src/qemu/qemu_domain.c | 30 ++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 2 ++ 2 files changed, 32 insertions(+)
ACK John
Refactor the function to use the new helpers. --- src/qemu/qemu_driver.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index becf415..6a2b58d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16457,7 +16457,6 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path, virDomainBlockJobInfoPtr info, unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; - qemuDomainObjPrivatePtr priv; virDomainObjPtr vm; char *device = NULL; int idx; @@ -16470,18 +16469,11 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path, if (!(vm = qemuDomObjFromDomain(dom))) return -1; - if (virDomainGetBlockJobInfoEnsureACL(dom->conn, vm->def) < 0) { - qemuDomObjEndAPI(&vm); - return -1; - } + if (virDomainGetBlockJobInfoEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; - priv = vm->privateData; - if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC) && - !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_SYNC)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("block jobs not supported with this QEMU binary")); + if (qemuDomainSupportsBlockJobs(vm, NULL) < 0) goto cleanup; - } if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) goto cleanup; @@ -16492,13 +16484,13 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path, goto endjob; } - device = qemuDiskPathToAlias(vm, path, &idx); - if (!device) + if (!(device = qemuDiskPathToAlias(vm, path, &idx))) goto endjob; disk = vm->def->disks[idx]; qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorBlockJobInfo(priv->mon, device, info, &bandwidth); + ret = qemuMonitorBlockJobInfo(qemuDomainGetMonitor(vm), device, info, + &bandwidth); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; if (ret < 0) -- 2.2.2
On 04/01/2015 01:20 PM, Peter Krempa wrote:
Refactor the function to use the new helpers. --- src/qemu/qemu_driver.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-)
It seems git am -3 is having issues for this patch with recent commit 'cfcdf5ff0'
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index becf415..6a2b58d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16457,7 +16457,6 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path, virDomainBlockJobInfoPtr info, unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; - qemuDomainObjPrivatePtr priv; virDomainObjPtr vm; char *device = NULL; int idx; @@ -16470,18 +16469,11 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path, if (!(vm = qemuDomObjFromDomain(dom))) return -1;
- if (virDomainGetBlockJobInfoEnsureACL(dom->conn, vm->def) < 0) { - qemuDomObjEndAPI(&vm); - return -1; - } + if (virDomainGetBlockJobInfoEnsureACL(dom->conn, vm->def) < 0) + goto cleanup;
- priv = vm->privateData; - if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC) && - !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_SYNC)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("block jobs not supported with this QEMU binary")); + if (qemuDomainSupportsBlockJobs(vm, NULL) < 0)
Commit id 'cfcdf5ff0' moves this check to after BeginJob and of course goto endjob... ACK - with the adjustment John
goto cleanup; - }
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) goto cleanup; @@ -16492,13 +16484,13 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path, goto endjob; }
- device = qemuDiskPathToAlias(vm, path, &idx); - if (!device) + if (!(device = qemuDiskPathToAlias(vm, path, &idx))) goto endjob; disk = vm->def->disks[idx];
qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorBlockJobInfo(priv->mon, device, info, &bandwidth); + ret = qemuMonitorBlockJobInfo(qemuDomainGetMonitor(vm), device, info, + &bandwidth); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; if (ret < 0)
qemuDomainBlockJobImpl become an unmaintainable mess over the years of adding new stuff to it. This patch starts splitting up individual functions from it until it can be killed entirely. In bulk this will add lines of code rather than delete them but it will be traded for maintainability. --- src/qemu/qemu_driver.c | 66 ++++++++++++++++++++++++++++++++++++-------- src/qemu/qemu_monitor.c | 19 +++++++++++++ src/qemu/qemu_monitor.h | 6 +++- src/qemu/qemu_monitor_json.c | 41 +++++++++++++++++++++------ src/qemu/qemu_monitor_json.h | 6 ++++ 5 files changed, 118 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6a2b58d..eec5457 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16339,10 +16339,8 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, } /* Convert bandwidth MiB to bytes, if needed */ - if ((mode == BLOCK_JOB_SPEED && - !(flags & VIR_DOMAIN_BLOCK_JOB_SPEED_BANDWIDTH_BYTES)) || - (mode == BLOCK_JOB_PULL && - !(flags & VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES))) { + if (mode == BLOCK_JOB_PULL && + !(flags & VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES)) { if (speed > LLONG_MAX >> 20) { virReportError(VIR_ERR_OVERFLOW, _("bandwidth must be less than %llu"), @@ -16532,23 +16530,69 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path, return ret; } + static int -qemuDomainBlockJobSetSpeed(virDomainPtr dom, const char *path, - unsigned long bandwidth, unsigned int flags) +qemuDomainBlockJobSetSpeed(virDomainPtr dom, + const char *path, + unsigned long bandwidth, + unsigned int flags) { + virQEMUDriverPtr driver = dom->conn->privateData; + int ret = -1; virDomainObjPtr vm; + bool modern; + const char *device; + unsigned long long speed = bandwidth; + virCheckFlags(VIR_DOMAIN_BLOCK_JOB_SPEED_BANDWIDTH_BYTES, -1); + /* Convert bandwidth MiB to bytes, if needed */ + if (!(flags & VIR_DOMAIN_BLOCK_JOB_SPEED_BANDWIDTH_BYTES)) { + if (speed > LLONG_MAX >> 20) { + virReportError(VIR_ERR_OVERFLOW, + _("bandwidth must be less than %llu"), + LLONG_MAX >> 20); + return -1; + } + speed <<= 20; + } + if (!(vm = qemuDomObjFromDomain(dom))) return -1; - if (virDomainBlockJobSetSpeedEnsureACL(dom->conn, vm->def) < 0) { - qemuDomObjEndAPI(&vm); - return -1; + if (virDomainBlockJobSetSpeedEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (qemuDomainSupportsBlockJobs(vm, &modern) < 0) + goto cleanup; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto endjob; } - return qemuDomainBlockJobImpl(vm, dom->conn, path, NULL, bandwidth, - BLOCK_JOB_SPEED, flags); + if (!(device = qemuDiskPathToAlias(vm, path, NULL))) + goto endjob; + + qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorBlockJobSetSpeed(qemuDomainGetMonitor(vm), + device, + speed, + modern); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + ret = -1; + + endjob: + qemuDomainObjEndJob(driver, vm); + + cleanup: + qemuDomObjEndAPI(&vm); + + return ret; } diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 5da0b8f..aae259f 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3606,6 +3606,25 @@ qemuMonitorBlockJob(qemuMonitorPtr mon, int +qemuMonitorBlockJobSetSpeed(qemuMonitorPtr mon, + const char *device, + unsigned long long bandwidth, + bool modern) +{ + VIR_DEBUG("mon=%p, device=%s, bandwidth=%lluB, modern=%d", + mon, device, bandwidth, modern); + + if (!mon->json) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("block jobs require JSON monitor")); + return -1; + } + + return qemuMonitorJSONBlockJobSetSpeed(mon, device, bandwidth, modern); +} + + +int qemuMonitorBlockJobInfo(qemuMonitorPtr mon, const char *device, virDomainBlockJobInfoPtr info, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index c7c3dab..9cd0d9d 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -759,7 +759,6 @@ int qemuMonitorSendKey(qemuMonitorPtr mon, typedef enum { BLOCK_JOB_ABORT, - BLOCK_JOB_SPEED, BLOCK_JOB_PULL, } qemuMonitorBlockJobCmd; @@ -772,6 +771,11 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon, bool modern) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int qemuMonitorBlockJobSetSpeed(qemuMonitorPtr mon, + const char *device, + unsigned long long bandwidth, + bool modern); + int qemuMonitorBlockJobInfo(qemuMonitorPtr mon, const char *device, virDomainBlockJobInfoPtr info, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index edd0a3f..34a6d56 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4333,14 +4333,6 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon, NULL); break; - case BLOCK_JOB_SPEED: - cmd_name = modern ? "block-job-set-speed" : "block_job_set_speed"; - cmd = qemuMonitorJSONMakeCommand(cmd_name, - "s:device", device, - modern ? "J:speed" : "J:value", speed, - NULL); - break; - case BLOCK_JOB_PULL: cmd_name = modern ? "block-stream" : "block_stream"; cmd = qemuMonitorJSONMakeCommand(cmd_name, @@ -4369,6 +4361,39 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon, return ret; } + +int +qemuMonitorJSONBlockJobSetSpeed(qemuMonitorPtr mon, + const char *device, + unsigned long long speed, + bool modern) +{ + int ret = -1; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + const char *cmd_name = modern ? "block-job-set-speed" : "block_job_set_speed"; + + if (!(cmd = qemuMonitorJSONMakeCommand(cmd_name, + "s:device", device, + modern ? "J:speed" : "J:value", speed, + NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; + + if (qemuMonitorJSONBlockJobError(reply, cmd_name, device) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + + int qemuMonitorJSONOpenGraphics(qemuMonitorPtr mon, const char *protocol, const char *fdname, diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 1be3b98..09f898c 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -308,6 +308,12 @@ int qemuMonitorJSONBlockJob(qemuMonitorPtr mon, bool modern) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int qemuMonitorJSONBlockJobSetSpeed(qemuMonitorPtr mon, + const char *device, + unsigned long long speed, + bool modern) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + int qemuMonitorJSONBlockJobInfo(qemuMonitorPtr mon, const char *device, virDomainBlockJobInfoPtr info, -- 2.2.2
On 04/01/2015 01:20 PM, Peter Krempa wrote:
qemuDomainBlockJobImpl become an unmaintainable mess over the years of adding new stuff to it. This patch starts splitting up individual functions from it until it can be killed entirely.
In bulk this will add lines of code rather than delete them but it will be traded for maintainability. --- src/qemu/qemu_driver.c | 66 ++++++++++++++++++++++++++++++++++++-------- src/qemu/qemu_monitor.c | 19 +++++++++++++ src/qemu/qemu_monitor.h | 6 +++- src/qemu/qemu_monitor_json.c | 41 +++++++++++++++++++++------ src/qemu/qemu_monitor_json.h | 6 ++++ 5 files changed, 118 insertions(+), 20 deletions(-)
ACK John
There are two leftover unused variables. Remove them and clean up the fallout of the change. --- src/qemu/qemu_driver.c | 5 +---- src/qemu/qemu_monitor.c | 21 +++++++++------------ src/qemu/qemu_monitor.h | 6 ++---- src/qemu/qemu_monitor_json.c | 5 ++--- src/qemu/qemu_monitor_json.h | 4 +--- tests/qemumonitorjsontest.c | 2 +- 6 files changed, 16 insertions(+), 27 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index eec5457..7c33ca3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16050,7 +16050,6 @@ qemuDomainBlockPivot(virConnectPtr conn, int ret = -1, rc; qemuDomainObjPrivatePtr priv = vm->privateData; virDomainBlockJobInfo info; - const char *format = NULL; bool resume = false; virStorageSourcePtr oldsrc = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); @@ -16062,8 +16061,6 @@ qemuDomainBlockPivot(virConnectPtr conn, goto cleanup; } - format = virStorageFileFormatTypeToString(disk->mirror->format); - /* Probe the status, if needed. */ if (!disk->mirrorState) { qemuDomainObjEnterMonitor(driver, vm); @@ -16147,7 +16144,7 @@ qemuDomainBlockPivot(virConnectPtr conn, * overall return value. */ disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT; qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorDrivePivot(priv->mon, device, disk->mirror->path, format); + ret = qemuMonitorDrivePivot(priv->mon, device); if (qemuDomainObjExitMonitor(driver, vm) < 0) { ret = -1; goto cleanup; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index aae259f..0e50dbb 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3492,23 +3492,20 @@ qemuMonitorDiskNameLookup(qemuMonitorPtr mon, } -/* Use the block-job-complete monitor command to pivot a block copy - * job. */ +/* Use the block-job-complete monitor command to pivot a block copy job. */ int -qemuMonitorDrivePivot(qemuMonitorPtr mon, const char *device, - const char *file, const char *format) +qemuMonitorDrivePivot(qemuMonitorPtr mon, + const char *device) { - int ret = -1; - - VIR_DEBUG("mon=%p, device=%s, file=%s, format=%s", - mon, device, file, NULLSTR(format)); + VIR_DEBUG("mon=%p, device=%s", mon, device); - if (mon->json) - ret = qemuMonitorJSONDrivePivot(mon, device, file, format); - else + if (mon->json) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("drive pivot requires JSON monitor")); - return ret; + return -1; + } + + return qemuMonitorJSONDrivePivot(mon, device); } int qemuMonitorArbitraryCommand(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 9cd0d9d..78f4648 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -721,10 +721,8 @@ int qemuMonitorDriveMirror(qemuMonitorPtr mon, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int qemuMonitorDrivePivot(qemuMonitorPtr mon, - const char *device, - const char *file, - const char *format) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + const char *device) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int qemuMonitorBlockCommit(qemuMonitorPtr mon, const char *device, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 34a6d56..e0f16ec 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3859,9 +3859,8 @@ qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, const char *device, } int -qemuMonitorJSONDrivePivot(qemuMonitorPtr mon, const char *device, - const char *file ATTRIBUTE_UNUSED, - const char *format ATTRIBUTE_UNUSED) +qemuMonitorJSONDrivePivot(qemuMonitorPtr mon, + const char *device) { int ret; virJSONValuePtr cmd; diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 09f898c..5185bbf 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -264,9 +264,7 @@ int qemuMonitorJSONDriveMirror(qemuMonitorPtr mon, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int qemuMonitorJSONDrivePivot(qemuMonitorPtr mon, - const char *device, - const char *file, - const char *format) + const char *device) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index f09176a..170d690 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1186,7 +1186,7 @@ GEN_TEST_FUNC(qemuMonitorJSONSetDrivePassphrase, "vda", "secret_passhprase") GEN_TEST_FUNC(qemuMonitorJSONDriveMirror, "vdb", "/foo/bar", NULL, 1024, 0, 0, VIR_DOMAIN_BLOCK_REBASE_SHALLOW | VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT) GEN_TEST_FUNC(qemuMonitorJSONBlockCommit, "vdb", "/foo/bar1", "/foo/bar2", NULL, 1024) -GEN_TEST_FUNC(qemuMonitorJSONDrivePivot, "vdb", NULL, NULL) +GEN_TEST_FUNC(qemuMonitorJSONDrivePivot, "vdb") GEN_TEST_FUNC(qemuMonitorJSONScreendump, "/foo/bar") GEN_TEST_FUNC(qemuMonitorJSONOpenGraphics, "spice", "spicefd", false) GEN_TEST_FUNC(qemuMonitorJSONNBDServerStart, "localhost", 12345) -- 2.2.2
On 04/01/2015 01:20 PM, Peter Krempa wrote:
There are two leftover unused variables. Remove them and clean up the fallout of the change. --- src/qemu/qemu_driver.c | 5 +---- src/qemu/qemu_monitor.c | 21 +++++++++------------ src/qemu/qemu_monitor.h | 6 ++---- src/qemu/qemu_monitor_json.c | 5 ++--- src/qemu/qemu_monitor_json.h | 4 +--- tests/qemumonitorjsontest.c | 2 +- 6 files changed, 16 insertions(+), 27 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index eec5457..7c33ca3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16050,7 +16050,6 @@ qemuDomainBlockPivot(virConnectPtr conn, int ret = -1, rc; qemuDomainObjPrivatePtr priv = vm->privateData; virDomainBlockJobInfo info; - const char *format = NULL; bool resume = false; virStorageSourcePtr oldsrc = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); @@ -16062,8 +16061,6 @@ qemuDomainBlockPivot(virConnectPtr conn, goto cleanup; }
- format = virStorageFileFormatTypeToString(disk->mirror->format); - /* Probe the status, if needed. */ if (!disk->mirrorState) { qemuDomainObjEnterMonitor(driver, vm); @@ -16147,7 +16144,7 @@ qemuDomainBlockPivot(virConnectPtr conn, * overall return value. */ disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT; qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorDrivePivot(priv->mon, device, disk->mirror->path, format); + ret = qemuMonitorDrivePivot(priv->mon, device); if (qemuDomainObjExitMonitor(driver, vm) < 0) { ret = -1; goto cleanup; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index aae259f..0e50dbb 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3492,23 +3492,20 @@ qemuMonitorDiskNameLookup(qemuMonitorPtr mon, }
-/* Use the block-job-complete monitor command to pivot a block copy - * job. */ +/* Use the block-job-complete monitor command to pivot a block copy job. */
NIT: extraneous space before close comment...
int -qemuMonitorDrivePivot(qemuMonitorPtr mon, const char *device, - const char *file, const char *format) +qemuMonitorDrivePivot(qemuMonitorPtr mon, + const char *device) { - int ret = -1; - - VIR_DEBUG("mon=%p, device=%s, file=%s, format=%s", - mon, device, file, NULLSTR(format)); + VIR_DEBUG("mon=%p, device=%s", mon, device);
- if (mon->json) - ret = qemuMonitorJSONDrivePivot(mon, device, file, format); - else + if (mon->json) {
if (!mon->json) ACK with the adjustment... John
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("drive pivot requires JSON monitor")); - return ret; + return -1; + } + + return qemuMonitorJSONDrivePivot(mon, device); }
int qemuMonitorArbitraryCommand(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 9cd0d9d..78f4648 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -721,10 +721,8 @@ int qemuMonitorDriveMirror(qemuMonitorPtr mon, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int qemuMonitorDrivePivot(qemuMonitorPtr mon, - const char *device, - const char *file, - const char *format) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + const char *device) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int qemuMonitorBlockCommit(qemuMonitorPtr mon, const char *device, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 34a6d56..e0f16ec 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3859,9 +3859,8 @@ qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, const char *device, }
int -qemuMonitorJSONDrivePivot(qemuMonitorPtr mon, const char *device, - const char *file ATTRIBUTE_UNUSED, - const char *format ATTRIBUTE_UNUSED) +qemuMonitorJSONDrivePivot(qemuMonitorPtr mon, + const char *device) { int ret; virJSONValuePtr cmd; diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 09f898c..5185bbf 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -264,9 +264,7 @@ int qemuMonitorJSONDriveMirror(qemuMonitorPtr mon, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int qemuMonitorJSONDrivePivot(qemuMonitorPtr mon, - const char *device, - const char *file, - const char *format) + const char *device) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int qemuMonitorJSONBlockCommit(qemuMonitorPtr mon, diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index f09176a..170d690 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1186,7 +1186,7 @@ GEN_TEST_FUNC(qemuMonitorJSONSetDrivePassphrase, "vda", "secret_passhprase") GEN_TEST_FUNC(qemuMonitorJSONDriveMirror, "vdb", "/foo/bar", NULL, 1024, 0, 0, VIR_DOMAIN_BLOCK_REBASE_SHALLOW | VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT) GEN_TEST_FUNC(qemuMonitorJSONBlockCommit, "vdb", "/foo/bar1", "/foo/bar2", NULL, 1024) -GEN_TEST_FUNC(qemuMonitorJSONDrivePivot, "vdb", NULL, NULL) +GEN_TEST_FUNC(qemuMonitorJSONDrivePivot, "vdb") GEN_TEST_FUNC(qemuMonitorJSONScreendump, "/foo/bar") GEN_TEST_FUNC(qemuMonitorJSONOpenGraphics, "spice", "spicefd", false) GEN_TEST_FUNC(qemuMonitorJSONNBDServerStart, "localhost", 12345)
Support for drive-reopen was never present in the upstream code so we don't need to pause the VM when doing the block pivot. Kill all the code related to this semi-upstream artifact. --- src/qemu/qemu_driver.c | 48 +++++------------------------------------------- 1 file changed, 5 insertions(+), 43 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7c33ca3..44ee04f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16043,14 +16043,14 @@ qemuDiskPathToAlias(virDomainObjPtr vm, const char *path, int *idxret) * abort with pivot; this updates the VM definition as appropriate, on * either success or failure. */ static int -qemuDomainBlockPivot(virConnectPtr conn, - virQEMUDriverPtr driver, virDomainObjPtr vm, - const char *device, virDomainDiskDefPtr disk) +qemuDomainBlockPivot(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *device, + virDomainDiskDefPtr disk) { int ret = -1, rc; qemuDomainObjPrivatePtr priv = vm->privateData; virDomainBlockJobInfo info; - bool resume = false; virStorageSourcePtr oldsrc = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); @@ -16081,29 +16081,6 @@ qemuDomainBlockPivot(virConnectPtr conn, goto cleanup; } - /* If we are using the older 'drive-reopen', we want to make sure - * that management apps can tell whether the command succeeded, - * even if libvirtd is restarted at the wrong time. To accomplish - * that, we pause the guest before drive-reopen, and resume it - * only when we know the outcome; if libvirtd restarts, then - * management will see the guest still paused, and know that no - * guest I/O has caused the source and mirror to diverge. XXX - * With the newer 'block-job-complete', we need to use a - * persistent bitmap to make things safe; so for now, we just - * blindly pause the guest. */ - if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { - if (qemuProcessStopCPUs(driver, vm, VIR_DOMAIN_PAUSED_SAVE, - QEMU_ASYNC_JOB_NONE) < 0) - goto cleanup; - - resume = true; - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("guest unexpectedly quit")); - goto cleanup; - } - } - /* For active commit, the mirror is part of the already labeled * chain. For blockcopy, we previously labeled only the top-level * image; but if the user is reusing an external image that @@ -16177,21 +16154,6 @@ qemuDomainBlockPivot(virConnectPtr conn, if (oldsrc) disk->src = oldsrc; - if (resume && virDomainObjIsActive(vm) && - qemuProcessStartCPUs(driver, vm, conn, - VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_NONE) < 0) { - virObjectEventPtr event = NULL; - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_SUSPENDED, - VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR); - if (event) - qemuDomainEventQueue(driver, event); - if (virGetLastError() == NULL) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("resuming after drive-reopen failed")); - } - } virObjectUnref(cfg); return ret; } @@ -16295,7 +16257,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, } if (disk->mirror && (flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT)) { - ret = qemuDomainBlockPivot(conn, driver, vm, device, disk); + ret = qemuDomainBlockPivot(driver, vm, device, disk); if (ret < 0 && async) goto endjob; goto waitjob; -- 2.2.2
On 04/01/2015 01:20 PM, Peter Krempa wrote:
Support for drive-reopen was never present in the upstream code so we don't need to pause the VM when doing the block pivot. Kill all the code related to this semi-upstream artifact. --- src/qemu/qemu_driver.c | 48 +++++------------------------------------------- 1 file changed, 5 insertions(+), 43 deletions(-)
ACK - although it seems to be outside the realm of .0 "Refactor the block job code"... IOW - should this have been a separate patch. John
On Wed, Apr 08, 2015 at 11:31:18 -0400, John Ferlan wrote:
On 04/01/2015 01:20 PM, Peter Krempa wrote:
Support for drive-reopen was never present in the upstream code so we don't need to pause the VM when doing the block pivot. Kill all the code related to this semi-upstream artifact. --- src/qemu/qemu_driver.c | 48 +++++------------------------------------------- 1 file changed, 5 insertions(+), 43 deletions(-)
ACK - although it seems to be outside the realm of .0 "Refactor the block job code"... IOW - should this have been a separate patch.
Separate as in a separate series? .. well it fixes block job code so I think it belongs in this series. I've pushed patches 2,3,4,6 and 7 and will follow up with the rest in V3. Thanks for the review. Peter
Sacrifice a few lines of code in favor of the code being more readable. --- src/qemu/qemu_driver.c | 213 +++++++++++++++++++++++++------------------ src/qemu/qemu_migration.c | 8 +- src/qemu/qemu_monitor.c | 18 ++++ src/qemu/qemu_monitor.h | 6 +- src/qemu/qemu_monitor_json.c | 37 ++++++-- src/qemu/qemu_monitor_json.h | 5 + 6 files changed, 184 insertions(+), 103 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 44ee04f..7b7775d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16173,16 +16173,12 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, char *device = NULL; int ret = -1; bool async = false; - virObjectEventPtr event = NULL; - virObjectEventPtr event2 = NULL; int idx; virDomainDiskDefPtr disk; virStorageSourcePtr baseSource = NULL; unsigned int baseIndex = 0; char *basePath = NULL; char *backingPath = NULL; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - bool save = false; unsigned long long speed = bandwidth; if (!virDomainObjIsActive(vm)) { @@ -16234,40 +16230,6 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, if (mode == BLOCK_JOB_PULL && qemuDomainDiskBlockJobIsActive(disk)) goto endjob; - if (mode == BLOCK_JOB_ABORT) { - if (async && !(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC)) { - /* prepare state for event delivery */ - 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, - _("another job on disk '%s' is still being ended"), - disk->dst); - goto endjob; - } - - if (disk->mirror && (flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT)) { - ret = qemuDomainBlockPivot(driver, vm, device, disk); - if (ret < 0 && async) - goto endjob; - goto waitjob; - } - if (disk->mirror) { - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_ABORT; - save = true; - } - } - if (base && (virStorageFileParseChainIndex(disk->dst, base, &baseIndex) < 0 || !(baseSource = virStorageFileChainLookup(disk->src, disk->src, @@ -16319,13 +16281,107 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; if (ret < 0) { - if (mode == BLOCK_JOB_ABORT && disk->mirror) - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; goto endjob; } else if (mode == BLOCK_JOB_PULL) { disk->blockjob = true; } + endjob: + qemuDomainObjEndJob(driver, vm); + + cleanup: + VIR_FREE(basePath); + VIR_FREE(backingPath); + VIR_FREE(device); + qemuDomObjEndAPI(&vm); + return ret; +} + +static int +qemuDomainBlockJobAbort(virDomainPtr dom, + const char *path, + unsigned int flags) +{ + 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; + virDomainObjPtr vm; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC | + VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT, -1); + + if (!(vm = qemuDomObjFromDomain(dom))) + return -1; + + if (virDomainBlockJobAbortEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (qemuDomainSupportsBlockJobs(vm, &async) < 0) + goto cleanup; + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto endjob; + } + + if (!(device = qemuDiskPathToAlias(vm, path, &idx))) + goto endjob; + disk = vm->def->disks[idx]; + + if (async && !(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC)) { + /* prepare state for event delivery */ + 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, + _("another job on disk '%s' is still being ended"), + disk->dst); + goto endjob; + } + + if (disk->mirror && (flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT)) { + ret = qemuDomainBlockPivot(driver, vm, device, disk); + if (ret < 0 && async) + goto endjob; + goto waitjob; + } + 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; + + 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 @@ -16340,37 +16396,35 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, * block to guarantee synchronous operation. We do the waiting * while still holding the VM job, to prevent newly scheduled * block jobs from confusing us. */ - if (mode == BLOCK_JOB_ABORT) { - 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 (!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; } - - qemuBlockJobEventProcess(driver, vm, disk, - disk->blockJobType, - disk->blockJobStatus); - disk->blockJobSync = false; } + + qemuBlockJobEventProcess(driver, vm, disk, + disk->blockJobType, + disk->blockJobStatus); + disk->blockJobSync = false; } endjob: @@ -16378,8 +16432,6 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, cleanup: virObjectUnref(cfg); - VIR_FREE(basePath); - VIR_FREE(backingPath); VIR_FREE(device); qemuDomObjEndAPI(&vm); if (event) @@ -16389,25 +16441,6 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, return ret; } -static int -qemuDomainBlockJobAbort(virDomainPtr dom, const char *path, unsigned int flags) -{ - virDomainObjPtr vm; - - virCheckFlags(VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC | - VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT, -1); - - if (!(vm = qemuDomObjFromDomain(dom))) - return -1; - - if (virDomainBlockJobAbortEnsureACL(dom->conn, vm->def) < 0) { - qemuDomObjEndAPI(&vm); - return -1; - } - - return qemuDomainBlockJobImpl(vm, dom->conn, path, NULL, 0, - BLOCK_JOB_ABORT, flags); -} static int qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index d34bb02..5394311 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1844,10 +1844,9 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, continue; if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT) == 0) { - if (qemuMonitorBlockJob(priv->mon, diskAlias, NULL, NULL, 0, - BLOCK_JOB_ABORT, true) < 0) { + if (qemuMonitorBlockJobCancel(priv->mon, diskAlias, true) < 0) VIR_WARN("Unable to cancel block-job on '%s'", diskAlias); - } + if (qemuDomainObjExitMonitor(driver, vm) < 0) break; } else { @@ -1914,8 +1913,7 @@ qemuMigrationCancelDriveMirror(qemuMigrationCookiePtr mig, QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) goto cleanup; - if (qemuMonitorBlockJob(priv->mon, diskAlias, NULL, NULL, 0, - BLOCK_JOB_ABORT, true) < 0) + if (qemuMonitorBlockJobCancel(priv->mon, diskAlias, true) < 0) VIR_WARN("Unable to stop block job on %s", diskAlias); if (qemuDomainObjExitMonitor(driver, vm) < 0) { ret = -1; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 0e50dbb..e413d91 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3603,6 +3603,24 @@ qemuMonitorBlockJob(qemuMonitorPtr mon, int +qemuMonitorBlockJobCancel(qemuMonitorPtr mon, + const char *device, + bool modern) +{ + VIR_DEBUG("mon=%p, device=%s, modern=%d", + mon, device, modern); + + if (!mon->json) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("block jobs require JSON monitor")); + return -1; + } + + return qemuMonitorJSONBlockJobCancel(mon, device, modern); +} + + +int qemuMonitorBlockJobSetSpeed(qemuMonitorPtr mon, const char *device, unsigned long long bandwidth, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 78f4648..5cc811a 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -756,7 +756,6 @@ int qemuMonitorSendKey(qemuMonitorPtr mon, unsigned int nkeycodes); typedef enum { - BLOCK_JOB_ABORT, BLOCK_JOB_PULL, } qemuMonitorBlockJobCmd; @@ -769,6 +768,11 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon, bool modern) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int qemuMonitorBlockJobCancel(qemuMonitorPtr mon, + const char *device, + bool modern) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + int qemuMonitorBlockJobSetSpeed(qemuMonitorPtr mon, const char *device, unsigned long long bandwidth, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e0f16ec..01a4f9a 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4325,13 +4325,6 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon, } switch (mode) { - case BLOCK_JOB_ABORT: - cmd_name = modern ? "block-job-cancel" : "block_job_cancel"; - cmd = qemuMonitorJSONMakeCommand(cmd_name, - "s:device", device, - NULL); - break; - case BLOCK_JOB_PULL: cmd_name = modern ? "block-stream" : "block_stream"; cmd = qemuMonitorJSONMakeCommand(cmd_name, @@ -4362,6 +4355,36 @@ qemuMonitorJSONBlockJob(qemuMonitorPtr mon, int +qemuMonitorJSONBlockJobCancel(qemuMonitorPtr mon, + const char *device, + bool modern) +{ + int ret = -1; + virJSONValuePtr cmd = NULL; + virJSONValuePtr reply = NULL; + const char *cmd_name = modern ? "block-job-cancel" : "block_job_cancel"; + + if (!(cmd = qemuMonitorJSONMakeCommand(cmd_name, + "s:device", device, + NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; + + if (qemuMonitorJSONBlockJobError(reply, cmd_name, device) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + + +int qemuMonitorJSONBlockJobSetSpeed(qemuMonitorPtr mon, const char *device, unsigned long long speed, diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 5185bbf..c88972c 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -306,6 +306,11 @@ int qemuMonitorJSONBlockJob(qemuMonitorPtr mon, bool modern) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int qemuMonitorJSONBlockJobCancel(qemuMonitorPtr mon, + const char *device, + bool modern) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + int qemuMonitorJSONBlockJobSetSpeed(qemuMonitorPtr mon, const char *device, unsigned long long speed, -- 2.2.2
On 04/01/2015 01:20 PM, Peter Krempa wrote:
Sacrifice a few lines of code in favor of the code being more readable. --- src/qemu/qemu_driver.c | 213 +++++++++++++++++++++++++------------------ src/qemu/qemu_migration.c | 8 +- src/qemu/qemu_monitor.c | 18 ++++ src/qemu/qemu_monitor.h | 6 +- src/qemu/qemu_monitor_json.c | 37 ++++++-- src/qemu/qemu_monitor_json.h | 5 + 6 files changed, 184 insertions(+), 103 deletions(-)
ACK John
Since it now handles only block pull code paths we can refactor it and remove tons of cruft. --- src/qemu/qemu_driver.c | 86 ++++++++++++++++++++------------------------ src/qemu/qemu_monitor.c | 30 ++++++++-------- src/qemu/qemu_monitor.h | 17 ++++----- src/qemu/qemu_monitor_json.c | 59 +++++++----------------------- src/qemu/qemu_monitor_json.h | 13 ++++--- 5 files changed, 78 insertions(+), 127 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7b7775d..2dd8ed4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16162,17 +16162,16 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, /* bandwidth in MiB/s per public API. Caller must lock vm beforehand, * and not access it afterwards. */ static int -qemuDomainBlockJobImpl(virDomainObjPtr vm, - virConnectPtr conn, - const char *path, const char *base, - unsigned long bandwidth, - int mode, unsigned int flags) +qemuDomainBlockPullCommon(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *path, + const char *base, + unsigned long bandwidth, + unsigned int flags) { - virQEMUDriverPtr driver = conn->privateData; - qemuDomainObjPrivatePtr priv; + qemuDomainObjPrivatePtr priv = vm->privateData; char *device = NULL; - int ret = -1; - bool async = false; + bool modern; int idx; virDomainDiskDefPtr disk; virStorageSourcePtr baseSource = NULL; @@ -16180,12 +16179,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, char *basePath = NULL; char *backingPath = NULL; unsigned long long speed = bandwidth; - - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain is not running")); - goto cleanup; - } + int ret = -1; if (flags & VIR_DOMAIN_BLOCK_REBASE_RELATIVE && !base) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -16194,23 +16188,23 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, goto cleanup; } - priv = vm->privateData; - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC)) { - async = true; - } else if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_SYNC)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("block jobs not supported with this QEMU binary")); - goto cleanup; - } else if (base) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("partial block pull not supported with this " - "QEMU binary")); - goto cleanup; - } else if (mode == BLOCK_JOB_PULL && bandwidth) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("setting bandwidth at start of block pull not " - "supported with this QEMU binary")); + if (qemuDomainSupportsBlockJobs(vm, &modern) < 0) goto cleanup; + + if (!modern) { + if (base) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("partial block pull not supported with this " + "QEMU binary")); + goto cleanup; + } + + if (bandwidth) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("setting bandwidth at start of block pull not " + "supported with this QEMU binary")); + goto cleanup; + } } if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) @@ -16222,12 +16216,11 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, goto endjob; } - device = qemuDiskPathToAlias(vm, path, &idx); - if (!device) + if (!(device = qemuDiskPathToAlias(vm, path, &idx))) goto endjob; disk = vm->def->disks[idx]; - if (mode == BLOCK_JOB_PULL && qemuDomainDiskBlockJobIsActive(disk)) + if (qemuDomainDiskBlockJobIsActive(disk)) goto endjob; if (base && @@ -16250,7 +16243,6 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, &backingPath) < 0) goto endjob; - if (!backingPath) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("can't keep relative backing relationship")); @@ -16260,8 +16252,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, } /* Convert bandwidth MiB to bytes, if needed */ - if (mode == BLOCK_JOB_PULL && - !(flags & VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES)) { + if (!(flags & VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES)) { if (speed > LLONG_MAX >> 20) { virReportError(VIR_ERR_OVERFLOW, _("bandwidth must be less than %llu"), @@ -16276,15 +16267,15 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, basePath = qemuMonitorDiskNameLookup(priv->mon, device, disk->src, baseSource); if (!baseSource || basePath) - ret = qemuMonitorBlockJob(priv->mon, device, basePath, backingPath, - speed, mode, async); + ret = qemuMonitorBlockStream(priv->mon, device, basePath, backingPath, + speed, modern); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; - if (ret < 0) { + + if (ret < 0) goto endjob; - } else if (mode == BLOCK_JOB_PULL) { - disk->blockjob = true; - } + + disk->blockjob = true; endjob: qemuDomainObjEndJob(driver, vm); @@ -16297,6 +16288,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, return ret; } + static int qemuDomainBlockJobAbort(virDomainPtr dom, const char *path, @@ -16778,6 +16770,7 @@ static int qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, unsigned long bandwidth, unsigned int flags) { + virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; unsigned long long speed = bandwidth; @@ -16800,8 +16793,7 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, /* For normal rebase (enhanced blockpull), the common code handles * everything, including vm cleanup. */ if (!(flags & VIR_DOMAIN_BLOCK_REBASE_COPY)) - return qemuDomainBlockJobImpl(vm, dom->conn, path, base, bandwidth, - BLOCK_JOB_PULL, flags); + return qemuDomainBlockPullCommon(driver, vm, path, base, bandwidth, flags); /* If we got here, we are doing a block copy rebase. */ if (VIR_ALLOC(dest) < 0) @@ -16934,8 +16926,8 @@ qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth, return -1; } - return qemuDomainBlockJobImpl(vm, dom->conn, path, NULL, bandwidth, - BLOCK_JOB_PULL, flags); + return qemuDomainBlockPullCommon(dom->conn->privateData, + vm, path, NULL, bandwidth, flags); } diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index e413d91..4791029 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3577,28 +3577,26 @@ int qemuMonitorScreendump(qemuMonitorPtr mon, /* bandwidth is in bytes/sec */ int -qemuMonitorBlockJob(qemuMonitorPtr mon, - const char *device, - const char *base, - const char *backingName, - unsigned long long bandwidth, - qemuMonitorBlockJobCmd mode, - bool modern) +qemuMonitorBlockStream(qemuMonitorPtr mon, + const char *device, + const char *base, + const char *backingName, + unsigned long long bandwidth, + bool modern) { - int ret = -1; - VIR_DEBUG("mon=%p, device=%s, base=%s, backingName=%s, bandwidth=%lluB, " - "mode=%o, modern=%d", + "modern=%d", mon, device, NULLSTR(base), NULLSTR(backingName), - bandwidth, mode, modern); + bandwidth, modern); - if (mon->json) - ret = qemuMonitorJSONBlockJob(mon, device, base, backingName, - bandwidth, mode, modern); - else + if (!mon->json) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("block jobs require JSON monitor")); - return ret; + return -1; + } + + return qemuMonitorJSONBlockStream(mon, device, base, backingName, + bandwidth, modern); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 5cc811a..d673154 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -755,17 +755,12 @@ int qemuMonitorSendKey(qemuMonitorPtr mon, unsigned int *keycodes, unsigned int nkeycodes); -typedef enum { - BLOCK_JOB_PULL, -} qemuMonitorBlockJobCmd; - -int qemuMonitorBlockJob(qemuMonitorPtr mon, - const char *device, - const char *base, - const char *backingName, - unsigned long long bandwidth, - qemuMonitorBlockJobCmd mode, - bool modern) +int qemuMonitorBlockStream(qemuMonitorPtr mon, + const char *device, + const char *base, + const char *backingName, + unsigned long long bandwidth, + bool modern) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int qemuMonitorBlockJobCancel(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 01a4f9a..d02567d 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4286,57 +4286,24 @@ qemuMonitorJSONBlockJobError(virJSONValuePtr reply, /* speed is in bytes/sec */ int -qemuMonitorJSONBlockJob(qemuMonitorPtr mon, - const char *device, - const char *base, - const char *backingName, - unsigned long long speed, - qemuMonitorBlockJobCmd mode, - bool modern) +qemuMonitorJSONBlockStream(qemuMonitorPtr mon, + const char *device, + const char *base, + const char *backingName, + unsigned long long speed, + bool modern) { int ret = -1; virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL; - const char *cmd_name = NULL; - - if (base && (mode != BLOCK_JOB_PULL || !modern)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("only modern block pull supports base: %s"), base); - return -1; - } - - if (backingName && mode != BLOCK_JOB_PULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("backing name is supported only for block pull")); - return -1; - } - - if (backingName && !base) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("backing name requires a base image")); - return -1; - } + const char *cmd_name = modern ? "block-stream" : "block_stream"; - if (speed && mode == BLOCK_JOB_PULL && !modern) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("only modern block pull supports speed: %llu"), - speed); - return -1; - } - - switch (mode) { - case BLOCK_JOB_PULL: - cmd_name = modern ? "block-stream" : "block_stream"; - cmd = qemuMonitorJSONMakeCommand(cmd_name, - "s:device", device, - "Y:speed", speed, - "S:base", base, - "S:backing-file", backingName, - NULL); - break; - } - - if (!cmd) + if (!(cmd = qemuMonitorJSONMakeCommand(cmd_name, + "s:device", device, + "Y:speed", speed, + "S:base", base, + "S:backing-file", backingName, + NULL))) return -1; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index c88972c..43adc90 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -297,13 +297,12 @@ int qemuMonitorJSONSendKey(qemuMonitorPtr mon, int qemuMonitorJSONScreendump(qemuMonitorPtr mon, const char *file); -int qemuMonitorJSONBlockJob(qemuMonitorPtr mon, - const char *device, - const char *base, - const char *backingName, - unsigned long long speed, - qemuMonitorBlockJobCmd mode, - bool modern) +int qemuMonitorJSONBlockStream(qemuMonitorPtr mon, + const char *device, + const char *base, + const char *backingName, + unsigned long long speed, + bool modern) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int qemuMonitorJSONBlockJobCancel(qemuMonitorPtr mon, -- 2.2.2
On 04/01/2015 01:20 PM, Peter Krempa wrote:
Since it now handles only block pull code paths we can refactor it and remove tons of cruft. --- src/qemu/qemu_driver.c | 86 ++++++++++++++++++++------------------------ src/qemu/qemu_monitor.c | 30 ++++++++-------- src/qemu/qemu_monitor.h | 17 ++++----- src/qemu/qemu_monitor_json.c | 59 +++++++----------------------- src/qemu/qemu_monitor_json.h | 13 ++++--- 5 files changed, 78 insertions(+), 127 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7b7775d..2dd8ed4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16162,17 +16162,16 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, /* bandwidth in MiB/s per public API. Caller must lock vm beforehand, * and not access it afterwards. */ static int -qemuDomainBlockJobImpl(virDomainObjPtr vm, - virConnectPtr conn, - const char *path, const char *base, - unsigned long bandwidth, - int mode, unsigned int flags) +qemuDomainBlockPullCommon(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *path, + const char *base, + unsigned long bandwidth, + unsigned int flags) { - virQEMUDriverPtr driver = conn->privateData; - qemuDomainObjPrivatePtr priv; + qemuDomainObjPrivatePtr priv = vm->privateData; char *device = NULL; - int ret = -1; - bool async = false; + bool modern; int idx; virDomainDiskDefPtr disk; virStorageSourcePtr baseSource = NULL; @@ -16180,12 +16179,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, char *basePath = NULL; char *backingPath = NULL; unsigned long long speed = bandwidth; - - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain is not running")); - goto cleanup; - } + int ret = -1;
if (flags & VIR_DOMAIN_BLOCK_REBASE_RELATIVE && !base) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -16194,23 +16188,23 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, goto cleanup; }
- priv = vm->privateData; - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC)) { - async = true; - } else if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_SYNC)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("block jobs not supported with this QEMU binary")); - goto cleanup; - } else if (base) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("partial block pull not supported with this " - "QEMU binary")); - goto cleanup; - } else if (mode == BLOCK_JOB_PULL && bandwidth) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("setting bandwidth at start of block pull not " - "supported with this QEMU binary")); + if (qemuDomainSupportsBlockJobs(vm, &modern) < 0) goto cleanup; + + if (!modern) { + if (base) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("partial block pull not supported with this " + "QEMU binary")); + goto cleanup; + } + + if (bandwidth) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("setting bandwidth at start of block pull not " + "supported with this QEMU binary")); + goto cleanup; + } }
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) @@ -16222,12 +16216,11 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, goto endjob; }
- device = qemuDiskPathToAlias(vm, path, &idx); - if (!device) + if (!(device = qemuDiskPathToAlias(vm, path, &idx))) goto endjob; disk = vm->def->disks[idx];
- if (mode == BLOCK_JOB_PULL && qemuDomainDiskBlockJobIsActive(disk)) + if (qemuDomainDiskBlockJobIsActive(disk)) goto endjob;
if (base && @@ -16250,7 +16243,6 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, &backingPath) < 0) goto endjob;
- if (!backingPath) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("can't keep relative backing relationship")); @@ -16260,8 +16252,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, }
/* Convert bandwidth MiB to bytes, if needed */ - if (mode == BLOCK_JOB_PULL && - !(flags & VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES)) { + if (!(flags & VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES)) { if (speed > LLONG_MAX >> 20) { virReportError(VIR_ERR_OVERFLOW, _("bandwidth must be less than %llu"), @@ -16276,15 +16267,15 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, basePath = qemuMonitorDiskNameLookup(priv->mon, device, disk->src, baseSource); if (!baseSource || basePath) - ret = qemuMonitorBlockJob(priv->mon, device, basePath, backingPath, - speed, mode, async); + ret = qemuMonitorBlockStream(priv->mon, device, basePath, backingPath, + speed, modern);
These don't use your new qemuDomainGetMonitor(vm) - not that it really matters, but since you created it...
if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; - if (ret < 0) { + + if (ret < 0) goto endjob; - } else if (mode == BLOCK_JOB_PULL) { - disk->blockjob = true; - } + + disk->blockjob = true;
endjob: qemuDomainObjEndJob(driver, vm); @@ -16297,6 +16288,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, return ret; }
+ static int qemuDomainBlockJobAbort(virDomainPtr dom, const char *path, @@ -16778,6 +16770,7 @@ static int qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, unsigned long bandwidth, unsigned int flags) { + virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; unsigned long long speed = bandwidth; @@ -16800,8 +16793,7 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, /* For normal rebase (enhanced blockpull), the common code handles * everything, including vm cleanup. */ if (!(flags & VIR_DOMAIN_BLOCK_REBASE_COPY)) - return qemuDomainBlockJobImpl(vm, dom->conn, path, base, bandwidth, - BLOCK_JOB_PULL, flags); + return qemuDomainBlockPullCommon(driver, vm, path, base, bandwidth, flags);
/* If we got here, we are doing a block copy rebase. */ if (VIR_ALLOC(dest) < 0) @@ -16934,8 +16926,8 @@ qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth, return -1; }
- return qemuDomainBlockJobImpl(vm, dom->conn, path, NULL, bandwidth, - BLOCK_JOB_PULL, flags); + return qemuDomainBlockPullCommon(dom->conn->privateData, + vm, path, NULL, bandwidth, flags); }
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index e413d91..4791029 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3577,28 +3577,26 @@ int qemuMonitorScreendump(qemuMonitorPtr mon,
/* bandwidth is in bytes/sec */ int -qemuMonitorBlockJob(qemuMonitorPtr mon, - const char *device, - const char *base, - const char *backingName, - unsigned long long bandwidth, - qemuMonitorBlockJobCmd mode, - bool modern) +qemuMonitorBlockStream(qemuMonitorPtr mon, + const char *device, + const char *base, + const char *backingName, + unsigned long long bandwidth, + bool modern) { - int ret = -1; - VIR_DEBUG("mon=%p, device=%s, base=%s, backingName=%s, bandwidth=%lluB, " - "mode=%o, modern=%d", + "modern=%d", mon, device, NULLSTR(base), NULLSTR(backingName), - bandwidth, mode, modern); + bandwidth, modern);
- if (mon->json) - ret = qemuMonitorJSONBlockJob(mon, device, base, backingName, - bandwidth, mode, modern); - else + if (!mon->json) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("block jobs require JSON monitor")); - return ret; + return -1; + } + + return qemuMonitorJSONBlockStream(mon, device, base, backingName, + bandwidth, modern); }
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 5cc811a..d673154 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -755,17 +755,12 @@ int qemuMonitorSendKey(qemuMonitorPtr mon, unsigned int *keycodes, unsigned int nkeycodes);
-typedef enum { - BLOCK_JOB_PULL, -} qemuMonitorBlockJobCmd; - -int qemuMonitorBlockJob(qemuMonitorPtr mon, - const char *device, - const char *base, - const char *backingName, - unsigned long long bandwidth, - qemuMonitorBlockJobCmd mode, - bool modern) +int qemuMonitorBlockStream(qemuMonitorPtr mon, + const char *device, + const char *base, + const char *backingName, + unsigned long long bandwidth, + bool modern) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int qemuMonitorBlockJobCancel(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 01a4f9a..d02567d 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4286,57 +4286,24 @@ qemuMonitorJSONBlockJobError(virJSONValuePtr reply,
/* speed is in bytes/sec */ int -qemuMonitorJSONBlockJob(qemuMonitorPtr mon, - const char *device, - const char *base, - const char *backingName, - unsigned long long speed, - qemuMonitorBlockJobCmd mode, - bool modern) +qemuMonitorJSONBlockStream(qemuMonitorPtr mon, + const char *device, + const char *base, + const char *backingName, + unsigned long long speed, + bool modern) { int ret = -1; virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL; - const char *cmd_name = NULL; - - if (base && (mode != BLOCK_JOB_PULL || !modern)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("only modern block pull supports base: %s"), base); - return -1; - }
Just checking... This change is essentially the same as in qemuDomainBlockPullCommon where if (!modern) {} was added right?
- - if (backingName && mode != BLOCK_JOB_PULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("backing name is supported only for block pull")); - return -1; - }
And this won't be necessary.... since we no longer have multiple (ab)users of the same API
- - if (backingName && !base) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("backing name requires a base image")); - return -1; - }
Is there a check for this somewhere that I missed?
+ const char *cmd_name = modern ? "block-stream" : "block_stream";
- if (speed && mode == BLOCK_JOB_PULL && !modern) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("only modern block pull supports speed: %llu"), - speed); - return -1; - }
And this is the second half of the check in qemuDomainBlockPullCommon ACK - in general - Just want to make sure the "if (backingName && !base) wasn't erroneously removed. John
- - switch (mode) { - case BLOCK_JOB_PULL: - cmd_name = modern ? "block-stream" : "block_stream"; - cmd = qemuMonitorJSONMakeCommand(cmd_name, - "s:device", device, - "Y:speed", speed, - "S:base", base, - "S:backing-file", backingName, - NULL); - break; - } - - if (!cmd) + if (!(cmd = qemuMonitorJSONMakeCommand(cmd_name, + "s:device", device, + "Y:speed", speed, + "S:base", base, + "S:backing-file", backingName, + NULL))) return -1;
if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index c88972c..43adc90 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -297,13 +297,12 @@ int qemuMonitorJSONSendKey(qemuMonitorPtr mon, int qemuMonitorJSONScreendump(qemuMonitorPtr mon, const char *file);
-int qemuMonitorJSONBlockJob(qemuMonitorPtr mon, - const char *device, - const char *base, - const char *backingName, - unsigned long long speed, - qemuMonitorBlockJobCmd mode, - bool modern) +int qemuMonitorJSONBlockStream(qemuMonitorPtr mon, + const char *device, + const char *base, + const char *backingName, + unsigned long long speed, + bool modern) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int qemuMonitorJSONBlockJobCancel(qemuMonitorPtr mon,
On Wed, Apr 08, 2015 at 13:10:12 -0400, John Ferlan wrote:
On 04/01/2015 01:20 PM, Peter Krempa wrote:
Since it now handles only block pull code paths we can refactor it and remove tons of cruft. --- src/qemu/qemu_driver.c | 86 ++++++++++++++++++++------------------------ src/qemu/qemu_monitor.c | 30 ++++++++-------- src/qemu/qemu_monitor.h | 17 ++++----- src/qemu/qemu_monitor_json.c | 59 +++++++----------------------- src/qemu/qemu_monitor_json.h | 13 ++++--- 5 files changed, 78 insertions(+), 127 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7b7775d..2dd8ed4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c
...
@@ -16276,15 +16267,15 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, basePath = qemuMonitorDiskNameLookup(priv->mon, device, disk->src, baseSource); if (!baseSource || basePath) - ret = qemuMonitorBlockJob(priv->mon, device, basePath, backingPath, - speed, mode, async); + ret = qemuMonitorBlockStream(priv->mon, device, basePath, backingPath, + speed, modern);
These don't use your new qemuDomainGetMonitor(vm) - not that it really matters, but since you created it...
It was meant for functions that don't need the 'priv' variable otherwise so that it can be avoided. Here 'priv' is needed for checking a capability so I did not bother changing it.
if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; - if (ret < 0) { + + if (ret < 0) goto endjob; - } else if (mode == BLOCK_JOB_PULL) { - disk->blockjob = true; - } + + disk->blockjob = true;
endjob: qemuDomainObjEndJob(driver, vm);
...
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 01a4f9a..d02567d 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4286,57 +4286,24 @@ qemuMonitorJSONBlockJobError(virJSONValuePtr reply,
/* speed is in bytes/sec */ int -qemuMonitorJSONBlockJob(qemuMonitorPtr mon, - const char *device, - const char *base, - const char *backingName, - unsigned long long speed, - qemuMonitorBlockJobCmd mode, - bool modern) +qemuMonitorJSONBlockStream(qemuMonitorPtr mon, + const char *device, + const char *base, + const char *backingName, + unsigned long long speed, + bool modern) { int ret = -1; virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL; - const char *cmd_name = NULL; - - if (base && (mode != BLOCK_JOB_PULL || !modern)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("only modern block pull supports base: %s"), base); - return -1; - }
Just checking...
This change is essentially the same as in qemuDomainBlockPullCommon where if (!modern) {} was added right?
Yes. This one would be redundant.
- - if (backingName && mode != BLOCK_JOB_PULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("backing name is supported only for block pull")); - return -1; - }
And this won't be necessary.... since we no longer have multiple (ab)users of the same API
Exactly.
- - if (backingName && !base) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("backing name requires a base image")); - return -1; - }
Is there a check for this somewhere that I missed?
The caller ensures that this does not happen. We could leave this one possibly in if you want.
+ const char *cmd_name = modern ? "block-stream" : "block_stream";
- if (speed && mode == BLOCK_JOB_PULL && !modern) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("only modern block pull supports speed: %llu"), - speed); - return -1; - }
And this is the second half of the check in qemuDomainBlockPullCommon
ACK - in general - Just want to make sure the "if (backingName && !base) wasn't erroneously removed.
John
Peter
On 04/09/2015 10:09 AM, Peter Krempa wrote: ...
Just checking...
This change is essentially the same as in qemuDomainBlockPullCommon where if (!modern) {} was added right?
Yes. This one would be redundant.
- - if (backingName && mode != BLOCK_JOB_PULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("backing name is supported only for block pull")); - return -1; - }
And this won't be necessary.... since we no longer have multiple (ab)users of the same API
Exactly.
- - if (backingName && !base) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("backing name requires a base image")); - return -1; - }
Is there a check for this somewhere that I missed?
The caller ensures that this does not happen. We could leave this one possibly in if you want.
Not a requirement (but at least you know I read the code ;-)). It's one of those paranoia things. John
+ const char *cmd_name = modern ? "block-stream" : "block_stream";
- if (speed && mode == BLOCK_JOB_PULL && !modern) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("only modern block pull supports speed: %llu"), - speed); - return -1; - }
And this is the second half of the check in qemuDomainBlockPullCommon
ACK - in general - Just want to make sure the "if (backingName && !base) wasn't erroneously removed.
John
Peter
QEMU does not abandon the mirror. The job carries on in the synchronised phase and it might be either pivoted again or cancelled. The commit hints that the described behavior was happening in a downstream version. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1202704 --- Notes: I've spent some time looking how the active commit and copy job actually works in qemu, but I did not check if that behavior changed in the upstream releases. At any rate, it makes sense thus I expect that it was there ever-since. src/qemu/qemu_driver.c | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2dd8ed4..e9431ad 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16128,27 +16128,10 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, } if (ret < 0) { - /* On failure, qemu abandons the mirror, and reverts back to - * the source disk (RHEL 6.3 has a bug where the revert could - * cause catastrophic failure in qemu, but we don't need to - * worry about it here as it is not an upstream qemu problem. */ - /* XXX should we be parsing the exact qemu error, or calling - * 'query-block', to see what state we really got left in - * before killing the mirroring job? - * XXX We want to revoke security labels and disk lease, as - * well as audit that revocation, before dropping the original - * source. But it gets tricky if both source and mirror share - * common backing files (we want to only revoke the non-shared - * portion of the chain); so for now, we leak the access to - * the original. */ - virStorageSourceFree(disk->mirror); - disk->mirror = NULL; - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; - disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; - disk->blockjob = false; + /* The pivot failed. The block job in QEMU remains in the synchronised + * phase. Reset the state we changed and return the error to the user */ + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY; } - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) - ret = -1; cleanup: if (oldsrc) -- 2.2.2
QEMU does not abandon the mirror. The job carries on in the synchronised phase and it might be either pivoted again or cancelled. The commit hints that the described behavior was happening in a downstream version. If the command returns false there are two possible options: 1) qemu did not reach the point where it would ask the block job to pivot 2) pivotting failed in the actual qemu coroutine If either of those would happen we return failure and reset the condition that waits for the block job to complete. This makes the API fail but in case where qemu would actually abandon the mirror the fact is notified via the event and handled asynchronously. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1202704 --- Notes: I've spent some time looking how the active commit and copy job actually works in qemu, but I did not check if that behavior changed in the upstream releases. At any rate, it makes sense thus I expect that it was there ever-since. Version 2: - this version resets the flag that makes libvirt wait on the event. This should make the API as rugged as it can possibly be. src/qemu/qemu_driver.c | 27 ++++++--------------------- 1 file changed, 6 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2dd8ed4..52c3587 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16128,27 +16128,10 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, } if (ret < 0) { - /* On failure, qemu abandons the mirror, and reverts back to - * the source disk (RHEL 6.3 has a bug where the revert could - * cause catastrophic failure in qemu, but we don't need to - * worry about it here as it is not an upstream qemu problem. */ - /* XXX should we be parsing the exact qemu error, or calling - * 'query-block', to see what state we really got left in - * before killing the mirroring job? - * XXX We want to revoke security labels and disk lease, as - * well as audit that revocation, before dropping the original - * source. But it gets tricky if both source and mirror share - * common backing files (we want to only revoke the non-shared - * portion of the chain); so for now, we leak the access to - * the original. */ - virStorageSourceFree(disk->mirror); - disk->mirror = NULL; - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; - disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; - disk->blockjob = false; + /* The pivot failed. The block job in QEMU remains in the synchronised + * phase. Reset the state we changed and return the error to the user */ + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY; } - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) - ret = -1; cleanup: if (oldsrc) @@ -16354,8 +16337,10 @@ qemuDomainBlockJobAbort(virDomainPtr dom, if (disk->mirror && (flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT)) { ret = qemuDomainBlockPivot(driver, vm, device, disk); - if (ret < 0 && async) + if (ret < 0 && async) { + disk->blockJobSync = false; goto endjob; + } goto waitjob; } if (disk->mirror) { -- 2.2.2
On 04/01/2015 04:40 PM, Peter Krempa wrote:
QEMU does not abandon the mirror. The job carries on in the synchronised phase and it might be either pivoted again or cancelled. The commit hints that the described behavior was happening in a downstream version.
If the command returns false there are two possible options: 1) qemu did not reach the point where it would ask the block job to pivot 2) pivotting failed in the actual qemu coroutine
If either of those would happen we return failure and reset the condition that waits for the block job to complete. This makes the API fail but in case where qemu would actually abandon the mirror the fact is notified via the event and handled asynchronously.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1202704 ---
Notes: I've spent some time looking how the active commit and copy job actually works in qemu, but I did not check if that behavior changed in the upstream releases. At any rate, it makes sense thus I expect that it was there ever-since.
Version 2: - this version resets the flag that makes libvirt wait on the event. This should make the API as rugged as it can possibly be.
src/qemu/qemu_driver.c | 27 ++++++--------------------- 1 file changed, 6 insertions(+), 21 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2dd8ed4..52c3587 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16128,27 +16128,10 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, }
if (ret < 0) { - /* On failure, qemu abandons the mirror, and reverts back to - * the source disk (RHEL 6.3 has a bug where the revert could - * cause catastrophic failure in qemu, but we don't need to - * worry about it here as it is not an upstream qemu problem. */ - /* XXX should we be parsing the exact qemu error, or calling - * 'query-block', to see what state we really got left in - * before killing the mirroring job? - * XXX We want to revoke security labels and disk lease, as - * well as audit that revocation, before dropping the original - * source. But it gets tricky if both source and mirror share - * common backing files (we want to only revoke the non-shared - * portion of the chain); so for now, we leak the access to - * the original. */ - virStorageSourceFree(disk->mirror); - disk->mirror = NULL; - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; - disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; - disk->blockjob = false; + /* The pivot failed. The block job in QEMU remains in the synchronised + * phase. Reset the state we changed and return the error to the user */ + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY; }
I won't pretend to say I understand all the comments that get deleted in this, but I assume they were reacting to various "changes" as time went on with perhaps a final decision at some point in time to So I'd say a weak ACK only because I don't have all the history on this nor do I have in mind all the various states... Although since I do see the code checks a few lines above: if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY) { error... } returning mirrorState back to READY does seem logical to me. My only other comment would be you could move the comment outside the {} and then remove the {}'s... John FWIW: Depending on the dictionary one uses - synchronized or canceled may be used, but using synchronised and cancelled seems to go against the will of Thunderbird's spell checker ;-)
- if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) - ret = -1;
cleanup: if (oldsrc) @@ -16354,8 +16337,10 @@ qemuDomainBlockJobAbort(virDomainPtr dom,
if (disk->mirror && (flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT)) { ret = qemuDomainBlockPivot(driver, vm, device, disk); - if (ret < 0 && async) + if (ret < 0 && async) { + disk->blockJobSync = false; goto endjob; + } goto waitjob; } if (disk->mirror) {
On 04/01/2015 01:20 PM, Peter Krempa wrote:
QEMU does not abandon the mirror. The job carries on in the synchronised phase and it might be either pivoted again or cancelled. The commit hints that the described behavior was happening in a downstream version.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1202704 ---
Notes: I've spent some time looking how the active commit and copy job actually works in qemu, but I did not check if that behavior changed in the upstream releases. At any rate, it makes sense thus I expect that it was there ever-since.
src/qemu/qemu_driver.c | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-)
I'll respond to the v2... which got sent "outside" the series (I believe you forgot the git format-patch --in-reply-to=...) John
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) { - 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)) { + 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; } -- 2.2.2
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 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) {
This one (non) change of 'async' to 'modern' still has me wondering a bit... Especially since So previously "async" was what is now modern, so does this reference need to change as well? Now 'async' means 'flags' provided ASYNC compared to previously where it meant the capability supports async. The comment before this check describes the synch vs. asynch, but now there's no else (async)... Something just doesn't feel right.
- /* 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 {
This else used to be off the old (!async) (or now modern)... John
+ 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)) { + 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; }
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
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 = 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 a
Actually mirrorState won't be enough for block jobs that don't use the mirroring approach, which is namely blockCommit to the non-active layer or block stream (block pull).
signals COMPLETED if a drive mirror is canceled after it's fully synced.
Ghm I might want to go re-read the code then ...
+ virReportError(VIR_ERR_OPERATION_FAILED, + _("failed to terminate block job on disk '%s'"), + disk->dst); + ret = -1; + } + disk->blockJobSync = false; }
Thanks for the input. Peter
participants (3)
-
John Ferlan -
Michael Chapman -
Peter Krempa