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
>
