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

[Zhang Chen]

On Sun, Mar 11, 2018 at 5:33 AM, Eric Blake <address@hidden> wrote:

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



Maybe Paolo or David?  CC paolo and David, any comments about this series?

I have addressed all comments about this patch and sent the PATCH V6,
Thanks your comments and please review it.

Thanks
Zhang Chen




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