[libvirt] [PATCH 0/3] 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" Nikolay Shirokovskiy (3): qemu: simplify switch case for blockjob events qemu: use blockjob completed event's error field to detect errors qemu: report block job errors from qemu to the user src/qemu/qemu_blockjob.c | 13 +++++++++++-- src/qemu/qemu_blockjob.h | 3 ++- src/qemu/qemu_domain.h | 1 + src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_migration.c | 34 ++++++++++++++++++++++------------ src/qemu/qemu_monitor.c | 5 +++-- src/qemu/qemu_monitor.h | 4 +++- src/qemu/qemu_monitor_json.c | 12 +++++------- src/qemu/qemu_process.c | 3 +++ 9 files changed, 52 insertions(+), 27 deletions(-) -- 1.8.3.1
--- src/qemu/qemu_monitor_json.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e1494df..6c2884d 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -841,10 +841,8 @@ qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon, case VIR_DOMAIN_BLOCK_JOB_CANCELED: case VIR_DOMAIN_BLOCK_JOB_READY: break; - case VIR_DOMAIN_BLOCK_JOB_FAILED: - case VIR_DOMAIN_BLOCK_JOB_LAST: - VIR_DEBUG("should not get here"); - break; + default: + VIR_WARN("unexpected block job type: %d", event); } out: -- 1.8.3.1
On Wed, Oct 05, 2016 at 16:52:08 +0300, Nikolay Shirokovskiy wrote:
--- src/qemu/qemu_monitor_json.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e1494df..6c2884d 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -841,10 +841,8 @@ qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon, case VIR_DOMAIN_BLOCK_JOB_CANCELED: case VIR_DOMAIN_BLOCK_JOB_READY: break; - case VIR_DOMAIN_BLOCK_JOB_FAILED: - case VIR_DOMAIN_BLOCK_JOB_LAST: - VIR_DEBUG("should not get here"); - break; + default: + VIR_WARN("unexpected block job type: %d", event); }
NACK, we intentionally don't use default branches. Default switch branch disables GCC warnings in case any enum item is not handled. Runtime warnings are invisible while compile time warnings are very easy to spot. Jirka
On 05.10.2016 17:36, Jiri Denemark wrote:
On Wed, Oct 05, 2016 at 16:52:08 +0300, Nikolay Shirokovskiy wrote:
--- src/qemu/qemu_monitor_json.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e1494df..6c2884d 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -841,10 +841,8 @@ qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon, case VIR_DOMAIN_BLOCK_JOB_CANCELED: case VIR_DOMAIN_BLOCK_JOB_READY: break; - case VIR_DOMAIN_BLOCK_JOB_FAILED: - case VIR_DOMAIN_BLOCK_JOB_LAST: - VIR_DEBUG("should not get here"); - break; + default: + VIR_WARN("unexpected block job type: %d", event); }
NACK, we intentionally don't use default branches. Default switch branch disables GCC warnings in case any enum item is not handled.
Runtime warnings are invisible while compile time warnings are very easy to spot.
Ok, thanks for the explanation. Let's drop it. Nikolay
BLOCK_JOB_COMPLETED has error field set on error from day one (12bd451f) thus there is no need to guess for error. Is it true that when len == offset then can be no error? --- src/qemu/qemu_monitor_json.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 6c2884d..8218e12 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -801,6 +801,7 @@ qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon, int event) { const char *device; + const char *error = NULL; const char *type_str; int type = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; unsigned long long offset, len; @@ -834,8 +835,7 @@ qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon, switch ((virConnectDomainEventBlockJobStatus) event) { case VIR_DOMAIN_BLOCK_JOB_COMPLETED: - /* Make sure the whole device has been processed */ - if (offset != len) + if ((error = virJSONValueObjectGetString(data, "error"))) event = VIR_DOMAIN_BLOCK_JOB_FAILED; break; case VIR_DOMAIN_BLOCK_JOB_CANCELED: -- 1.8.3.1
On Wed, Oct 05, 2016 at 16:52:09 +0300, Nikolay Shirokovskiy wrote:
BLOCK_JOB_COMPLETED has error field set on error from day one (12bd451f) thus there is no need to guess for error. Is it true that when len == offset then can be no error?
That is the question you should be able to answer when sending a patch, not a content for the commit message. AFAIK a copy job can fail even after all data was copied while syncing the buffers so this change makes sense. Additionally qemu also supports the BLOCK_JOB_ERROR event which is currently not handled by qemu. I'll need to have a look into it.
--- src/qemu/qemu_monitor_json.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 6c2884d..8218e12 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -801,6 +801,7 @@ qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon, int event) { const char *device; + const char *error = NULL; const char *type_str; int type = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; unsigned long long offset, len; @@ -834,8 +835,7 @@ qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon,
switch ((virConnectDomainEventBlockJobStatus) event) { case VIR_DOMAIN_BLOCK_JOB_COMPLETED: - /* Make sure the whole device has been processed */ - if (offset != len) + if ((error = virJSONValueObjectGetString(data, "error")))
I'd prefer if you'd keep the original check as long as adding the check for the error field. The 'error' variable is not used besides setting it twice, so you apparently don't need it at all.
event = VIR_DOMAIN_BLOCK_JOB_FAILED; break; case VIR_DOMAIN_BLOCK_JOB_CANCELED:
On 06.10.2016 11:02, Peter Krempa wrote:
On Wed, Oct 05, 2016 at 16:52:09 +0300, Nikolay Shirokovskiy wrote:
BLOCK_JOB_COMPLETED has error field set on error from day one (12bd451f) thus there is no need to guess for error. Is it true that when len == offset then can be no error?
That is the question you should be able to answer when sending a patch, not a content for the commit message.
AFAIK a copy job can fail even after all data was copied while syncing the buffers so this change makes sense.
Additionally qemu also supports the BLOCK_JOB_ERROR event which is currently not handled by qemu. I'll need to have a look into it.
--- src/qemu/qemu_monitor_json.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 6c2884d..8218e12 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -801,6 +801,7 @@ qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon, int event) { const char *device; + const char *error = NULL; const char *type_str; int type = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; unsigned long long offset, len; @@ -834,8 +835,7 @@ qemuMonitorJSONHandleBlockJobImpl(qemuMonitorPtr mon,
switch ((virConnectDomainEventBlockJobStatus) event) { case VIR_DOMAIN_BLOCK_JOB_COMPLETED: - /* Make sure the whole device has been processed */ - if (offset != len) + if ((error = virJSONValueObjectGetString(data, "error")))
I'd prefer if you'd keep the original check as long as adding the check for the error field.
Like for safety reasons? Ok.
The 'error' variable is not used besides setting it twice, so you apparently don't need it at all.
This is intentional. Or next patch will touch this place one more time. If it does't matter then ok.
event = VIR_DOMAIN_BLOCK_JOB_FAILED; break; case VIR_DOMAIN_BLOCK_JOB_CANCELED:
--- src/qemu/qemu_blockjob.c | 13 +++++++++++-- src/qemu/qemu_blockjob.h | 3 ++- src/qemu/qemu_domain.h | 1 + src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_migration.c | 34 ++++++++++++++++++++++------------ src/qemu/qemu_monitor.c | 5 +++-- src/qemu/qemu_monitor.h | 4 +++- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_process.c | 3 +++ 9 files changed, 48 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 83a5a3f..937d931 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -33,6 +33,7 @@ #include "virstoragefile.h" #include "virthread.h" #include "virtime.h" +#include "viralloc.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -53,16 +54,24 @@ VIR_LOG_INIT("qemu.qemu_blockjob"); int qemuBlockJobUpdate(virQEMUDriverPtr driver, virDomainObjPtr vm, - 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, diskPriv->blockJobType, diskPriv->blockJobStatus); diskPriv->blockJobStatus = -1; + if (error) + VIR_STEAL_PTR(*error, diskPriv->blockJobHint); + else + VIR_FREE(diskPriv->blockJobHint); } return status; @@ -241,6 +250,6 @@ qemuBlockJobSyncEnd(virQEMUDriverPtr driver, virDomainDiskDefPtr disk) { VIR_DEBUG("disk=%s", disk->dst); - qemuBlockJobUpdate(driver, vm, disk); + qemuBlockJobUpdate(driver, vm, disk, NULL); QEMU_DOMAIN_DISK_PRIVATE(disk)->blockJobSync = false; } diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h index 775ce95..c452edc 100644 --- a/src/qemu/qemu_blockjob.h +++ b/src/qemu/qemu_blockjob.h @@ -27,7 +27,8 @@ int qemuBlockJobUpdate(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDiskDefPtr disk); + virDomainDiskDefPtr disk, + char **error); void qemuBlockJobEventProcess(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 521531b..79d88fa 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -290,6 +290,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 *blockJobHint; /* hint from block job event (like error message) */ bool blockJobSync; /* the block job needs synchronized termination */ bool migrating; /* the disk is being migrated */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ee16cb5..ac204c3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16280,13 +16280,13 @@ qemuDomainBlockJobAbort(virDomainPtr dom, VIR_DOMAIN_BLOCK_JOB_CANCELED); } else { qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - qemuBlockJobUpdate(driver, vm, disk); + qemuBlockJobUpdate(driver, vm, disk, NULL); while (diskPriv->blockjob) { if (virDomainObjWait(vm) < 0) { ret = -1; goto endjob; } - qemuBlockJobUpdate(driver, vm, disk); + qemuBlockJobUpdate(driver, vm, disk, NULL); } } } diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 8a220d9..0671e23 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1853,17 +1853,20 @@ 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; if (!diskPriv->migrating) continue; - status = qemuBlockJobUpdate(driver, vm, disk); + status = qemuBlockJobUpdate(driver, vm, disk, &error); if (status == VIR_DOMAIN_BLOCK_JOB_FAILED) { virReportError(VIR_ERR_OPERATION_FAILED, - _("migration of disk %s failed"), - disk->dst); + _("migration of disk %s failed: %s"), + disk->dst, NULLSTR(error)); + VIR_FREE(error); return -1; } + VIR_FREE(error); if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY) notReady++; @@ -1902,17 +1905,18 @@ 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; if (!diskPriv->migrating) continue; - 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); + _("migration of disk %s failed: %s"), + disk->dst, NULLSTR(error)); failed = true; } /* fallthrough */ @@ -1925,6 +1929,7 @@ qemuMigrationDriveMirrorCancelled(virQEMUDriverPtr driver, default: active++; } + VIR_FREE(error); } if (failed) { @@ -1962,24 +1967,28 @@ 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; + _("migration of disk %s failed: %s"), + disk->dst, NULLSTR(error)); + ret = -1; + goto cleanup; } - return 1; + ret = 1; + goto cleanup; case VIR_DOMAIN_BLOCK_JOB_COMPLETED: - return 1; + ret = 1; + goto cleanup; } if (!(diskAlias = qemuAliasFromDisk(disk))) @@ -1997,6 +2006,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 8083a36..3466225 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 *hint) { int ret = -1; VIR_DEBUG("mon=%p", mon); QEMU_MONITOR_CALLBACK(mon, ret, domainBlockJob, mon->vm, - diskAlias, type, status); + diskAlias, type, status, hint); return ret; } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 7d78e5b..a5d8181 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 *hint, 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 *hint); 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 8218e12..55b3ed8 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -846,7 +846,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 27d04a4..4638994 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 *hint, void *opaque) { virQEMUDriverPtr driver = opaque; @@ -996,6 +997,8 @@ 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->blockJobHint); + ignore_value(VIR_STRDUP_QUIET(diskPriv->blockJobHint, hint)); virDomainObjBroadcast(vm); } else { /* there is no waiting SYNC API, dispatch the update to a thread */ -- 1.8.3.1
On Wed, Oct 05, 2016 at 16:52:10 +0300, Nikolay Shirokovskiy wrote:
--- src/qemu/qemu_blockjob.c | 13 +++++++++++-- src/qemu/qemu_blockjob.h | 3 ++- src/qemu/qemu_domain.h | 1 + src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_migration.c | 34 ++++++++++++++++++++++------------ src/qemu/qemu_monitor.c | 5 +++-- src/qemu/qemu_monitor.h | 4 +++- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_process.c | 3 +++ 9 files changed, 48 insertions(+), 21 deletions(-)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 83a5a3f..937d931 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -33,6 +33,7 @@ #include "virstoragefile.h" #include "virthread.h" #include "virtime.h" +#include "viralloc.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -53,16 +54,24 @@ VIR_LOG_INIT("qemu.qemu_blockjob"); int qemuBlockJobUpdate(virQEMUDriverPtr driver, virDomainObjPtr vm, - 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, diskPriv->blockJobType, diskPriv->blockJobStatus); diskPriv->blockJobStatus = -1; + if (error) + VIR_STEAL_PTR(*error, diskPriv->blockJobHint); + else + VIR_FREE(diskPriv->blockJobHint);
What if qemuBlockJobUpdate is never called? Shouldn't qemuBlockJobEventProcess be the place to free the error?
}
return status; @@ -241,6 +250,6 @@ qemuBlockJobSyncEnd(virQEMUDriverPtr driver, virDomainDiskDefPtr disk) { VIR_DEBUG("disk=%s", disk->dst); - qemuBlockJobUpdate(driver, vm, disk); + qemuBlockJobUpdate(driver, vm, disk, NULL); QEMU_DOMAIN_DISK_PRIVATE(disk)->blockJobSync = false; } diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h index 775ce95..c452edc 100644 --- a/src/qemu/qemu_blockjob.h +++ b/src/qemu/qemu_blockjob.h @@ -27,7 +27,8 @@
int qemuBlockJobUpdate(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDiskDefPtr disk); + virDomainDiskDefPtr disk, + char **error); void qemuBlockJobEventProcess(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 521531b..79d88fa 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -290,6 +290,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 *blockJobHint; /* hint from block job event (like error message) */
So why is this called blockJobHint if it's used for storing the errors? I think blockJobError would be a better name... And you forgot to free it in qemuDomainDiskPrivateDispose.
bool blockJobSync; /* the block job needs synchronized termination */
bool migrating; /* the disk is being migrated */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ee16cb5..ac204c3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16280,13 +16280,13 @@ qemuDomainBlockJobAbort(virDomainPtr dom, VIR_DOMAIN_BLOCK_JOB_CANCELED); } else { qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - qemuBlockJobUpdate(driver, vm, disk); + qemuBlockJobUpdate(driver, vm, disk, NULL); while (diskPriv->blockjob) { if (virDomainObjWait(vm) < 0) { ret = -1; goto endjob; } - qemuBlockJobUpdate(driver, vm, disk); + qemuBlockJobUpdate(driver, vm, disk, NULL); } } } diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 8a220d9..0671e23 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1853,17 +1853,20 @@ 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;
if (!diskPriv->migrating) continue;
- status = qemuBlockJobUpdate(driver, vm, disk); + status = qemuBlockJobUpdate(driver, vm, disk, &error); if (status == VIR_DOMAIN_BLOCK_JOB_FAILED) { virReportError(VIR_ERR_OPERATION_FAILED, - _("migration of disk %s failed"), - disk->dst); + _("migration of disk %s failed: %s"), + disk->dst, NULLSTR(error));
I think we'll need two error messages depending on whether error is NULL. Reporting "migration of disk vda failed: <null>" is not a good idea.
+ VIR_FREE(error); return -1; } + VIR_FREE(error);
if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY) notReady++; @@ -1902,17 +1905,18 @@ 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;
if (!diskPriv->migrating) continue;
- 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); + _("migration of disk %s failed: %s"), + disk->dst, NULLSTR(error));
Ditto.
failed = true; } /* fallthrough */ @@ -1925,6 +1929,7 @@ qemuMigrationDriveMirrorCancelled(virQEMUDriverPtr driver, default: active++; } + VIR_FREE(error); }
if (failed) { @@ -1962,24 +1967,28 @@ 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; + _("migration of disk %s failed: %s"), + disk->dst, NULLSTR(error));
Ditto.
+ ret = -1;
No need to set ret = -1.
+ goto cleanup; } - return 1; + ret = 1; + goto cleanup;
I think we should just let it fall through to the next case...
case VIR_DOMAIN_BLOCK_JOB_COMPLETED: - return 1; + ret = 1; + goto cleanup; }
if (!(diskAlias = qemuAliasFromDisk(disk))) @@ -1997,6 +2006,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 8083a36..3466225 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 *hint)
s/hint/error/ here and in the rest of the patch. Jirka
On 06.10.2016 11:01, Jiri Denemark wrote:
On Wed, Oct 05, 2016 at 16:52:10 +0300, Nikolay Shirokovskiy wrote:
--- src/qemu/qemu_blockjob.c | 13 +++++++++++-- src/qemu/qemu_blockjob.h | 3 ++- src/qemu/qemu_domain.h | 1 + src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_migration.c | 34 ++++++++++++++++++++++------------ src/qemu/qemu_monitor.c | 5 +++-- src/qemu/qemu_monitor.h | 4 +++- src/qemu/qemu_monitor_json.c | 2 +- src/qemu/qemu_process.c | 3 +++ 9 files changed, 48 insertions(+), 21 deletions(-)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 83a5a3f..937d931 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -33,6 +33,7 @@ #include "virstoragefile.h" #include "virthread.h" #include "virtime.h" +#include "viralloc.h"
#define VIR_FROM_THIS VIR_FROM_QEMU
@@ -53,16 +54,24 @@ VIR_LOG_INIT("qemu.qemu_blockjob"); int qemuBlockJobUpdate(virQEMUDriverPtr driver, virDomainObjPtr vm, - 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, diskPriv->blockJobType, diskPriv->blockJobStatus); diskPriv->blockJobStatus = -1; + if (error) + VIR_STEAL_PTR(*error, diskPriv->blockJobHint); + else + VIR_FREE(diskPriv->blockJobHint);
What if qemuBlockJobUpdate is never called? Shouldn't qemuBlockJobEventProcess be the place to free the error?
blockJobHint is used only in block job "sync" mode, thus user will call qemuBlockJobSyncEnd and it will take care.
}
return status; @@ -241,6 +250,6 @@ qemuBlockJobSyncEnd(virQEMUDriverPtr driver, virDomainDiskDefPtr disk) { VIR_DEBUG("disk=%s", disk->dst); - qemuBlockJobUpdate(driver, vm, disk); + qemuBlockJobUpdate(driver, vm, disk, NULL); QEMU_DOMAIN_DISK_PRIVATE(disk)->blockJobSync = false; } diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h index 775ce95..c452edc 100644 --- a/src/qemu/qemu_blockjob.h +++ b/src/qemu/qemu_blockjob.h @@ -27,7 +27,8 @@
int qemuBlockJobUpdate(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDiskDefPtr disk); + virDomainDiskDefPtr disk, + char **error); void qemuBlockJobEventProcess(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 521531b..79d88fa 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -290,6 +290,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 *blockJobHint; /* hint from block job event (like error message) */
So why is this called blockJobHint if it's used for storing the errors? I think blockJobError would be a better name...
Different qemu blockjob events use the same interface on the notification path. Ready, cancelled, failed, complete. I doubt 'error' can be applied to any of them except "failed", so I decided choose a more neutral name. It's like void* in namespace of variable names )) Anyway it is not that principal, I just wanted to explain my POV Nikolay
participants (3)
-
Jiri Denemark -
Nikolay Shirokovskiy -
Peter Krempa