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

[Nikolay Shirokovskiy]

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