[for-2.9,V4,2/2] Add a new qmp command to do checkpoint, get replication error

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

Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Signed-off-by: Wen Congyang <wencongyang@gmail.com>
---
 docs/qmp-commands.txt | 24 ++++++++++++++++++++++++
 migration/colo.c      | 10 ++++++++++
 qapi-schema.json      | 22 ++++++++++++++++++++++
 3 files changed, 56 insertions(+)

On 12/15/2016 08:46 PM, Zhang Chen wrote:
> We can call this qmp command to do checkpoint outside of qemu.
> Like Xen colo need this function.
> 
> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
> Signed-off-by: Wen Congyang <wencongyang@gmail.com>
> ---
>  docs/qmp-commands.txt | 24 ++++++++++++++++++++++++
>  migration/colo.c      | 10 ++++++++++
>  qapi-schema.json      | 22 ++++++++++++++++++++++
>  3 files changed, 56 insertions(+)
> 

>  
> +void qmp_xen_get_replication_error(Error **errp)
> +    {
> +        replication_get_error_all(errp);
> +    }

Is this trying to cause a replication error, or is it trying to collect
status on whether an error has occurred? The name 'get' usually implies
a query that does not change state, but a query usually needs some way
to return what was queried back to the user, so a void function with no
parameters other than error is odd (either the command succeeds because
there was no error, or the command fails with an error message because
the query had something to get?).


> +++ b/qapi-schema.json
> @@ -4695,6 +4695,28 @@
>    'data': { 'enable': 'bool', 'primary': 'bool', '*failover' : 'bool' } }
>  
>  ##
> +# @xen-get-replication-error
> +#
> +# Get replication error that occurs when the vm is running.
> +#
> +# Returns: nothing.
> +#
> +# Since: 2.9
> +##
> +{ 'command': 'xen-get-replication-error' }

So I don't think this is the right name for its signature, but I don't
know what you are trying to accomplish with this command to suggest the
right name.

> +
> +##
> +# @xen-do-checkpoint
> +#
> +# Do checkpoint.
> +#
> +# Returns: nothing.
> +#
> +# Since: 2.9
> +##
> +{ 'command': 'xen-do-checkpoint' }

This one is probably okay.

On Fri, 16 Dec 2016, Zhang Chen wrote:
> We can call this qmp command to do checkpoint outside of qemu.
> Like Xen colo need this function.
> 
> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
> Signed-off-by: Wen Congyang <wencongyang@gmail.com>
> ---
>  docs/qmp-commands.txt | 24 ++++++++++++++++++++++++
>  migration/colo.c      | 10 ++++++++++
>  qapi-schema.json      | 22 ++++++++++++++++++++++
>  3 files changed, 56 insertions(+)
> 
> diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt
> index a8e9eb6..093b7eb 100644
> --- a/docs/qmp-commands.txt
> +++ b/docs/qmp-commands.txt
> @@ -450,6 +450,30 @@ Example:
>       "arguments": {"enable": true, "primary": false} }
>  <- { "return": {} }
>  
> +xen-get-replication-error
> +-------------------------
> +
> +Get replication error that occurs when vm is running.
> +
> +Arguments: None.
> +
> +Example:
> +
> +-> { "execute": "xen-get-replication-error" }
> +<- { "return": {} }
> +
> +xen-do-checkpoint
> +-----------------
> +
> +Do checkpoint.
> +
> +Arguments: None.
> +
> +Example:
> +
> +-> { "execute": "xen-do-checkpoint" }
> +<- { "return": {} }

I don't know much about QMP, but shouldn't these two functions return a
success or failure at least? Especially xen-get-replication-error? How
does xen-get-replication-error actually return the error?

Please also write this info on the description of the commands
(docs/qmp-commands.txt, qapi-schema.json).


>  migrate
>  -------
>  
> diff --git a/migration/colo.c b/migration/colo.c
> index 6fc2ade..1e962f6 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -127,6 +127,16 @@ void qmp_xen_set_replication(bool enable, bool primary,
>      }
>  }
>  
> +void qmp_xen_get_replication_error(Error **errp)
> +    {
> +        replication_get_error_all(errp);
> +    }
> +
> +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 78802f4..79fe4dd 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4695,6 +4695,28 @@
>    'data': { 'enable': 'bool', 'primary': 'bool', '*failover' : 'bool' } }
>  
>  ##
> +# @xen-get-replication-error
> +#
> +# Get replication error that occurs when the vm is running.
> +#
> +# Returns: nothing.
> +#
> +# Since: 2.9
> +##
> +{ 'command': 'xen-get-replication-error' }
> +
> +##
> +# @xen-do-checkpoint
> +#
> +# Do checkpoint.
> +#
> +# Returns: nothing.
> +#
> +# Since: 2.9
> +##
> +{ 'command': 'xen-do-checkpoint' }
> +
> +##
>  # @GICCapability:
>  #
>  # The struct describes capability for a specific GIC (Generic
> -- 
> 2.7.4
> 
> 
> 
>

On 12/20/2016 10:42 PM, Eric Blake wrote:
> On 12/15/2016 08:46 PM, Zhang Chen wrote:
>> We can call this qmp command to do checkpoint outside of qemu.
>> Like Xen colo need this function.
>>
>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>> Signed-off-by: Wen Congyang <wencongyang@gmail.com>
>> ---
>>   docs/qmp-commands.txt | 24 ++++++++++++++++++++++++
>>   migration/colo.c      | 10 ++++++++++
>>   qapi-schema.json      | 22 ++++++++++++++++++++++
>>   3 files changed, 56 insertions(+)
>>
>>   
>> +void qmp_xen_get_replication_error(Error **errp)
>> +    {
>> +        replication_get_error_all(errp);
>> +    }
> Is this trying to cause a replication error, or is it trying to collect
> status on whether an error has occurred? The name 'get' usually implies
> a query that does not change state, but a query usually needs some way
> to return what was queried back to the user, so a void function with no
> parameters other than error is odd (either the command succeeds because
> there was no error, or the command fails with an error message because
> the query had something to get?).

Make sense, this command trying to collect status on whether
an error has occurred, and the "replication_get_error_all(errp)"
is always succeeds. So, Can you suggest to me the right name?


>
>
>> +++ b/qapi-schema.json
>> @@ -4695,6 +4695,28 @@
>>     'data': { 'enable': 'bool', 'primary': 'bool', '*failover' : 'bool' } }
>>   
>>   ##
>> +# @xen-get-replication-error
>> +#
>> +# Get replication error that occurs when the vm is running.
>> +#
>> +# Returns: nothing.
>> +#
>> +# Since: 2.9
>> +##
>> +{ 'command': 'xen-get-replication-error' }
> So I don't think this is the right name for its signature, but I don't
> know what you are trying to accomplish with this command to suggest the
> right name.

This interface is called to check if error happened in replication.

Thanks
Zhang Chen


>> +
>> +##
>> +# @xen-do-checkpoint
>> +#
>> +# Do checkpoint.
>> +#
>> +# Returns: nothing.
>> +#
>> +# Since: 2.9
>> +##
>> +{ 'command': 'xen-do-checkpoint' }
> This one is probably okay.
>
>

On 12/21/2016 08:18 AM, Stefano Stabellini wrote:
> On Fri, 16 Dec 2016, Zhang Chen wrote:
>> We can call this qmp command to do checkpoint outside of qemu.
>> Like Xen colo need this function.
>>
>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>> Signed-off-by: Wen Congyang <wencongyang@gmail.com>
>> ---
>>   docs/qmp-commands.txt | 24 ++++++++++++++++++++++++
>>   migration/colo.c      | 10 ++++++++++
>>   qapi-schema.json      | 22 ++++++++++++++++++++++
>>   3 files changed, 56 insertions(+)
>>
>> diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt
>> index a8e9eb6..093b7eb 100644
>> --- a/docs/qmp-commands.txt
>> +++ b/docs/qmp-commands.txt
>> @@ -450,6 +450,30 @@ Example:
>>        "arguments": {"enable": true, "primary": false} }
>>   <- { "return": {} }
>>   
>> +xen-get-replication-error
>> +-------------------------
>> +
>> +Get replication error that occurs when vm is running.
>> +
>> +Arguments: None.
>> +
>> +Example:
>> +
>> +-> { "execute": "xen-get-replication-error" }
>> +<- { "return": {} }
>> +
>> +xen-do-checkpoint
>> +-----------------
>> +
>> +Do checkpoint.
>> +
>> +Arguments: None.
>> +
>> +Example:
>> +
>> +-> { "execute": "xen-do-checkpoint" }
>> +<- { "return": {} }
> I don't know much about QMP, but shouldn't these two functions return a
> success or failure at least? Especially xen-get-replication-error? How
> does xen-get-replication-error actually return the error?
>
> Please also write this info on the description of the commands
> (docs/qmp-commands.txt, qapi-schema.json).
>

OK, I will add more description of the commands in next version.

Thanks
Zhang Chen

>>   migrate
>>   -------
>>   
>> diff --git a/migration/colo.c b/migration/colo.c
>> index 6fc2ade..1e962f6 100644
>> --- a/migration/colo.c
>> +++ b/migration/colo.c
>> @@ -127,6 +127,16 @@ void qmp_xen_set_replication(bool enable, bool primary,
>>       }
>>   }
>>   
>> +void qmp_xen_get_replication_error(Error **errp)
>> +    {
>> +        replication_get_error_all(errp);
>> +    }
>> +
>> +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 78802f4..79fe4dd 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -4695,6 +4695,28 @@
>>     'data': { 'enable': 'bool', 'primary': 'bool', '*failover' : 'bool' } }
>>   
>>   ##
>> +# @xen-get-replication-error
>> +#
>> +# Get replication error that occurs when the vm is running.
>> +#
>> +# Returns: nothing.
>> +#
>> +# Since: 2.9
>> +##
>> +{ 'command': 'xen-get-replication-error' }
>> +
>> +##
>> +# @xen-do-checkpoint
>> +#
>> +# Do checkpoint.
>> +#
>> +# Returns: nothing.
>> +#
>> +# Since: 2.9
>> +##
>> +{ 'command': 'xen-do-checkpoint' }
>> +
>> +##
>>   # @GICCapability:
>>   #
>>   # The struct describes capability for a specific GIC (Generic
>> -- 
>> 2.7.4
>>
>>
>>
>>
>
> .
>

On 12/21/2016 12:56 AM, Zhang Chen wrote:

>>>   +void qmp_xen_get_replication_error(Error **errp)
>>> +    {
>>> +        replication_get_error_all(errp);
>>> +    }
>> Is this trying to cause a replication error, or is it trying to collect
>> status on whether an error has occurred? The name 'get' usually implies
>> a query that does not change state, but a query usually needs some way
>> to return what was queried back to the user, so a void function with no
>> parameters other than error is odd (either the command succeeds because
>> there was no error, or the command fails with an error message because
>> the query had something to get?).
> 
> Make sense, this command trying to collect status on whether
> an error has occurred, and the "replication_get_error_all(errp)"
> is always succeeds. So, Can you suggest to me the right name?

If replication_get_error_all() always succeeds, then what failure is
possible to be checking for?

Maybe the problem is deeper, in that replication_get_error_all() has an
unusual signature, and needs to be fixed first.  I don't know, and
haven't looked; I'm only coming at this from the user interface
perspective.  But it makes no sense to have a command that queries
whether an error occurred, but where an error having occurred is fatal
(you want the command to successfully report that an error has occurred,
not error out with a second error because a first error was present).


>>> +# Since: 2.9
>>> +##
>>> +{ 'command': 'xen-get-replication-error' }
>> So I don't think this is the right name for its signature, but I don't
>> know what you are trying to accomplish with this command to suggest the
>> right name.
> 
> This interface is called to check if error happened in replication.

Then you probably want a query style interface:

{ 'command': 'query-xen-replication-status',
  'returns': 'SomeStruct' }

where SomeStruct contains details such as status (perhaps an enum that
reports 'normal' or 'error'), and where you are free to add additional
pieces of information that may prove useful later (rather than having to
invent yet more commands that give only a boolean result of success or
failure based on whether the state is normal or in error).

On 12/21/2016 10:25 PM, Eric Blake wrote:
> On 12/21/2016 12:56 AM, Zhang Chen wrote:
>
>>>>    +void qmp_xen_get_replication_error(Error **errp)
>>>> +    {
>>>> +        replication_get_error_all(errp);
>>>> +    }
>>> Is this trying to cause a replication error, or is it trying to collect
>>> status on whether an error has occurred? The name 'get' usually implies
>>> a query that does not change state, but a query usually needs some way
>>> to return what was queried back to the user, so a void function with no
>>> parameters other than error is odd (either the command succeeds because
>>> there was no error, or the command fails with an error message because
>>> the query had something to get?).
>> Make sense, this command trying to collect status on whether
>> an error has occurred, and the "replication_get_error_all(errp)"
>> is always succeeds. So, Can you suggest to me the right name?
> If replication_get_error_all() always succeeds, then what failure is
> possible to be checking for?

We can read the errp to check the last error.

>
> Maybe the problem is deeper, in that replication_get_error_all() has an
> unusual signature, and needs to be fixed first.  I don't know, and
> haven't looked; I'm only coming at this from the user interface
> perspective.  But it makes no sense to have a command that queries
> whether an error occurred, but where an error having occurred is fatal
> (you want the command to successfully report that an error has occurred,
> not error out with a second error because a first error was present).

Do you means we should fix "void replication_get_error_all()"
to "int replication_get_error_all()" first for get the return value?

>
>>>> +# Since: 2.9
>>>> +##
>>>> +{ 'command': 'xen-get-replication-error' }
>>> So I don't think this is the right name for its signature, but I don't
>>> know what you are trying to accomplish with this command to suggest the
>>> right name.
>> This interface is called to check if error happened in replication.
> Then you probably want a query style interface:
>
> { 'command': 'query-xen-replication-status',
>    'returns': 'SomeStruct' }
>
> where SomeStruct contains details such as status (perhaps an enum that
> reports 'normal' or 'error'), and where you are free to add additional
> pieces of information that may prove useful later (rather than having to
> invent yet more commands that give only a boolean result of success or
> failure based on whether the state is normal or in error).
>

Thanks for your suggestion.
Zhang Chen

On 12/22/2016 12:08 AM, Zhang Chen wrote:
>>> Make sense, this command trying to collect status on whether
>>> an error has occurred, and the "replication_get_error_all(errp)"
>>> is always succeeds. So, Can you suggest to me the right name?
>> If replication_get_error_all() always succeeds, then what failure is
>> possible to be checking for?
> 
> We can read the errp to check the last error.

But turning around and reporting an error to the caller is not nice.
The caller can't distinguish between "I called the command correctly,
and it is telling me the system has encountered a replication error" and
"I called the command incorrectly, and it is telling me my usage is
wrong even though the system has never encountered a replication error".
 Passing information through errp is NOT the right way to successfully
report status.

> 
>>
>> Maybe the problem is deeper, in that replication_get_error_all() has an
>> unusual signature, and needs to be fixed first.  I don't know, and
>> haven't looked; I'm only coming at this from the user interface
>> perspective.  But it makes no sense to have a command that queries
>> whether an error occurred, but where an error having occurred is fatal
>> (you want the command to successfully report that an error has occurred,
>> not error out with a second error because a first error was present).
> 
> Do you means we should fix "void replication_get_error_all()"
> to "int replication_get_error_all()" first for get the return value?

Quite possibly yes. But maybe you don't have to do that, and can come up
with a scheme where only the QMP command wrapper has to be careful.
Perhaps something like this would work:

>> Then you probably want a query style interface:
>>
>> { 'command': 'query-xen-replication-status',
>>    'returns': 'SomeStruct' }
>>
>> where SomeStruct contains details such as status (perhaps an enum that
>> reports 'normal' or 'error'), and where you are free to add additional
>> pieces of information that may prove useful later (rather than having to
>> invent yet more commands that give only a boolean result of success or
>> failure based on whether the state is normal or in error).

SomeStruct *qmp_query_xen_replication_status(Error **errp)
{
    Error *err = NULL;
    SomeStruct *result = g_new0(SomeStruct, 1);
    replication_get_error_all(&err);
    result.state = err ? SOME_ENUM_ERRORED : SOME_ENUM_NORMAL;
    error_free(err);
    /* ... and now you can add additional status items to the API,
       as needed. errp remains unset, because the command succeeds */
}

[resend, I don't know how I botched the From: line in my first attempt,
or if that botch changed who received the mail]

On 12/22/2016 12:08 AM, Zhang Chen wrote:
>>> Make sense, this command trying to collect status on whether
>>> an error has occurred, and the "replication_get_error_all(errp)"
>>> is always succeeds. So, Can you suggest to me the right name?
>> If replication_get_error_all() always succeeds, then what failure is
>> possible to be checking for?
> 
> We can read the errp to check the last error.

But turning around and reporting an error to the caller is not nice.
The caller can't distinguish between "I called the command correctly,
and it is telling me the system has encountered a replication error" and
"I called the command incorrectly, and it is telling me my usage is
wrong even though the system has never encountered a replication error".
 Passing information through errp is NOT the right way to successfully
report status.

> 
>>
>> Maybe the problem is deeper, in that replication_get_error_all() has an
>> unusual signature, and needs to be fixed first.  I don't know, and
>> haven't looked; I'm only coming at this from the user interface
>> perspective.  But it makes no sense to have a command that queries
>> whether an error occurred, but where an error having occurred is fatal
>> (you want the command to successfully report that an error has occurred,
>> not error out with a second error because a first error was present).
> 
> Do you means we should fix "void replication_get_error_all()"
> to "int replication_get_error_all()" first for get the return value?

Quite possibly yes. But maybe you don't have to do that, and can come up
with a scheme where only the QMP command wrapper has to be careful.
Perhaps something like this would work:

>> Then you probably want a query style interface:
>>
>> { 'command': 'query-xen-replication-status',
>>    'returns': 'SomeStruct' }
>>
>> where SomeStruct contains details such as status (perhaps an enum that
>> reports 'normal' or 'error'), and where you are free to add additional
>> pieces of information that may prove useful later (rather than having to
>> invent yet more commands that give only a boolean result of success or
>> failure based on whether the state is normal or in error).

SomeStruct *qmp_query_xen_replication_status(Error **errp)
{
    Error *err = NULL;
    SomeStruct *result = g_new0(SomeStruct, 1);
    replication_get_error_all(&err);
    result.state = err ? SOME_ENUM_ERRORED : SOME_ENUM_NORMAL;
    error_free(err);
    /* ... and now you can add additional status items to the API,
       as needed. errp remains unset, because the command succeeds */
}

On 12/22/2016 08:52 PM, Eric Blake wrote:
> [resend, I don't know how I botched the From: line in my first attempt,
> or if that botch changed who received the mail]
>
> On 12/22/2016 12:08 AM, Zhang Chen wrote:
>>>> Make sense, this command trying to collect status on whether
>>>> an error has occurred, and the "replication_get_error_all(errp)"
>>>> is always succeeds. So, Can you suggest to me the right name?
>>> If replication_get_error_all() always succeeds, then what failure is
>>> possible to be checking for?
>> We can read the errp to check the last error.
> But turning around and reporting an error to the caller is not nice.
> The caller can't distinguish between "I called the command correctly,
> and it is telling me the system has encountered a replication error" and
> "I called the command incorrectly, and it is telling me my usage is
> wrong even though the system has never encountered a replication error".
>   Passing information through errp is NOT the right way to successfully
> report status.
>
>>> Maybe the problem is deeper, in that replication_get_error_all() has an
>>> unusual signature, and needs to be fixed first.  I don't know, and
>>> haven't looked; I'm only coming at this from the user interface
>>> perspective.  But it makes no sense to have a command that queries
>>> whether an error occurred, but where an error having occurred is fatal
>>> (you want the command to successfully report that an error has occurred,
>>> not error out with a second error because a first error was present).
>> Do you means we should fix "void replication_get_error_all()"
>> to "int replication_get_error_all()" first for get the return value?
> Quite possibly yes. But maybe you don't have to do that, and can come up
> with a scheme where only the QMP command wrapper has to be careful.
> Perhaps something like this would work:
>
>>> Then you probably want a query style interface:
>>>
>>> { 'command': 'query-xen-replication-status',
>>>     'returns': 'SomeStruct' }
>>>
>>> where SomeStruct contains details such as status (perhaps an enum that
>>> reports 'normal' or 'error'), and where you are free to add additional
>>> pieces of information that may prove useful later (rather than having to
>>> invent yet more commands that give only a boolean result of success or
>>> failure based on whether the state is normal or in error).
> SomeStruct *qmp_query_xen_replication_status(Error **errp)
> {
>      Error *err = NULL;
>      SomeStruct *result = g_new0(SomeStruct, 1);
>      replication_get_error_all(&err);
>      result.state = err ? SOME_ENUM_ERRORED : SOME_ENUM_NORMAL;
>      error_free(err);
>      /* ... and now you can add additional status items to the API,
>         as needed. errp remains unset, because the command succeeds */
> }

Good idea!
I will fix this in next version.

Thanks
Zhang Chen

>