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

[Peter Xu]

On Mon, Nov 30, 2015 at 11:27:35AM -0700, Eric Blake wrote:
> 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.

Yes, sorry for that. This was pointed out by Paolo and it's fixed in v3.

> 
> > 
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> >  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.

Yes. Fixed in v3. Thanks!

> 
> > +# 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.

Sorry for the old unclear API. Now it contains:

- status: enum
- written_bytes: int
- total_bytes: int

These were modified in v3 (as Fam & Paolo suggested). Thanks!

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

Changed in v3.

> 
> > +#
> > +# 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.

Changed in v3.

Thanks!
Peter

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>