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

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...