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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3875 bytes --]

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 <peterx@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, ...);

> +++ 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/

>  - "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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]