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 <pet...@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.
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
>
