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

[Peter Xu <peterx@redhat.com>]

On Wed, Dec 02, 2015 at 07:45:52AM -0700, Eric Blake wrote:
> On 12/01/2015 06:11 PM, Fam Zheng wrote:
> > 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.

Will the raw memory total size useful in any way? I am totally ok to
add this, just failed to find a way for user to use it besides
calculating finished work during dump... :(

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

Sorry for the misunderstanding. I will make sure to use a new patch
when needed next time. For this change, I will revert it and take
Fam's latter suggestion to avoid modifying error.c.

Thanks.
Peter

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