Re: [Qemu-devel] [PATCH V8 2/2] Add a new qmp command to do checkpoint, query xen replication status

[Eric Blake]

On 03/03/2017 07:45 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> On 02/23/2017 01:14 AM, Zhang Chen wrote:
>>> We can call this qmp command to do checkpoint outside of qemu.
>>> Xen colo will need this function.
>>>
>>> Signed-off-by: Zhang Chen <address@hidden>
>>> Signed-off-by: Wen Congyang <address@hidden>
>>> Reviewed-by: Eric Blake <address@hidden>
>>
>> You made a substantial change to this patch since v7 in response to my
>> comments; when you do that, it's best to remove the R-b to make sure I
>> re-review the changes and am still happy with them.
>>

>>> +# @ReplicationStatus:
>>> +#
>>> +# The result format for 'query-xen-replication-status'.
>>> +#
>>> +# @status: true to error, false to normal.
>>
>> This is now a poor name for the parameter.  Please rename it; probably
>> to @error (if you want to keep true meaning a problem has been
>> detected), or to @okay (if you want to invert the sense, and @desc is
>> only present when @okay is false).
> 
> We could replace both members by '*error': 'str', present exactly when
> status is "bad".
> 
>>> +#
>>> +# @desc: #optional the human readable error description string, when
>>> +#        @status is 'true'.
>>> +#
>>> +# Since: 2.9
>>> +##
>>> +{ 'struct': 'ReplicationStatus',
>>> +  'data': { 'status': 'bool', '*desc': 'str' } }

Except that ReplicationStatus would then be the empty struct on success.
 I guess that's okay, but seems a bit odd.


>>> +# Example:
>>> +#
>>> +# -> { "execute": "query-xen-replication-status" }
>>> +# <- { "return": { "status": "normal" } }
>>
>> This example is now wrong.
>>
>> You'll need a v9.

which has already been submitted and turned into a pull request:
https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg07652.html

It pre-dated your mandate of "all new QMP commands must have a test":
https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg00296.html

but the idea is still relevant, so a followup patch for 2.9 that adds
testsuite coverage of the new commands is appropriate during soft freeze.

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

signature.asc
Description: OpenPGP digital signature