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

[Markus Armbruster <armbru@redhat.com>]

Zhang Chen <zhangchen.fnst@cn.fujitsu.com> writes:

> On 02/21/2017 07:15 PM, Markus Armbruster wrote:
>> Zhang Chen <zhangchen.fnst@cn.fujitsu.com> 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
>
>
>
>>
>>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>>> Signed-off-by: Wen Congyang <wencongyang@gmail.com>
>>> ---
>>>   migration/colo.c | 17 ++++++++++++++++
>>>   qapi-schema.json | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 77 insertions(+)
>>>
>>> diff --git a/migration/colo.c b/migration/colo.c
>>> index 6fc2ade..2f98a33 100644
>>> --- a/migration/colo.c
>>> +++ b/migration/colo.c
>>> @@ -127,6 +127,23 @@ void qmp_xen_set_replication(bool enable, bool primary,
>>>       }
>>>   }
>>>   +ReplicationResult *qmp_query_xen_replication_status(Error
>>> **errp)
>>> +{
>>> +    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.  QMP itself
does that.  Quoting qmp-spec.txt:

    2.4.2 error
    -----------

    The format of an error response is:

    { "error": { "class": json-string, "desc": json-string }, "id": json-value }

     Where,

    - The "class" member contains the error class name (eg. "GenericError")
--> - The "desc" member is a human-readable error message. Clients should
-->   not attempt to parse this message.
    - The "id" member contains the transaction identification associated with
      the command execution if issued by the Client

>>> +    error_free(err);
>>> +    return result;
>>> +}
>>> +
>>> +void qmp_xen_do_checkpoint(Error **errp)
>>> +{
>>> +    replication_do_checkpoint_all(errp);
>>> +}
>>> +
>>>   static void colo_send_message(QEMUFile *f, COLOMessage msg,
>>>                                 Error **errp)
>>>   {
>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>> index 9445b93..719744a 100644
>>> --- a/qapi-schema.json
>>> +++ b/qapi-schema.json
>>> @@ -5931,6 +5931,66 @@
>>>     'data': { 'enable': 'bool', 'primary': 'bool', '*failover' : 'bool' } }
>>>     ##
>>> +# @ReplicationStatus:
>>> +#
>>> +# Describe the status of replication.
>>> +#
>>> +# @error: Replication has an error.
>>> +#
>>> +# @normal: Replication is running normally.
>>> +#
>>> +# Since: 2.9
>>> +##
>>> +{ '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?
>>
>>> +
>>> +##
>>> +# @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'} }
>>> +
>>> +##
>>> +# @query-xen-replication-status:
>>> +#
>>> +# Query replication status while the vm is running.
>>> +#
>>> +# Returns: A @ReplicationResult objects showing the status.
>>> +#
>>> +# Example:
>>> +#
>>> +# -> { "execute": "query-xen-replication-status" }
>>> +# <- { "return": { "status": "normal" } }
>>> +#
>>> +# Since: 2.9
>>> +##
>>> +{ 'command': 'query-xen-replication-status',
>>> +  'returns': 'ReplicationResult' }
>> The naming is a bit unfortunate: query-xen-replication-status returns
>> ReplicationResult, which contains ReplicationStatus.
>
> I will change the name in next version.
>
> Thanks
> Zhang Chen
>
>>
>>> +
>>> +##
>>> +# @xen-do-checkpoint:
>>> +#
>>> +# Xen uses this command to notify replication to trigger a checkpoint.
>>> +#
>>> +# Returns: nothing.
>>> +#
>>> +# Example:
>>> +#
>>> +# -> { "execute": "xen-do-checkpoint" }
>>> +# <- { "return": {} }
>>> +#
>>> +# Since: 2.9
>>> +##
>>> +{ 'command': 'xen-do-checkpoint' }
>>> +
>>> +##
>>>   # @GICCapability:
>>>   #
>>>   # The struct describes capability for a specific GIC (Generic
>>
>> .
>>