Re: [Qemu-devel] [PATCH v3 02/12] dump-guest-memory: add "detach" flag for QMP/HMP interfaces.

[Peter Xu]

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 <address@hidden>
> > ---
> >  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
>