Re: [Qemu-devel] [PATCH v2 5/8] dump-query: add "dump-query" command to query dump status

[Eric Blake <eblake@redhat.com>]

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

On 11/26/2015 07:48 PM, Peter Xu wrote:
> This patch is only adding the QMP/HMP interface for "dump-query"
> command, but not implementing them. This command could be used to
> query background dump status. Please refer to the next patch to see
> how dump status are defined.
> 
> Currently, only fake results are returned.

Feels a bit awkward to return fake results; maybe you should squash some
patches together or fail with an error instead of returning fake results.

> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  dump.c           |  9 +++++++++
>  hmp-commands.hx  | 15 +++++++++++++++
>  hmp.c            |  6 ++++++
>  hmp.h            |  1 +
>  qapi-schema.json | 21 +++++++++++++++++++++
>  qmp-commands.hx  | 29 ++++++++++++++++++++++++++++-
>  6 files changed, 80 insertions(+), 1 deletion(-)
> 

> +++ b/qapi-schema.json
> @@ -2139,6 +2139,27 @@
>              '*format': 'DumpGuestMemoryFormat'} }
>  
>  ##
> +# @DumpStatus
> +#
> +# Status for the last guest memory dump.
> +#

Missing documentation for 'status' and 'percentage' fields.

> +# Since: 2.6
> +##
> +{ 'struct': 'DumpStatus',
> +  'data': { 'status': 'str', 'percentage': 'str' } }

What values will 'status' contain?  If it is a finite set of status
strings, then it should be an enum type.

'percentage' should NOT be a string.  It should probably be numeric;
'number' if you intend to return a floating point value between 0 and 1.
 Or, like other interfaces, you should probably return two numbers
(current and total, both 'int'), and let the caller compute percentage
themselves.

> +
> +##
> +# @dump-query

Most query commands are named 'query-FOO', not 'FOO-query'.  Unless you
have a compelling reason otherwise, this should be 'query-dump'.

> +#
> +# Query latest dump status.
> +#
> +# Returns: A @DumpStatus object showing the dump status.
> +#
> +# Since: 2.6
> +##
> +{ 'command': 'dump-query', 'returns': 'DumpStatus' }
> +

> +SQMP
> +dump-query
> +----------
> +
> +Query background dump status.
> +
> +Arguments: None.
> +
> +Example:
> +
> +-> { "execute": "dump-query" }
> +<- { "return": {"status": "IN_PROGRESS", "percentage": "85%" } }

ALL_CAPS status is annoying to read; if you add an enum type, it should
be 'lower-case' values.

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