[V7,2/2] Add a new qmp command to do checkpoint, query xen replication status

We can call this qmp command to do checkpoint outside of qemu.
Xen colo will need this function.

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(+)

On 02/07/2017 11:24 PM, 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 <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(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

On 02/09/2017 05:35 AM, Eric Blake wrote:
> On 02/07/2017 11:24 PM, 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 <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(+)
>>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thank you for your review.
By the way, who can pick up this patch? You or Jason?

Thanks
Zhang Chen

On Wed, 8 Feb 2017, Eric Blake wrote:
> On 02/07/2017 11:24 PM, 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 <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(+)
> > 
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Given that the series is all acked, are you going to take care of the
pull request?

Ping...

No new for a long time.

Who can pick up this patch?


Thanks

Zhang Chen


On 02/14/2017 04:28 AM, Stefano Stabellini wrote:
> On Wed, 8 Feb 2017, Eric Blake wrote:
>> On 02/07/2017 11:24 PM, 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 <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(+)
>>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
> Given that the series is all acked, are you going to take care of the
> pull request?
>
>
> .
>

On 2017年02月16日 11:25, Zhang Chen wrote:
> Ping...
>
> No new for a long time.
>
> Who can pick up this patch?
>

I believe you'd better cc migration maintainers (cced), have you tried 
scripts/get_maintainer ?

Thanks

>
> Thanks
>
> Zhang Chen
>
>
> On 02/14/2017 04:28 AM, Stefano Stabellini wrote:
>> On Wed, 8 Feb 2017, Eric Blake wrote:
>>> On 02/07/2017 11:24 PM, 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 <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(+)
>>>>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Given that the series is all acked, are you going to take care of the
>> pull request?
>>
>>
>> .
>>
>

On 02/16/2017 01:08 PM, Jason Wang wrote:
>
>
> On 2017年02月16日 11:25, Zhang Chen wrote:
>> Ping...
>>
>> No new for a long time.
>>
>> Who can pick up this patch?
>>
>
> I believe you'd better cc migration maintainers (cced), have you tried 
> scripts/get_maintainer ?

Thanks Jason.
Add cc Markus Armbruster <armbru@redhat.com>,
        Amit Shah <amit.shah@redhat.com>,
        zhanghailiang <zhang.zhanghailiang@huawei.com>


>
> Thanks
>
>>
>> Thanks
>>
>> Zhang Chen
>>
>>
>> On 02/14/2017 04:28 AM, Stefano Stabellini wrote:
>>> On Wed, 8 Feb 2017, Eric Blake wrote:
>>>> On 02/07/2017 11:24 PM, 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 <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(+)
>>>>>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> Given that the series is all acked, are you going to take care of the
>>> pull request?
>>>
>>>
>>> .
>>>
>>
>
>
>
> .
>

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?

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

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

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.

> +
> +##
> +# @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

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

>
>> +    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.

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

On 2017/2/8 13:24, 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 <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);

Indent, line break.

> +    replication_get_error_all(&err);
> +    result->status = err ?
> +                     REPLICATION_STATUS_ERROR :
> +                     REPLICATION_STATUS_NORMAL;
> +    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' ] }
> +
> +##
> +# @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'} }
                                            ^ Space

> +
> +##
> +# @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' }
> +
> +##
> +# @xen-do-checkpoint:
> +#

Maybe use the name 'xen-checkpoint-notify' or 'xen-colo-checkpoint-notify' ?

> +# 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
>

On 02/21/2017 09:01 PM, Hailiang Zhang wrote:
> On 2017/2/8 13:24, 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 <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);
>
> Indent, line break.

OK~~ will fix in next version.

>
>> +    replication_get_error_all(&err);
>> +    result->status = err ?
>> +                     REPLICATION_STATUS_ERROR :
>> +                     REPLICATION_STATUS_NORMAL;
>> +    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' ] }
>> +
>> +##
>> +# @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'} }
>                                            ^ Space

OK.

>
>> +
>> +##
>> +# @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' }
>> +
>> +##
>> +# @xen-do-checkpoint:
>> +#
>
> Maybe use the name 'xen-checkpoint-notify' or 
> 'xen-colo-checkpoint-notify' ?

I will change the name to 'xen-colo-checkpoint-notify'.

Thank
Zhang Chen

>
>> +# 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
>>
>
>
>
> .
>

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

On 02/21/2017 08:07 AM, Markus Armbruster wrote:
> 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

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.

On 02/22/2017 12:20 AM, Eric Blake wrote:
> On 02/21/2017 08:07 AM, Markus Armbruster wrote:
>> 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
> Blake, but you're not the first to mis-type it.

I'm very sorry about this typo. =_=!.

>
>>>>> +{
>>>>> +    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]

I got your point, but I don't understand this:

result->desc = ...extract string from err

Where we can get the string from err?
Can you give me a example?



>
>>>>> +##
>>>>> +{ '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?

OK, I will change it to bool in next version.

>
>>>>> +
>>>>> +##
>>>>> +# @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.

I don't know how to get the 'desc'.
Please tell me.

Thanks
Zhang Chen

>

On 02/22/2017 01:54 AM, Zhang Chen wrote:

>> 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]
> 
> I got your point, but I don't understand this:
> 
> result->desc = ...extract string from err
> 
> Where we can get the string from err?
> Can you give me a example?

See qmp_query_migrate (migration/migration.c) for similar code that
populates an optional description member when an error is present:

    case MIGRATION_STATUS_FAILED:
        info->has_status = true;
        if (s->error) {
            info->has_error_desc = true;
            info->error_desc = g_strdup(error_get_pretty(s->error));
        }
        break;

On 02/22/2017 11:23 PM, Eric Blake wrote:
> On 02/22/2017 01:54 AM, Zhang Chen wrote:
>
>>> 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]
>> I got your point, but I don't understand this:
>>
>> result->desc = ...extract string from err
>>
>> Where we can get the string from err?
>> Can you give me a example?
> See qmp_query_migrate (migration/migration.c) for similar code that
> populates an optional description member when an error is present:
>
>      case MIGRATION_STATUS_FAILED:
>          info->has_status = true;
>          if (s->error) {
>              info->has_error_desc = true;
>              info->error_desc = g_strdup(error_get_pretty(s->error));
>          }
>          break;

I will fix it in next version.

Thanks
Zhang Chen

>