[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 08/11] dump-guest-memory: add qmp event DUMP_
From: |
Peter Xu |
Subject: |
Re: [Qemu-devel] [PATCH v4 08/11] dump-guest-memory: add qmp event DUMP_COMPLETED |
Date: |
Wed, 2 Dec 2015 16:20:23 +0800 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
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
> >
- Re: [Qemu-devel] [PATCH v4 03/11] dump-guest-memory: using static DumpState, add DumpStatus, (continued)
- [Qemu-devel] [PATCH v4 04/11] dump-guest-memory: add dump_in_progress() helper function, Peter Xu, 2015/12/01
- [Qemu-devel] [PATCH v4 05/11] dump-guest-memory: introduce dump_process() helper function., Peter Xu, 2015/12/01
- [Qemu-devel] [PATCH v4 06/11] dump-guest-memory: disable dump when in INMIGRATE state, Peter Xu, 2015/12/01
- [Qemu-devel] [PATCH v4 07/11] dump-guest-memory: add "detach" support, Peter Xu, 2015/12/01
- [Qemu-devel] [PATCH v4 08/11] dump-guest-memory: add qmp event DUMP_COMPLETED, Peter Xu, 2015/12/01
- Re: [Qemu-devel] [PATCH v4 08/11] dump-guest-memory: add qmp event DUMP_COMPLETED, Fam Zheng, 2015/12/01
- Re: [Qemu-devel] [PATCH v4 08/11] dump-guest-memory: add qmp event DUMP_COMPLETED, Eric Blake, 2015/12/02
- Re: [Qemu-devel] [PATCH v4 08/11] dump-guest-memory: add qmp event DUMP_COMPLETED, Peter Xu, 2015/12/02
- Re: [Qemu-devel] [PATCH v4 08/11] dump-guest-memory: add qmp event DUMP_COMPLETED, Eric Blake, 2015/12/02
- Re: [Qemu-devel] [PATCH v4 08/11] dump-guest-memory: add qmp event DUMP_COMPLETED, Peter Xu, 2015/12/02
[Qemu-devel] [PATCH v4 09/11] DumpState: adding total_size and written_size fields, Peter Xu, 2015/12/01