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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list