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