[libvirt] [PATCH 2/3] qemu: use blockjob completed event's error field to detect errors

[Nikolay Shirokovskiy]

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: