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 09:11:31AM +0800, Fam Zheng wrote:
> On Tue, 12/01 21:28, Peter Xu wrote:
> > +
> > +##
> > +# @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.

Ok. I can add more words to describe it. Maybe something like:

# @error: #optional human-readable error string that provides
#         hint on why dump failed. Only presents on failure. The
#         user should not try to interpret the error string.

How do you think about this one?

IMHO, the "#optional" and the name "error" itself is clear enough
though, to show that it will be there only if error happens.

> 
> >  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. 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, in which case the caller need to
> check the pointer anyway. And that is the dominant use case for
> error_get_pretty in the code base.
> 
> IMO the caller can always do this:
> 
>     err ? error_get_pretty(err) : NULL
> 
> in place of your proposed
> 
>     error_get_pretty(err)
> 
> So maybe leave this and change dump_process like above? Or if you insist, make
> this hunk a separate patch please.

I think both should work as long as the modification is backward
compatible and without performance drop (at least, it is
safer). Whatever, since this is the first patch from me, I'd like to
take your advice to avoid modifying shared codes. :)

Thanks.
Peter

> 
> Fam
> 
> >  }
> >  
> >  void error_report_err(Error *err)
> > -- 
> > 2.4.3
> >