[libvirt] [PATCH v3 REBASE v2 0/2] qemu: report block job errors from qemu to the user
So that you can see nice report on migration: "error: operation failed: migration of disk sda failed: No space left on device" diff from v2: ============ 1. split into 2 patches 2. change formal documentation where it is present accordingly 3. add variable initialization for safety Nikolay Shirokovskiy (2): qemu: prepare blockjob complete event error usage qemu: report drive mirror errors on migration src/qemu/qemu_blockjob.c | 14 +++++++++-- src/qemu/qemu_blockjob.h | 3 ++- src/qemu/qemu_domain.c | 10 +++++++- src/qemu/qemu_domain.h | 1 + src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_migration.c | 55 +++++++++++++++++++++++++++++++------------- src/qemu/qemu_monitor.c | 5 ++-- src/qemu/qemu_monitor.h | 4 +++- src/qemu/qemu_monitor_json.c | 4 +++- src/qemu/qemu_process.c | 4 ++++ 10 files changed, 78 insertions(+), 26 deletions(-) -- 1.8.3.1
This patch pass event error up to the place where we can use it. Error is passed only for sync blockjob event mode as we can't use the error in async mode. In async mode we just pass the event details to the client thru event API but current blockjob event API can not carry extra parameter. --- src/qemu/qemu_blockjob.c | 2 ++ src/qemu/qemu_domain.c | 10 +++++++++- src/qemu/qemu_domain.h | 1 + src/qemu/qemu_monitor.c | 5 +++-- src/qemu/qemu_monitor.h | 4 +++- src/qemu/qemu_monitor_json.c | 4 +++- src/qemu/qemu_process.c | 4 ++++ 7 files changed, 25 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index c1b46f7..a85ae94 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -35,6 +35,7 @@ #include "virthread.h" #include "virtime.h" #include "locking/domain_lock.h" +#include "viralloc.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -66,6 +67,7 @@ qemuBlockJobUpdate(virQEMUDriverPtr driver, diskPriv->blockJobType, diskPriv->blockJobStatus); diskPriv->blockJobStatus = -1; + VIR_FREE(diskPriv->blockJobError); } return status; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 13e77ee..7298e9e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -891,6 +891,7 @@ qemuDomainSecretInfoFree(qemuDomainSecretInfoPtr *secinfo) static virClassPtr qemuDomainDiskPrivateClass; +static void qemuDomainDiskPrivateDispose(void *obj); static int qemuDomainDiskPrivateOnceInit(void) @@ -898,7 +899,7 @@ qemuDomainDiskPrivateOnceInit(void) qemuDomainDiskPrivateClass = virClassNew(virClassForObject(), "qemuDomainDiskPrivate", sizeof(qemuDomainDiskPrivate), - NULL); + qemuDomainDiskPrivateDispose); if (!qemuDomainDiskPrivateClass) return -1; else @@ -921,6 +922,13 @@ qemuDomainDiskPrivateNew(void) return (virObjectPtr) priv; } +static void +qemuDomainDiskPrivateDispose(void *obj) +{ + qemuDomainDiskPrivatePtr priv = obj; + + VIR_FREE(priv->blockJobError); +} static virClassPtr qemuDomainStorageSourcePrivateClass; static void qemuDomainStorageSourcePrivateDispose(void *obj); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index ff53282..735e810 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -350,6 +350,7 @@ struct _qemuDomainDiskPrivate { /* for some synchronous block jobs, we need to notify the owner */ int blockJobType; /* type of the block job from the event */ int blockJobStatus; /* status of the finished block job */ + char *blockJobError; /* block job completed event error */ bool blockJobSync; /* the block job needs synchronized termination */ bool migrating; /* the disk is being migrated */ diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index dd9d64a..a5e95be 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1515,13 +1515,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 bc8494f..cbe5fa8 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -176,6 +176,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, @@ -375,7 +376,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 05cc634..53be827 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -851,6 +851,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; @@ -883,6 +884,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; @@ -897,7 +899,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 fdc8689..4bfad5d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1000,6 +1000,7 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, const char *diskAlias, int type, int status, + const char *error, void *opaque) { virQEMUDriverPtr driver = opaque; @@ -1021,6 +1022,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 */ -- 1.8.3.1
On Fri, Oct 27, 2017 at 15:37:22 +0300, Nikolay Shirokovskiy wrote:
This patch pass event error up to the place where we can use it. Error is passed only for sync blockjob event mode as we can't use the error in async mode. In async mode we just pass the event details to the client thru event API but current blockjob event API can not carry extra parameter. ... diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index fdc8689..4bfad5d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1000,6 +1000,7 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, const char *diskAlias, int type, int status, + const char *error, void *opaque) { virQEMUDriverPtr driver = opaque; @@ -1021,6 +1022,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);
Checking error is not needed, you can just VIR_STRDUP it (NULL input is ignored without triggering an error). And I think we don't even need to emit the warning, we just won't have any error if there's not enough memory for making a copy of it. In other words, I think VIR_FREE(diskPriv->blockJobError); ignore_value(VIR_STRDUP_QUIET(diskPriv->blockJobError, error)); Would be sufficient. ACK with this small change. Jirka
--- src/qemu/qemu_blockjob.c | 14 +++++++++--- src/qemu/qemu_blockjob.h | 3 ++- src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_migration.c | 55 +++++++++++++++++++++++++++++++++-------------- 4 files changed, 54 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index a85ae94..0b1616a 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -47,6 +47,7 @@ VIR_LOG_INIT("qemu.qemu_blockjob"); * @driver: qemu driver * @vm: domain * @disk: domain disk + * @error: error (output parameter) * * Update disk's mirror state in response to a block job event stored in * blockJobStatus by qemuProcessHandleBlockJob event handler. @@ -57,17 +58,24 @@ int qemuBlockJobUpdate(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob asyncJob, - virDomainDiskDefPtr disk) + virDomainDiskDefPtr disk, + char **error) { qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); int status = diskPriv->blockJobStatus; + if (error) + *error = NULL; + if (status != -1) { qemuBlockJobEventProcess(driver, vm, disk, asyncJob, diskPriv->blockJobType, diskPriv->blockJobStatus); diskPriv->blockJobStatus = -1; - VIR_FREE(diskPriv->blockJobError); + if (error) + VIR_STEAL_PTR(*error, diskPriv->blockJobError); + else + VIR_FREE(diskPriv->blockJobError); } return status; @@ -249,6 +257,6 @@ qemuBlockJobSyncEnd(virQEMUDriverPtr driver, virDomainDiskDefPtr disk) { VIR_DEBUG("disk=%s", disk->dst); - qemuBlockJobUpdate(driver, vm, asyncJob, disk); + qemuBlockJobUpdate(driver, vm, asyncJob, disk, NULL); QEMU_DOMAIN_DISK_PRIVATE(disk)->blockJobSync = false; } diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h index 47aa4c1..e71d691 100644 --- a/src/qemu/qemu_blockjob.h +++ b/src/qemu/qemu_blockjob.h @@ -29,7 +29,8 @@ int qemuBlockJobUpdate(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob asyncJob, - virDomainDiskDefPtr disk); + virDomainDiskDefPtr disk, + char **error); void qemuBlockJobEventProcess(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 74fdfdb..300ae85 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16728,13 +16728,13 @@ qemuDomainBlockJobAbort(virDomainPtr dom, * block jobs from confusing us. */ if (!async) { qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - qemuBlockJobUpdate(driver, vm, QEMU_ASYNC_JOB_NONE, disk); + qemuBlockJobUpdate(driver, vm, QEMU_ASYNC_JOB_NONE, disk, NULL); while (diskPriv->blockjob) { if (virDomainObjWait(vm) < 0) { ret = -1; goto endjob; } - qemuBlockJobUpdate(driver, vm, QEMU_ASYNC_JOB_NONE, disk); + qemuBlockJobUpdate(driver, vm, QEMU_ASYNC_JOB_NONE, disk, NULL); } } diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index af74466..f9c3b41 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -611,17 +611,25 @@ qemuMigrationDriveMirrorReady(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 = NULL; if (!diskPriv->migrating) continue; - status = qemuBlockJobUpdate(driver, vm, asyncJob, disk); + status = qemuBlockJobUpdate(driver, vm, asyncJob, disk, &error); if (status == VIR_DOMAIN_BLOCK_JOB_FAILED) { - 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); + VIR_FREE(error); + } else { + virReportError(VIR_ERR_OPERATION_FAILED, + _("migration of disk %s failed"), disk->dst); + } return -1; } + VIR_FREE(error); if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY) notReady++; @@ -663,17 +671,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 = NULL; if (!diskPriv->migrating) continue; - status = qemuBlockJobUpdate(driver, vm, asyncJob, disk); + status = qemuBlockJobUpdate(driver, vm, asyncJob, 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; } ATTRIBUTE_FALLTHROUGH; @@ -689,6 +703,8 @@ qemuMigrationDriveMirrorCancelled(virQEMUDriverPtr driver, if (status == VIR_DOMAIN_BLOCK_JOB_COMPLETED) completed++; + + VIR_FREE(error); } /* Updating completed block job drops the lock thus we have to recheck @@ -736,24 +752,30 @@ qemuMigrationCancelOneDriveMirror(virQEMUDriverPtr driver, { qemuDomainObjPrivatePtr priv = vm->privateData; char *diskAlias = NULL; + char *error = NULL; int ret = -1; int status; int rv; - status = qemuBlockJobUpdate(driver, vm, asyncJob, disk); + status = qemuBlockJobUpdate(driver, vm, asyncJob, 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))) @@ -771,6 +793,7 @@ qemuMigrationCancelOneDriveMirror(virQEMUDriverPtr driver, cleanup: VIR_FREE(diskAlias); + VIR_FREE(error); return ret; } -- 1.8.3.1
On Fri, Oct 27, 2017 at 15:37:23 +0300, Nikolay Shirokovskiy wrote:
--- src/qemu/qemu_blockjob.c | 14 +++++++++--- src/qemu/qemu_blockjob.h | 3 ++- src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_migration.c | 55 +++++++++++++++++++++++++++++++++-------------- 4 files changed, 54 insertions(+), 22 deletions(-) ... @@ -736,24 +752,30 @@ qemuMigrationCancelOneDriveMirror(virQEMUDriverPtr driver, { qemuDomainObjPrivatePtr priv = vm->privateData; char *diskAlias = NULL; + char *error = NULL; int ret = -1; int status; int rv;
- status = qemuBlockJobUpdate(driver, vm, asyncJob, disk); + status = qemuBlockJobUpdate(driver, vm, asyncJob, 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 */
Use ATTRIBUTE_FALLTHROUGH instead of this comment.
case VIR_DOMAIN_BLOCK_JOB_COMPLETED: - return 1; + ret = 1; + goto cleanup; }
if (!(diskAlias = qemuAliasFromDisk(disk))) @@ -771,6 +793,7 @@ qemuMigrationCancelOneDriveMirror(virQEMUDriverPtr driver,
cleanup: VIR_FREE(diskAlias); + VIR_FREE(error); return ret; }
ACK with the small change. I'll push this series after the release. Jirka
ping On 27.10.2017 15:37, Nikolay Shirokovskiy wrote:
So that you can see nice report on migration:
"error: operation failed: migration of disk sda failed: No space left on device"
diff from v2: ============ 1. split into 2 patches 2. change formal documentation where it is present accordingly 3. add variable initialization for safety
Nikolay Shirokovskiy (2): qemu: prepare blockjob complete event error usage qemu: report drive mirror errors on migration
src/qemu/qemu_blockjob.c | 14 +++++++++-- src/qemu/qemu_blockjob.h | 3 ++- src/qemu/qemu_domain.c | 10 +++++++- src/qemu/qemu_domain.h | 1 + src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_migration.c | 55 +++++++++++++++++++++++++++++++------------- src/qemu/qemu_monitor.c | 5 ++-- src/qemu/qemu_monitor.h | 4 +++- src/qemu/qemu_monitor_json.c | 4 +++- src/qemu/qemu_process.c | 4 ++++ 10 files changed, 78 insertions(+), 26 deletions(-)
participants (2)
-
Jiri Denemark -
Nikolay Shirokovskiy