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

[Eric Blake]

On 02/21/2017 08:07 AM, Markus Armbruster wrote:
> Zhang Chen <address@hidden> writes:
> 
>> On 02/21/2017 07:15 PM, Markus Armbruster wrote:
>>> Zhang Chen <address@hidden> writes:
>>>
>>>> We can call this qmp command to do checkpoint outside of qemu.
>>>> Xen colo will need this function.
>>> I know nothing about "Xen colo", but I'll try anyway.
>>>
>>> I *guess* "Xen colo" is a long-running activity, and the new commands
>>> interact with it.  Correct?
>>
>> Yes, you are right.
> 
> We need to build a generic framework for interacting with long-running
> activities.  But asking you to wait for it wouldn't be fair.
> 
>> By the way this patch has been reviewed by Eric black.
>>
>> https://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg03435.html

Blake, but you're not the first to mis-type it.

>>>> +{
>>>> +    Error *err = NULL;
>>>> +    ReplicationResult *result = g_new0(ReplicationResult, 1);
>>>> +    replication_get_error_all(&err);
>>>> +    result->status = err ?
>>>> +                     REPLICATION_STATUS_ERROR :
>>>> +                     REPLICATION_STATUS_NORMAL;
>>> Reports only that there is an error, throws away the actual error
>>> message.  Naive question: could the error message be good to know for
>>> the QMP user?
>>
>> Yes, Xen colo will handle it.
> 
> Is that "yes, the QMP user could use the error message, but we're not
> giving it to him regardless", or "no, the QMP user does not need to
> know, and that's because we don't give it to him"?
> 
> Even if we want QMP clients to treat all errors the same, transmitting
> the error message can still be useful for troubleshooting.

Ah, as in:

if (err) {
  result->status = REPLICATION_STATUS_ERROR;
  result->has_desc = true;
  result->desc = ...extract string from err
} else {
  result->status = REPLICATION_STATUS_NORMAL;
}

by modifying the JSON [1]

>>>> +##
>>>> +{ 'enum': 'ReplicationStatus',
>>>> +  'data': [ 'error', 'normal' ] }
>>> Do you expect more status values in the future?
>>>
>>> If yes, what should clients do when they see a status value they don't
>>> know?
>>
>> We will add other status for it, now, that's enough.
> 
> What should a QMP client do when it sees a ReplicationStatus value other
> than 'error' and 'normal'?
> 
> You need to provide some guidance, or else you won't be able to add
> status values without breaking clients!
> 
>>> If no, why not simply use bool?

Off-hand, what other states do you envision adding?

>>>
>>>> +
>>>> +##
>>>> +# @ReplicationResult:
>>>> +#
>>>> +# The result format for 'query-xen-replication-status'.
>>>> +#
>>>> +# @status: enum of @ReplicationStatus, which shows current
>>>> +#          replication error status
>>>> +#
>>>> +# Since: 2.9
>>>> +##
>>>> +{ 'struct': 'ReplicationResult',
>>>> +  'data': { 'status': 'ReplicationStatus'} }

[1] the modification would be:

'data': { 'status': 'ReplicationStatus', '*desc': 'str' }

with documentation that @desc is #optional, is only for human
consumption, and is only present when @status indicates an error.

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

signature.asc
Description: OpenPGP digital signature