Re: [Qemu-devel] [PATCH v4 08/11] dump-guest-memory: add qmp event DUMP_COMPLETED

[Eric Blake]

On 12/01/2015 06:11 PM, Fam Zheng wrote:
> On Tue, 12/01 21:28, Peter Xu wrote:
>> One new QMP event DUMP_COMPLETED is added. When a dump finishes, one
>> DUMP_COMPLETED event will occur to notify the user.
>>
>> Signed-off-by: Peter Xu <address@hidden>
>> ---

>> +++ b/qapi/event.json
>> @@ -356,3 +356,16 @@
>>  ##
>>  { 'event': 'MEM_UNPLUG_ERROR',
>>    'data': { 'device': 'str', 'msg': 'str' } }
>> +
>> +##
>> +# @DUMP_COMPLETED
>> +#
>> +# Emitted when background dump has completed
>> +#
>> +# @error: #optional human-readable error string that provides
>> +#         hint on why dump failed.
> 
> Please explicitly mention that successful dump emits DUMP_COMPLETED without
> error, and failed dump emits DUMP_COMPLETED that has an error str.

In fact, I wonder if it would also be worth having a
'status':'DumpStatus' field, which records the final status of the dump
(either 'completed' or 'failed'), and which is always present.


>> +++ b/util/error.c
>> @@ -197,7 +197,11 @@ ErrorClass error_get_class(const Error *err)
>>  
>>  const char *error_get_pretty(Error *err)
>>  {
>> -    return err->msg;
>> +    if (err) {
>> +        return err->msg;
>> +    } else {
>> +        return NULL;
>> +    }
> 
> This change belongs to a separate patch, if any.

Indeed.  When I was musing about the idea, I was not expecting you to
actually implement it, so much as questioning whether it is a worthwhile
idea.  But as it impacts more than just your series, it definitely needs
to be a separate patch, if at all.

> But personally I don't like
> it, because it doesn't work very well when error_get_pretty is used in
> printf-like function parameters:
> 
>     Error *err = NULL;
>     error_report("error: %s", error_get_pretty(err));
> 
> will print "error: (null)" which is ugly,

Or even segfault.  glibc is nice for printing "(null)", but the behavior
is undefined by POSIX and other libc aren't as nice as glibc.  And that
was not a consequence I thought about when first raising the question of
whether it was even worth changing the contract of error_get_pretty().

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature