Re: [libvirt] [PATCH v2] qemu: report block job errors from qemu to the user

No commit message... This definitely needs one. Also when there's one patch, you don't need a cover letter - instead you put whatever "message" you want to give to reviewers, the pointer to the previous version, changes with the current code, etc. after the "---" -----------------------------------------------+ | On 10/14/2016 05:12 AM, Nikolay Shirokovskiy wrote: | I think this could have been split into setting the blockJobError in qemuMonitorJSONHandleBlockJobImpl along with the VIR_FREE in qemuDomainDiskPrivateDispose and of course the set/fetch in qemuMonitorJSONHandleBlockJobImpl. Then the second patch wold handle grabbing that error message... ... Need to update function comments to add @error Is the VIR_FREE is necessary since on the "next" usage we'd free or we'd free on private dispose. Why only support sync mode? (a reference to Jira's suggestion about adjustment to qemuBlockJobEventProcess). Although you know your function does the *error = NULL; - it's always good practice to just initialize here anyway - better safe than sorry... I'll only mention it once, but it is repeated... Perhaps somewhat ironic that this code fetches diskPriv and the API we're about to call fills in error with diskPriv->blockJobError or am I missing something more subtle. I was going to say use NULLSTR(), but I see Jirka already requested separate messages in the review of v1. Another option would be using the first error message format and then in order to determine what to use for the second %s - error ? error : _("unknown") [although that may send message purists off the deepend]. > return -1; > } > + VIR_FREE(error); > > if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY) > notReady++; > @@ -1908,17 +1916,23 @@ qemuMigrationDriveMirrorCancelled(virQEMUDriverPtr driver, > for (i = 0; i < vm->def->ndisks; i++) { > virDomainDiskDefPtr disk = vm->def->disks[i]; > qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); > + char *error; > > > - status = qemuBlockJobUpdate(driver, vm, disk); > + status = qemuBlockJobUpdate(driver, vm, disk, &error); > switch (status) { > case VIR_DOMAIN_BLOCK_JOB_FAILED: > if (check) { > - virReportError(VIR_ERR_OPERATION_FAILED, > - _("migration of disk %s failed"), > - disk->dst); > + if (error) { > + virReportError(VIR_ERR_OPERATION_FAILED, > + _("migration of disk %s failed: %s"), > + disk->dst, error); > + } else { > + virReportError(VIR_ERR_OPERATION_FAILED, > + _("migration of disk %s failed"), disk->dst); > + } > failed = true; > } > /* fallthrough */ > @@ -1931,6 +1945,7 @@ qemuMigrationDriveMirrorCancelled(virQEMUDriverPtr driver, > default: > active++; > } > + VIR_FREE(error); > } > > if (failed) { > @@ -1968,24 +1983,30 @@ qemuMigrationCancelOneDriveMirror(virQEMUDriverPtr driver, > { > qemuDomainObjPrivatePtr priv = vm->privateData; > char *diskAlias = NULL; > + char *error; > int ret = -1; > int status; > int rv; > > - status = qemuBlockJobUpdate(driver, vm, disk); > + status = qemuBlockJobUpdate(driver, vm, disk, &error); > switch (status) { > case VIR_DOMAIN_BLOCK_JOB_FAILED: > case VIR_DOMAIN_BLOCK_JOB_CANCELED: > if (failNoJob) { > - virReportError(VIR_ERR_OPERATION_FAILED, > - _("migration of disk %s failed"), > - disk->dst); > - return -1; > + if (error) { > + virReportError(VIR_ERR_OPERATION_FAILED, > + _("migration of disk %s failed: %s"), > + disk->dst, error); > + } else { > + virReportError(VIR_ERR_OPERATION_FAILED, > + _("migration of disk %s failed"), disk->dst); > + } > + goto cleanup; > } > - return 1; > - > + /* fallthrough */ > case VIR_DOMAIN_BLOCK_JOB_COMPLETED: > - return 1; > + ret = 1; > + goto cleanup; > } > > if (!(diskAlias = qemuAliasFromDisk(disk))) > @@ -2003,6 +2024,7 @@ qemuMigrationCancelOneDriveMirror(virQEMUDriverPtr driver, > > cleanup: > VIR_FREE(diskAlias); > + VIR_FREE(error); > return ret; > } > > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c > index 5175f4e..34a393a 100644 > --- a/src/qemu/qemu_monitor.c > +++ b/src/qemu/qemu_monitor.c > @@ -1458,13 +1458,14 @@ int > qemuMonitorEmitBlockJob(qemuMonitorPtr mon, > const char *diskAlias, > int type, > - int status) > + int status, > + const char *error) > { > int ret = -1; > VIR_DEBUG("mon=%p", mon); > > QEMU_MONITOR_CALLBACK(mon, ret, domainBlockJob, mon->vm, > - diskAlias, type, status); > + diskAlias, type, status, error); > return ret; > } > > diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h > index 7d78e5b..522675f 100644 > --- a/src/qemu/qemu_monitor.h > +++ b/src/qemu/qemu_monitor.h > @@ -146,6 +146,7 @@ typedef int (*qemuMonitorDomainBlockJobCallback)(qemuMonitorPtr mon, > const char *diskAlias, > int type, > int status, > + const char *error, > void *opaque); > typedef int (*qemuMonitorDomainTrayChangeCallback)(qemuMonitorPtr mon, > virDomainObjPtr vm, > @@ -332,7 +333,8 @@ int qemuMonitorEmitPMSuspend(qemuMonitorPtr mon); > int qemuMonitorEmitBlockJob(qemuMonitorPtr mon, > const char *diskAlias, > int type, > - int status); > + int status, > + const char *error); > int qemuMonitorEmitBalloonChange(qemuMonitorPtr mon, > unsigned long long actual); > int qemuMonitorEmitPMSuspendDisk(qemuMonitorPtr mon); > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c > index b93220b..271598f 100644 > --- a/src/qemu/qemu_monitor_json.c > +++ b/src/qemu/qemu_monitor_json.c > @@ -802,6 +802,7 @@ qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon, > { > const char *device; > const char *type_str; > + const char *error = NULL; > int type = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; > unsigned long long offset, len; > > @@ -834,6 +835,7 @@ qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon, > > switch ((virConnectDomainEventBlockJobStatus) event) { > case VIR_DOMAIN_BLOCK_JOB_COMPLETED: > + error = virJSONValueObjectGetString(data, "error"); > /* Make sure the whole device has been processed */ > if (offset != len) > event = VIR_DOMAIN_BLOCK_JOB_FAILED; > @@ -848,7 +850,7 @@ qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon, > } > > out: > - qemuMonitorEmitBlockJob(mon, device, type, event); > + qemuMonitorEmitBlockJob(mon, device, type, event, error); > } > > static void > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 0f5a11b..e5390b4 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -975,6 +975,7 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, > const char *diskAlias, > int type, > int status, > + const char *error, > void *opaque) > { > virQEMUDriverPtr driver = opaque; > @@ -996,6 +997,9 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, > /* We have a SYNC API waiting for this event, dispatch it back */ > diskPriv->blockJobType = type; > diskPriv->blockJobStatus = status; > + VIR_FREE(diskPriv->blockJobError); > + if (error && VIR_STRDUP_QUIET(diskPriv->blockJobError, error) < 0) > + VIR_WARN("Can not pass error message further: %s", error); > virDomainObjBroadcast(vm); > } else { > /* there is no waiting SYNC API, dispatch the update to a thread */ > And why not augment qemuProcessEvent too? If you did, then the VIR_FREE(diskPriv->blockJobError) would go outside the "if" I would think and the eventual call to qemuBlockJobEventProcess in processBlockJobEvent would be able to manage the error. It's not the data isn't there is it? John