[libvirt] [PATCHv3 0/7] Block job fixes and refactors
Peter Krempa (7): qemu: monitor: json: Refactor error code class checker qemu: monitor: Extract handling of JSON block job error codes qemu: blockjob: Split qemuDomainBlockJobSetSpeed from qemuDomainBlockJobImpl 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_driver.c | 367 ++++++++++++++++++++++++------------------- src/qemu/qemu_migration.c | 8 +- src/qemu/qemu_monitor.c | 67 ++++++-- src/qemu/qemu_monitor.h | 29 ++-- src/qemu/qemu_monitor_json.c | 213 ++++++++++++++----------- src/qemu/qemu_monitor_json.h | 24 ++- 6 files changed, 414 insertions(+), 294 deletions(-) -- 2.2.2
Split out the function that checks the actual error class string into a separate helper as it will be useful later and refactor qemuMonitorJSONHasError to return bool type and remove few useless checks. Basically virJSONValueObjectHasKey are useless here since the next call to virJSONValueObjectGet is checking the return value again (which can't fail at that point). By removing the first check we save a function call. --- Notes: Version 3: - new in series src/qemu/qemu_monitor_json.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 80754f2..752250b 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -400,31 +400,27 @@ qemuMonitorJSONCheckError(virJSONValuePtr cmd, } -static int +static bool +qemuMonitorJSONErrorIsClass(virJSONValuePtr error, + const char *klass) +{ + return STREQ_NULLABLE(virJSONValueObjectGetString(error, "class"), klass); +} + + +static bool qemuMonitorJSONHasError(virJSONValuePtr reply, const char *klass) { virJSONValuePtr error; - const char *thisklass; - - if (!virJSONValueObjectHasKey(reply, "error")) - return 0; - error = virJSONValueObjectGet(reply, "error"); - if (!error) - return 0; - - if (!virJSONValueObjectHasKey(error, "class")) - return 0; - - thisklass = virJSONValueObjectGetString(error, "class"); + if (!(error = virJSONValueObjectGet(reply, "error"))) + return false; - if (!thisklass) - return 0; - - return STREQ(klass, thisklass); + return qemuMonitorJSONErrorIsClass(error, klass); } + /* Top-level commands and nested transaction list elements share a * common structure for everything except the dictionary names. */ static virJSONValuePtr ATTRIBUTE_SENTINEL -- 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. With the new helper qemuMonitorJSONErrorIsClass we can save a few function calls as we can extract the error object once. --- Notes: Version 3: - tweaked to change in previous patch to make coverity happy 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 752250b..f449d0e 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4247,6 +4247,39 @@ qemuMonitorJSONBlockJobInfo(qemuMonitorPtr mon, } +static int +qemuMonitorJSONBlockJobError(virJSONValuePtr reply, + const char *cmd_name, + const char *device) +{ + virJSONValuePtr error; + + if (!(error = virJSONValueObjectGet(reply, "error"))) + return 0; + + if (qemuMonitorJSONErrorIsClass(error, "DeviceNotActive")) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("No active operation on device: %s"), device); + } else if (qemuMonitorJSONErrorIsClass(error, "DeviceInUse")) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("Device %s in use"), device); + } else if (qemuMonitorJSONErrorIsClass(error, "NotSupported")) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Operation is not supported for device: %s"), device); + } else if (qemuMonitorJSONErrorIsClass(error, "CommandNotFound")) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("Command '%s' is not found"), cmd_name); + } else { + 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, @@ -4317,34 +4350,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
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. --- Notes: Version 3: - no change, already ACKed 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 00e4e17..ab962f3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16292,10 +16292,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"), @@ -16486,23 +16484,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 2b0e1a5..3a6a746 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3603,6 +3603,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 a3514cf..78f4648 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -757,7 +757,6 @@ int qemuMonitorSendKey(qemuMonitorPtr mon, typedef enum { BLOCK_JOB_ABORT, - BLOCK_JOB_SPEED, BLOCK_JOB_PULL, } qemuMonitorBlockJobCmd; @@ -770,6 +769,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 f449d0e..9482982 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4328,14 +4328,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, @@ -4364,6 +4356,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 fd86447..5185bbf 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -306,6 +306,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
Sacrifice a few lines of code in favor of the code being more readable. --- Notes: Version 3: - no change, already ACKed 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 ab962f3..1c0e966 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16167,16 +16167,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)) { @@ -16228,40 +16224,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, @@ -16313,13 +16275,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 @@ -16334,37 +16390,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: @@ -16372,8 +16426,6 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, cleanup: virObjectUnref(cfg); - VIR_FREE(basePath); - VIR_FREE(backingPath); VIR_FREE(device); qemuDomObjEndAPI(&vm); if (event) @@ -16383,25 +16435,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 29f5173..59d734b 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1846,10 +1846,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 { @@ -1920,8 +1919,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 3a6a746..882d2e3 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 9482982..97a6d9f 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4321,13 +4321,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, @@ -4358,6 +4351,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
Since it now handles only block pull code paths we can refactor it and remove tons of cruft. --- Notes: Version 3: - no change, ACKed and explained 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 1c0e966..2a2c26a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16156,17 +16156,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; @@ -16174,12 +16173,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", @@ -16188,23 +16182,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) @@ -16216,12 +16210,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 && @@ -16244,7 +16237,6 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, &backingPath) < 0) goto endjob; - if (!backingPath) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("can't keep relative backing relationship")); @@ -16254,8 +16246,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"), @@ -16270,15 +16261,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); @@ -16291,6 +16282,7 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, return ret; } + static int qemuDomainBlockJobAbort(virDomainPtr dom, const char *path, @@ -16773,6 +16765,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; @@ -16795,8 +16788,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 882d2e3..2846605 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 97a6d9f..3af319c 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4282,57 +4282,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
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. Version 3: - no change 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 2a2c26a..eebed55 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16122,27 +16122,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) @@ -16348,8 +16331,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
Change few variable names and refactor the code flow. As an additional bonus the function now fails if the event state is not as expected. --- Notes: Version 3: - fixed error reporting code and success code propagation from pivot src/qemu/qemu_driver.c | 107 +++++++++++++++++++++++-------------------------- 1 file changed, 51 insertions(+), 56 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index eebed55..8d4aa97 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16273,13 +16273,13 @@ qemuDomainBlockJobAbort(virDomainPtr dom, { virQEMUDriverPtr driver = dom->conn->privateData; char *device = NULL; - virObjectEventPtr event = NULL; - virObjectEventPtr event2 = NULL; virDomainDiskDefPtr disk; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); bool save = false; int idx; - bool async; + bool modern; + bool pivot = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT); + bool async = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC); virDomainObjPtr vm; int ret = -1; @@ -16292,7 +16292,7 @@ qemuDomainBlockJobAbort(virDomainPtr dom, if (virDomainBlockJobAbortEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (qemuDomainSupportsBlockJobs(vm, &async) < 0) + if (qemuDomainSupportsBlockJobs(vm, &modern) < 0) goto cleanup; if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) @@ -16308,19 +16308,14 @@ qemuDomainBlockJobAbort(virDomainPtr dom, goto endjob; disk = vm->def->disks[idx]; - if (async && !(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC)) { - /* prepare state for event delivery */ + if (modern && !async) { + /* prepare state for event delivery. Since qemuDomainBlockPivot is + * synchronous, but the event is delivered asynchronously we need to + * wait too */ disk->blockJobStatus = -1; disk->blockJobSync = true; } - if ((flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT) && - !(async && disk->mirror)) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("pivot of disk '%s' requires an active copy job"), - disk->dst); - goto endjob; - } if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_NONE && disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -16329,31 +16324,31 @@ qemuDomainBlockJobAbort(virDomainPtr dom, goto endjob; } - if (disk->mirror && (flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT)) { - ret = qemuDomainBlockPivot(driver, vm, device, disk); - if (ret < 0 && async) { + if (pivot) { + if ((ret = qemuDomainBlockPivot(driver, vm, device, disk)) < 0) { disk->blockJobSync = false; goto endjob; } - goto waitjob; - } - if (disk->mirror) { - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_ABORT; - save = true; - } + } else { + if (disk->mirror) { + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_ABORT; + save = true; + } - qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorBlockJobCancel(qemuDomainGetMonitor(vm), device, async); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - ret = -1; + qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorBlockJobCancel(qemuDomainGetMonitor(vm), device, modern); + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + ret = -1; + goto endjob; + } - 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. */ @@ -16368,33 +16363,37 @@ 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 we've got an explicit failure */ + if (disk->blockJobStatus == VIR_DOMAIN_BLOCK_JOB_FAILED) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("failed to terminate block job on disk '%s'"), + disk->dst); + ret = -1; + } + disk->blockJobSync = false; } @@ -16405,10 +16404,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/09/2015 12:45 PM, Peter Krempa wrote:
Change few variable names and refactor the code flow. As an additional bonus the function now fails if the event state is not as expected. ---
Notes: Version 3: - fixed error reporting code and success code propagation from pivot
src/qemu/qemu_driver.c | 107 +++++++++++++++++++++++-------------------------- 1 file changed, 51 insertions(+), 56 deletions(-)
ACK 1-6... Just one question below...
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index eebed55..8d4aa97 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16273,13 +16273,13 @@ qemuDomainBlockJobAbort(virDomainPtr dom, { virQEMUDriverPtr driver = dom->conn->privateData; char *device = NULL; - virObjectEventPtr event = NULL; - virObjectEventPtr event2 = NULL; virDomainDiskDefPtr disk; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); bool save = false; int idx; - bool async; + bool modern; + bool pivot = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT); + bool async = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC); virDomainObjPtr vm; int ret = -1;
@@ -16292,7 +16292,7 @@ qemuDomainBlockJobAbort(virDomainPtr dom, if (virDomainBlockJobAbortEnsureACL(dom->conn, vm->def) < 0) goto cleanup;
- if (qemuDomainSupportsBlockJobs(vm, &async) < 0) + if (qemuDomainSupportsBlockJobs(vm, &modern) < 0) goto cleanup;
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) @@ -16308,19 +16308,14 @@ qemuDomainBlockJobAbort(virDomainPtr dom, goto endjob; disk = vm->def->disks[idx];
- if (async && !(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC)) { - /* prepare state for event delivery */ + if (modern && !async) { + /* prepare state for event delivery. Since qemuDomainBlockPivot is + * synchronous, but the event is delivered asynchronously we need to + * wait too */ disk->blockJobStatus = -1; disk->blockJobSync = true; }
- if ((flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT) && - !(async && disk->mirror)) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("pivot of disk '%s' requires an active copy job"), - disk->dst); - goto endjob; - } if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_NONE && disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -16329,31 +16324,31 @@ qemuDomainBlockJobAbort(virDomainPtr dom, goto endjob; }
- if (disk->mirror && (flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT)) { - ret = qemuDomainBlockPivot(driver, vm, device, disk); - if (ret < 0 && async) { + if (pivot) { + if ((ret = qemuDomainBlockPivot(driver, vm, device, disk)) < 0) { disk->blockJobSync = false; goto endjob; } - goto waitjob; - } - if (disk->mirror) { - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_ABORT; - save = true; - } + } else { + if (disk->mirror) { + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_ABORT; + save = true; + }
- qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorBlockJobCancel(qemuDomainGetMonitor(vm), device, async); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - ret = -1; + qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorBlockJobCancel(qemuDomainGetMonitor(vm), device, modern); + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + ret = -1; + goto endjob; + }
should this just fall through now? Asked differently - should disk->mirrorState change before goto endjob if ExitMonitor fails? Would "if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0)" do the trick? ACK - in general - just making sure something wasn't missed. John
- 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. */ @@ -16368,33 +16363,37 @@ 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 we've got an explicit failure */ + if (disk->blockJobStatus == VIR_DOMAIN_BLOCK_JOB_FAILED) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("failed to terminate block job on disk '%s'"), + disk->dst); + ret = -1; + } + disk->blockJobSync = false; }
@@ -16405,10 +16404,6 @@ qemuDomainBlockJobAbort(virDomainPtr dom, virObjectUnref(cfg); VIR_FREE(device); qemuDomObjEndAPI(&vm); - if (event) - qemuDomainEventQueue(driver, event); - if (event2) - qemuDomainEventQueue(driver, event2); return ret; }
On Mon, Apr 13, 2015 at 21:08:27 -0400, John Ferlan wrote:
On 04/09/2015 12:45 PM, Peter Krempa wrote:
Change few variable names and refactor the code flow. As an additional bonus the function now fails if the event state is not as expected. ---
Notes: Version 3: - fixed error reporting code and success code propagation from pivot
src/qemu/qemu_driver.c | 107 +++++++++++++++++++++++-------------------------- 1 file changed, 51 insertions(+), 56 deletions(-)
ACK 1-6...
Just one question below...
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index eebed55..8d4aa97 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16273,13 +16273,13 @@ qemuDomainBlockJobAbort(virDomainPtr dom, { virQEMUDriverPtr driver = dom->conn->privateData; char *device = NULL; - virObjectEventPtr event = NULL; - virObjectEventPtr event2 = NULL; virDomainDiskDefPtr disk; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); bool save = false; int idx; - bool async; + bool modern; + bool pivot = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT); + bool async = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC); virDomainObjPtr vm; int ret = -1;
@@ -16292,7 +16292,7 @@ qemuDomainBlockJobAbort(virDomainPtr dom, if (virDomainBlockJobAbortEnsureACL(dom->conn, vm->def) < 0) goto cleanup;
- if (qemuDomainSupportsBlockJobs(vm, &async) < 0) + if (qemuDomainSupportsBlockJobs(vm, &modern) < 0) goto cleanup;
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) @@ -16308,19 +16308,14 @@ qemuDomainBlockJobAbort(virDomainPtr dom, goto endjob; disk = vm->def->disks[idx];
- if (async && !(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC)) { - /* prepare state for event delivery */ + if (modern && !async) { + /* prepare state for event delivery. Since qemuDomainBlockPivot is + * synchronous, but the event is delivered asynchronously we need to + * wait too */ disk->blockJobStatus = -1; disk->blockJobSync = true; }
- if ((flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT) && - !(async && disk->mirror)) { - virReportError(VIR_ERR_OPERATION_INVALID, - _("pivot of disk '%s' requires an active copy job"), - disk->dst); - goto endjob; - } if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_NONE && disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -16329,31 +16324,31 @@ qemuDomainBlockJobAbort(virDomainPtr dom, goto endjob; }
- if (disk->mirror && (flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT)) { - ret = qemuDomainBlockPivot(driver, vm, device, disk); - if (ret < 0 && async) { + if (pivot) { + if ((ret = qemuDomainBlockPivot(driver, vm, device, disk)) < 0) { disk->blockJobSync = false; goto endjob; } - goto waitjob; - } - if (disk->mirror) { - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_ABORT; - save = true; - } + } else { + if (disk->mirror) { + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_ABORT; + save = true; + }
- qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorBlockJobCancel(qemuDomainGetMonitor(vm), device, async); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - ret = -1; + qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorBlockJobCancel(qemuDomainGetMonitor(vm), device, modern); + if (qemuDomainObjExitMonitor(driver, vm) < 0) { + ret = -1; + goto endjob; + }
should this just fall through now? Asked differently - should disk->mirrorState change before goto endjob if ExitMonitor fails?
Would "if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0)" do the trick?
The mirror state was changed to VIR_DOMAIN_DISK_MIRROR_STATE_ABORT prior to the monitor call. If the call fails, we did not issue the cancel and thus need to revert to a safe state for a potential new operation.
ACK - in general - just making sure something wasn't missed.
I've pushed the series, thanks.
John
Peter
participants (2)
-
John Ferlan -
Peter Krempa