[Qemu-devel] [PATCH for-2.9 V4 0/2] Add new qmp commands to suppurt Xen COLO

[Qemu-devel] [PATCH for-2.9 V4 0/2] Add new qmp commands to suppurt Xen COLO

[Zhang Chen]

Xen COLO depend on qemu COLO replication function.
So, We need new qmp commands for Xen to use qemu replication.

Corresponding libxl patches already in xen.git.
Commit ID:

ed37ef1f91c20f0ab162ce60f8c38400b917fa64
COLO: introduce new API to prepare/start/do/get_error/stop replication

a0ddc0b359375b2418213966dfbdbfab593ecc6f
tools/libxl: introduction of libxl__qmp_restore to load qemu state


Zhang Chen (2):
  Add a new qmp command to start/stop replication
  Add a new qmp command to do checkpoint, get replication error

 docs/qmp-commands.txt | 42 ++++++++++++++++++++++++++++++++++++++++++
 migration/colo.c      | 33 +++++++++++++++++++++++++++++++++
 qapi-schema.json      | 41 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 116 insertions(+)

-- 
2.7.4

[Qemu-devel] [PATCH for-2.9 V4 1/2] Add a new qmp command to start/stop replication

[Zhang Chen]

We can call this qmp command to start/stop replication 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 | 18 ++++++++++++++++++
 migration/colo.c      | 23 +++++++++++++++++++++++
 qapi-schema.json      | 19 +++++++++++++++++++
 3 files changed, 60 insertions(+)

diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt
index abf210a..a8e9eb6 100644
--- a/docs/qmp-commands.txt
+++ b/docs/qmp-commands.txt
@@ -432,6 +432,24 @@ Example:
      "arguments": { "enable": true } }
 <- { "return": {} }
 
+xen-set-replication
+-------------------
+
+Enable or disable replication.
+
+Arguments:
+
+- "enable": Enable it or disable it.
+- "primary": True for primary or false for secondary.
+- "failover": Enable failover when stopping replication, but cannot be
+              specified if 'enable' is true (optional, default false).
+
+Example:
+
+-> { "execute": "xen-set-replicate",
+     "arguments": {"enable": true, "primary": false} }
+<- { "return": {} }
+
 migrate
 -------
 
diff --git a/migration/colo.c b/migration/colo.c
index 93c85c5..6fc2ade 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -19,6 +19,8 @@
 #include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "migration/failover.h"
+#include "replication.h"
+#include "qmp-commands.h"
 
 #define COLO_BUFFER_BASE_SIZE (4 * 1024 * 1024)
 
@@ -104,6 +106,27 @@ void colo_do_failover(MigrationState *s)
     }
 }
 
+void qmp_xen_set_replication(bool enable, bool primary,
+                             bool has_failover, bool failover,
+                             Error **errp)
+{
+    ReplicationMode mode = primary ?
+                           REPLICATION_MODE_PRIMARY :
+                           REPLICATION_MODE_SECONDARY;
+
+    if (has_failover && enable) {
+        error_setg(errp, "Parameter 'failover' is only for"
+                   " stopping replication");
+        return;
+    }
+
+    if (enable) {
+        replication_start_all(mode, errp);
+    } else {
+        replication_stop_all(failover, failover ? NULL : errp);
+    }
+}
+
 static void colo_send_message(QEMUFile *f, COLOMessage msg,
                               Error **errp)
 {
diff --git a/qapi-schema.json b/qapi-schema.json
index f3e9bfc..78802f4 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4676,6 +4676,25 @@
 { 'command': 'xen-load-devices-state', 'data': {'filename': 'str'} }
 
 ##
+# @xen-set-replication
+#
+# Enable or disable replication.
+#
+# @enable: true to enable, false to disable.
+#
+# @primary: true for primary or false for secondary.
+#
+# @failover: #optional true to do failover, false to stop. but cannot be
+#            specified if 'enable' is true. default value is false.
+#
+# Returns: nothing.
+#
+# Since: 2.9
+##
+{ 'command': 'xen-set-replication',
+  'data': { 'enable': 'bool', 'primary': 'bool', '*failover' : 'bool' } }
+
+##
 # @GICCapability:
 #
 # The struct describes capability for a specific GIC (Generic
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.9 V4 1/2] Add a new qmp command to start/stop replication

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 653 bytes --]

On 12/15/2016 08:46 PM, Zhang Chen wrote:
> We can call this qmp command to start/stop replication 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 | 18 ++++++++++++++++++
>  migration/colo.c      | 23 +++++++++++++++++++++++
>  qapi-schema.json      | 19 +++++++++++++++++++
>  3 files changed, 60 insertions(+)

For the QMP side:
Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for-2.9 V4 1/2] Add a new qmp command to start/stop replication

[Stefano Stabellini]

On Fri, 16 Dec 2016, Zhang Chen wrote:
> We can call this qmp command to start/stop replication 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 | 18 ++++++++++++++++++
>  migration/colo.c      | 23 +++++++++++++++++++++++
>  qapi-schema.json      | 19 +++++++++++++++++++
>  3 files changed, 60 insertions(+)
> 
> diff --git a/docs/qmp-commands.txt b/docs/qmp-commands.txt
> index abf210a..a8e9eb6 100644
> --- a/docs/qmp-commands.txt
> +++ b/docs/qmp-commands.txt
> @@ -432,6 +432,24 @@ Example:
>       "arguments": { "enable": true } }
>  <- { "return": {} }
>  
> +xen-set-replication
> +-------------------
> +
> +Enable or disable replication.
> +
> +Arguments:
> +
> +- "enable": Enable it or disable it.
> +- "primary": True for primary or false for secondary.
> +- "failover": Enable failover when stopping replication, but cannot be
> +              specified if 'enable' is true (optional, default false).
> +
> +Example:
> +
> +-> { "execute": "xen-set-replicate",

Shouldn't this be "xen-set-replication" ?


> +     "arguments": {"enable": true, "primary": false} }
> +<- { "return": {} }
> +
>  migrate
>  -------
>  
> diff --git a/migration/colo.c b/migration/colo.c
> index 93c85c5..6fc2ade 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -19,6 +19,8 @@
>  #include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include "migration/failover.h"
> +#include "replication.h"
> +#include "qmp-commands.h"
>  
>  #define COLO_BUFFER_BASE_SIZE (4 * 1024 * 1024)
>  
> @@ -104,6 +106,27 @@ void colo_do_failover(MigrationState *s)
>      }
>  }
>  
> +void qmp_xen_set_replication(bool enable, bool primary,
> +                             bool has_failover, bool failover,
> +                             Error **errp)
> +{
> +    ReplicationMode mode = primary ?
> +                           REPLICATION_MODE_PRIMARY :
> +                           REPLICATION_MODE_SECONDARY;
> +
> +    if (has_failover && enable) {
> +        error_setg(errp, "Parameter 'failover' is only for"
> +                   " stopping replication");
> +        return;
> +    }
> +
> +    if (enable) {
> +        replication_start_all(mode, errp);
> +    } else {
> +        replication_stop_all(failover, failover ? NULL : errp);
> +    }
> +}
> +
>  static void colo_send_message(QEMUFile *f, COLOMessage msg,
>                                Error **errp)
>  {
> diff --git a/qapi-schema.json b/qapi-schema.json
> index f3e9bfc..78802f4 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4676,6 +4676,25 @@
>  { 'command': 'xen-load-devices-state', 'data': {'filename': 'str'} }
>  
>  ##
> +# @xen-set-replication
> +#
> +# Enable or disable replication.
> +#
> +# @enable: true to enable, false to disable.
> +#
> +# @primary: true for primary or false for secondary.
> +#
> +# @failover: #optional true to do failover, false to stop. but cannot be
> +#            specified if 'enable' is true. default value is false.
> +#
> +# Returns: nothing.
> +#
> +# Since: 2.9
> +##
> +{ 'command': 'xen-set-replication',
> +  'data': { 'enable': 'bool', 'primary': 'bool', '*failover' : 'bool' } }
> +
> +##
>  # @GICCapability:
>  #
>  # The struct describes capability for a specific GIC (Generic
> -- 
> 2.7.4
> 
> 
> 
> 

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

[Zhang Chen]

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": {} }
+
 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

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1889 bytes --]

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.


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Zhang Chen]

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

-- 
Thanks
Zhang Chen

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2377 bytes --]

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


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Zhang Chen]

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


-- 
Thanks
Zhang Chen

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

[addr_cc]

[-- Attachment #1: Type: text/plain, Size: 2726 bytes --]

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 */
}

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2854 bytes --]

[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 */
}

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




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Zhang Chen]

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

>




-- 
Thanks
Zhang Chen

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

[Stefano Stabellini]

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

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

[Zhang Chen]

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

-- 
Thanks
Zhang Chen

Re: [Qemu-devel] [PATCH for-2.9 V4 0/2] Add new qmp commands to suppurt Xen COLO

[Stefano Stabellini]

I am OK with this, if the relevant maintainers (Migration, QMP) ack the
patches.

On Fri, 16 Dec 2016, Zhang Chen wrote:
> Xen COLO depend on qemu COLO replication function.
> So, We need new qmp commands for Xen to use qemu replication.
> 
> Corresponding libxl patches already in xen.git.
> Commit ID:
> 
> ed37ef1f91c20f0ab162ce60f8c38400b917fa64
> COLO: introduce new API to prepare/start/do/get_error/stop replication
> 
> a0ddc0b359375b2418213966dfbdbfab593ecc6f
> tools/libxl: introduction of libxl__qmp_restore to load qemu state
> 
> 
> Zhang Chen (2):
>   Add a new qmp command to start/stop replication
>   Add a new qmp command to do checkpoint, get replication error
> 
>  docs/qmp-commands.txt | 42 ++++++++++++++++++++++++++++++++++++++++++
>  migration/colo.c      | 33 +++++++++++++++++++++++++++++++++
>  qapi-schema.json      | 41 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 116 insertions(+)
> 
> -- 
> 2.7.4
> 
> 
> 
> 

Re: [Qemu-devel] [PATCH for-2.9 V4 0/2] Add new qmp commands to suppurt Xen COLO

[Zhang Chen]

On 12/20/2016 08:56 AM, Stefano Stabellini wrote:
> I am OK with this, if the relevant maintainers (Migration, QMP) ack the
> patches.

Can you add reviewed-by for this patch set?

Thanks
Zhang Chen

> On Fri, 16 Dec 2016, Zhang Chen wrote:
>> Xen COLO depend on qemu COLO replication function.
>> So, We need new qmp commands for Xen to use qemu replication.
>>
>> Corresponding libxl patches already in xen.git.
>> Commit ID:
>>
>> ed37ef1f91c20f0ab162ce60f8c38400b917fa64
>> COLO: introduce new API to prepare/start/do/get_error/stop replication
>>
>> a0ddc0b359375b2418213966dfbdbfab593ecc6f
>> tools/libxl: introduction of libxl__qmp_restore to load qemu state
>>
>>
>> Zhang Chen (2):
>>    Add a new qmp command to start/stop replication
>>    Add a new qmp command to do checkpoint, get replication error
>>
>>   docs/qmp-commands.txt | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   migration/colo.c      | 33 +++++++++++++++++++++++++++++++++
>>   qapi-schema.json      | 41 +++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 116 insertions(+)
>>
>> -- 
>> 2.7.4
>>
>>
>>
>>
>
> .
>

-- 
Thanks
Zhang Chen