On 07/29/2015 02:45 AM, zhanghailiang wrote: > We add helper function colo_supported() to indicate whether > colo is supported or not, with which we use to control whether or not > showing 'colo' string to users, they can use qmp command > 'query-migrate-capabilities' or hmp command 'info migrate_capabilities' > to learn if colo is supported. > > Cc: Juan Quintela <quint...@redhat.com> > Cc: Amit Shah <amit.s...@redhat.com> > Cc: Eric Blake <ebl...@redhat.com> > Cc: Markus Armbruster <arm...@redhat.com> > Signed-off-by: zhanghailiang <zhang.zhanghaili...@huawei.com> > Signed-off-by: Yang Hongyang <yan...@cn.fujitsu.com> > Signed-off-by: Gonglei <arei.gong...@huawei.com> > ---
Just focusing on the interface.
> @@ -338,6 +339,9 @@ MigrationCapabilityStatusList
> *qmp_query_migrate_capabilities(Error **errp)
>
> caps = NULL; /* silence compiler warning */
> for (i = 0; i < MIGRATION_CAPABILITY_MAX; i++) {
> + if (i == MIGRATION_CAPABILITY_COLO && !colo_supported()) {
> + continue;
> + }
Interesting - you completely omit the capability if it can't be set to
true; so the presence of the capability is therefore the witness of
whether it works. Which means it is a true runtime probe, rather than a
static introspection, and therefore more accurate :)
> if (head == NULL) {
> head = g_malloc0(sizeof(*caps));
> caps = head;
> @@ -478,6 +482,13 @@ void
> qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
> }
>
> for (cap = params; cap; cap = cap->next) {
> + if (cap->value->capability == MIGRATION_CAPABILITY_COLO &&
> + cap->value->state && !colo_supported()) {
> + error_setg(errp, "COLO is not currently supported, please"
> + " configure with --enable-colo option in order
> to"
> + " support COLO feature");
> + continue;
> + }
This says that you error out if colo:true is requested but not
supported, but silently ignore colo:false even when it is unsupported.
Isn't it better to error out that colo is unsupported, regardless of
enabled true/false being requested, since you explicitly avoided
advertising the feature?
> +++ b/qapi-schema.json
> @@ -529,11 +529,14 @@
> # @auto-converge: If enabled, QEMU will automatically throttle down the guest
> # to speed up convergence of RAM migration. (since 1.6)
> #
> +# @colo: If enabled, migration will never end, and the state of VM in
> primary side
Long line. Please wrap to fit within 80 columns.
> +# will be migrated continuously to VM in secondary side. (since 2.4)
You've missed 2.4; change this to 2.5.
Grammar suggestion:
s/of VM in primary/of the VM on the primary/
s/to VM in secondary/to the VM on the secondary/
> +++ b/qmp-commands.hx
> @@ -3434,6 +3434,7 @@ Query current migration capabilities
> - "rdma-pin-all" : RDMA Pin Page state (json-bool)
> - "auto-converge" : Auto Converge state (json-bool)
> - "zero-blocks" : Zero Blocks state (json-bool)
> + - "colo" : COLO FT state (json-bool)
Acronym soup. Might be nice to state more human-readable text about what
'colo' actually controls (stating COarse-grain LOcking fault tolerance
would help).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
