| Message ID | 1487834070-31374-3-git-send-email-zhangchen.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/23/2017 01:14 AM, 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> > Reviewed-by: Eric Blake <eblake@redhat.com> You made a substantial change to this patch since v7 in response to my comments; when you do that, it's best to remove the R-b to make sure I re-review the changes and am still happy with them. > > --- > migration/colo.c | 23 +++++++++++++++++++++++ > qapi-schema.json | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 71 insertions(+) > > ## > +# @ReplicationStatus: > +# > +# The result format for 'query-xen-replication-status'. > +# > +# @status: true to error, false to normal. This is now a poor name for the parameter. Please rename it; probably to @error (if you want to keep true meaning a problem has been detected), or to @okay (if you want to invert the sense, and @desc is only present when @okay is false). > +# > +# @desc: #optional the human readable error description string, when > +# @status is 'true'. > +# > +# Since: 2.9 > +## > +{ 'struct': 'ReplicationStatus', > + 'data': { 'status': 'bool', '*desc': 'str' } } > + > +## > +# @query-xen-replication-status: > +# > +# Query replication status while the vm is running. > +# > +# Returns: A @ReplicationResult objects showing the status. s/objects/object/ > +# > +# Example: > +# > +# -> { "execute": "query-xen-replication-status" } > +# <- { "return": { "status": "normal" } } This example is now wrong. You'll need a v9.
On 02/23/2017 11:34 PM, Eric Blake wrote: > On 02/23/2017 01:14 AM, 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> >> Reviewed-by: Eric Blake <eblake@redhat.com> > You made a substantial change to this patch since v7 in response to my > comments; when you do that, it's best to remove the R-b to make sure I > re-review the changes and am still happy with them. OK, I will remove the R-b in next version. > >> --- >> migration/colo.c | 23 +++++++++++++++++++++++ >> qapi-schema.json | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 71 insertions(+) >> >> ## >> +# @ReplicationStatus: >> +# >> +# The result format for 'query-xen-replication-status'. >> +# >> +# @status: true to error, false to normal. > This is now a poor name for the parameter. Please rename it; probably > to @error (if you want to keep true meaning a problem has been > detected), or to @okay (if you want to invert the sense, and @desc is > only present when @okay is false). I will rename it to "@error" in next version. > >> +# >> +# @desc: #optional the human readable error description string, when >> +# @status is 'true'. >> +# >> +# Since: 2.9 >> +## >> +{ 'struct': 'ReplicationStatus', >> + 'data': { 'status': 'bool', '*desc': 'str' } } >> + >> +## >> +# @query-xen-replication-status: >> +# >> +# Query replication status while the vm is running. >> +# >> +# Returns: A @ReplicationResult objects showing the status. > s/objects/object/ I got it. > >> +# >> +# Example: >> +# >> +# -> { "execute": "query-xen-replication-status" } >> +# <- { "return": { "status": "normal" } } > This example is now wrong. > > You'll need a v9. Yes, I will send the V9 later. Thanks Zhang Chen >
Eric Blake <eblake@redhat.com> writes: > On 02/23/2017 01:14 AM, 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> >> Reviewed-by: Eric Blake <eblake@redhat.com> > > You made a substantial change to this patch since v7 in response to my > comments; when you do that, it's best to remove the R-b to make sure I > re-review the changes and am still happy with them. > >> >> --- >> migration/colo.c | 23 +++++++++++++++++++++++ >> qapi-schema.json | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 71 insertions(+) >> > >> ## >> +# @ReplicationStatus: >> +# >> +# The result format for 'query-xen-replication-status'. >> +# >> +# @status: true to error, false to normal. > > This is now a poor name for the parameter. Please rename it; probably > to @error (if you want to keep true meaning a problem has been > detected), or to @okay (if you want to invert the sense, and @desc is > only present when @okay is false). We could replace both members by '*error': 'str', present exactly when status is "bad". >> +# >> +# @desc: #optional the human readable error description string, when >> +# @status is 'true'. >> +# >> +# Since: 2.9 >> +## >> +{ 'struct': 'ReplicationStatus', >> + 'data': { 'status': 'bool', '*desc': 'str' } } >> + >> +## >> +# @query-xen-replication-status: >> +# >> +# Query replication status while the vm is running. >> +# >> +# Returns: A @ReplicationResult objects showing the status. > > s/objects/object/ > >> +# >> +# Example: >> +# >> +# -> { "execute": "query-xen-replication-status" } >> +# <- { "return": { "status": "normal" } } > > This example is now wrong. > > You'll need a v9.
On 03/03/2017 07:45 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 02/23/2017 01:14 AM, 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> >>> Reviewed-by: Eric Blake <eblake@redhat.com> >> >> You made a substantial change to this patch since v7 in response to my >> comments; when you do that, it's best to remove the R-b to make sure I >> re-review the changes and am still happy with them. >> >>> +# @ReplicationStatus: >>> +# >>> +# The result format for 'query-xen-replication-status'. >>> +# >>> +# @status: true to error, false to normal. >> >> This is now a poor name for the parameter. Please rename it; probably >> to @error (if you want to keep true meaning a problem has been >> detected), or to @okay (if you want to invert the sense, and @desc is >> only present when @okay is false). > > We could replace both members by '*error': 'str', present exactly when > status is "bad". > >>> +# >>> +# @desc: #optional the human readable error description string, when >>> +# @status is 'true'. >>> +# >>> +# Since: 2.9 >>> +## >>> +{ 'struct': 'ReplicationStatus', >>> + 'data': { 'status': 'bool', '*desc': 'str' } } Except that ReplicationStatus would then be the empty struct on success. I guess that's okay, but seems a bit odd. >>> +# Example: >>> +# >>> +# -> { "execute": "query-xen-replication-status" } >>> +# <- { "return": { "status": "normal" } } >> >> This example is now wrong. >> >> You'll need a v9. which has already been submitted and turned into a pull request: https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg07652.html It pre-dated your mandate of "all new QMP commands must have a test": https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg00296.html but the idea is still relevant, so a followup patch for 2.9 that adds testsuite coverage of the new commands is appropriate during soft freeze.
diff --git a/migration/colo.c b/migration/colo.c index 6fc2ade..8355933 100644 --- a/migration/colo.c +++ b/migration/colo.c @@ -127,6 +127,29 @@ void qmp_xen_set_replication(bool enable, bool primary, } } +ReplicationStatus *qmp_query_xen_replication_status(Error **errp) +{ + Error *err = NULL; + ReplicationStatus *s = g_new0(ReplicationStatus, 1); + + replication_get_error_all(&err); + if (err) { + s->status = true; + s->has_desc = true; + s->desc = g_strdup(error_get_pretty(err)); + } else { + s->status = false; + } + + error_free(err); + return s; +} + +void qmp_xen_colo_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..a89ed77 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -5931,6 +5931,54 @@ 'data': { 'enable': 'bool', 'primary': 'bool', '*failover' : 'bool' } } ## +# @ReplicationStatus: +# +# The result format for 'query-xen-replication-status'. +# +# @status: true to error, false to normal. +# +# @desc: #optional the human readable error description string, when +# @status is 'true'. +# +# Since: 2.9 +## +{ 'struct': 'ReplicationStatus', + 'data': { 'status': 'bool', '*desc': 'str' } } + +## +# @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': 'ReplicationStatus' } + +## +# @xen-colo-do-checkpoint: +# +# Xen uses this command to notify replication to trigger a checkpoint. +# +# Returns: nothing. +# +# Example: +# +# -> { "execute": "xen-colo-do-checkpoint" } +# <- { "return": {} } +# +# Since: 2.9 +## +{ 'command': 'xen-colo-do-checkpoint' } + +## # @GICCapability: # # The struct describes capability for a specific GIC (Generic