Re: [Qemu-devel] [PATCH V5 11/17] qapi: Add new command to query colo status

[Eric Blake]

On 03/10/2018 03:22 PM, Zhang Chen wrote:
> Libvirt or other high level sofware can use this command query colo status.

s/sofware/software/

> You can test this command like that:
> {'execute':'query-colo-status'}
>
> Signed-off-by: Zhang Chen <address@hidden>
> ---
>   migration/colo.c    | 35 +++++++++++++++++++++++++++++++++++
>   qapi/migration.json | 35 +++++++++++++++++++++++++++++++++++
>   2 files changed, 70 insertions(+)

> +++ b/qapi/migration.json
> @@ -1201,3 +1201,38 @@
>   # Since: 2.9
>   ##
>   { 'command': 'xen-colo-do-checkpoint' }
> +
> +##
> +# @COLOStatus:
> +#
> +# The result format for 'query-colo-status'.
> +#
> +# @mode: which COLO mode the VM was in when it exited.
> +#        true is primary mode, false is secondary mode.

Eww. Why not make this an enum ("primary", "secondary") instead of 
having to remember what true means?

> +#
> +# @colo_running: if true means COLO running well, otherwise COLO have done.

New interfaces should use '-' rather than '_' as the separator; this 
should be 'colo-running'

> +#
> +# @reason: describes the reason for the COLO exit.
> +#          true is error, false is user request.

Again, would an enum ("error", "user") be nicer than a bool?  It is also 
more extensible if you add a third reason down the road.

> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'COLOStatus',
> +  'data': { 'mode': 'bool', 'colo_running': 'bool', 'reason': 'bool' } }
> +
> +##
> +# @query-colo-status:
> +#
> +# Query COLO status while the vm is running.
> +#
> +# Returns: A @COLOStatus object showing the status.
> +#
> +# Example:
> +#
> +# -> { "execute": "query-colo-status" }
> +# <- { "return": { "colo_running": "true", "mode": "true", "reason": "true" } }

Inconsistent with your declaration above; if these are truly declared 
'bool', then the example should be (with the spelling fix, but not 
fixing where you should have been using enums instead of bools):

{ "return": { "colo-running": true, "mode": true, "reason": true } }

We're awfully close to soft freeze, and this is a new feature - which 
maintainer will be trying to get this polished and into the tree in time?

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org