On Mon, Nov 30, 2015 at 03:05:10PM -0700, Eric Blake wrote:
> On 11/30/2015 04:32 AM, Peter Xu wrote:
> > This patch only adds the interfaces, but not implements them.
> > "detach" parameter is made optional, to make sure that all the old
> > dump-guest-memory requests will still be able to work.
> >
> > Signed-off-by: Peter Xu <pet...@redhat.com>
> > ---
> > dump.c | 5 +++--
> > hmp-commands.hx | 5 +++--
> > hmp.c | 9 +++++++--
> > qapi-schema.json | 8 ++++++--
> > qmp-commands.hx | 6 ++++--
> > 5 files changed, 23 insertions(+), 10 deletions(-)
>
> > +++ b/hmp.c
> > @@ -1586,8 +1586,10 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict
> > *qdict)
> > const char *file = qdict_get_str(qdict, "filename");
> > bool has_begin = qdict_haskey(qdict, "begin");
> > bool has_length = qdict_haskey(qdict, "length");
> > + bool has_detach = qdict_haskey(qdict, "detach");
>
> Here, you probe whether 'detach' is present...
>
> > int64_t begin = 0;
> > int64_t length = 0;
> > + bool detach = false;
>
> ...here, you default 'detach' to false,..
>
> > enum DumpGuestMemoryFormat dump_format = DUMP_GUEST_MEMORY_FORMAT_ELF;
> > char *prot;
> >
> > @@ -1615,11 +1617,14 @@ void hmp_dump_guest_memory(Monitor *mon, const
> > QDict *qdict)
> > if (has_length) {
> > length = qdict_get_int(qdict, "length");
> > }
> > + if (has_detach) {
> > + detach = qdict_get_try_bool(qdict, "detach", false);
>
> ...therefore, this line is only reachable if 'detach' is present, which
> means the default will never be needed.
>
> > + }
> >
> > prot = g_strconcat("file:", file, NULL);
> >
> > - qmp_dump_guest_memory(paging, prot, has_begin, begin, has_length,
> > length,
> > - true, dump_format, &err);
> > + qmp_dump_guest_memory(paging, prot, has_detach, detach, has_begin,
> > begin,
> > + has_length, length, true, dump_format, &err);
>
> There are two competing ways to simplify this; I don't care which one
> you choose, although you can't do both:
>
> 1. Note that since the second assignment to detach only happens if
> has_detach is true, you can use the simpler:
>
> if (has_detach) {
> detach = qdict_get_bool(qdict, "detach");
> }
>
> If you go with this approach, you could even get away without
> initializing 'detach' outside of the 'if (has_detach)' conditional
> (although it's still probably wiser to avoid uninitialized memory than
> to rely on qmp_dump_guest_memory() correctly ignoring 'detach' when
> 'has_detach' is false).
>
> 2. Note that since qdict_get_try_bool() lets you specify a default, you
> can eliminate the has_detach variable and instead just do:
>
> bool detach = qdict_get_try_bool(qdict, "detach", false);
> ...
> qmp_dump_guest_memory(paging, prot, true, detach, ...);
Yes, the default is lost. Thanks.
I think (2) is better in term of lines of codes (and also
clear). However I may need to keep the QMP interface (to keep the
has_detach parameter in qmp_dump_guest_memory), so I'd like to
choose (1).
>
> > +++ b/qapi-schema.json
> > @@ -2115,6 +2115,9 @@
> > # 2. fd: the protocol starts with "fd:", and the following
> > string
> > # is the fd's name.
> > #
> > +# @detach: #optional if true, QMP will return immediately rather than
> > +# waiting dump to be finished (since 2.6).
>
> s/dump/for the dump/
> s/be finished/finish/
>
> > @@ -857,6 +857,8 @@ Arguments:
> > - "paging": do paging to get guest's memory mapping (json-bool)
> > - "protocol": destination file(started with "file:") or destination file
> > descriptor (started with "fd:") (json-string)
> > +- "detach": if specificed, command will return immediately, without waiting
>
> s/specificed/specified/
>
> > + for dump to be finished (json-bool)
>
> s/dump/the dump/
> s/be finished/finish/
Thanks for review again on the English!
Peter
>
> > - "begin": the starting physical address. It's optional, and should be
> > specified
> > with length together (json-int)
> > - "length": the memory size, in bytes. It's optional, and should be
> > specified
> >
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
