Re: [Qemu-devel] [PATCH COLO-Frame v8 02/34] migration: Introduce capability 'colo' to migration

[zhanghailiang]

On 2015/8/29 5:54, Eric Blake wrote:
> 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 <address@hidden>
> > Cc: Amit Shah <address@hidden>
> > Cc: Eric Blake <address@hidden>
> > Cc: Markus Armbruster <address@hidden>
> > Signed-off-by: zhanghailiang <address@hidden>
> > Signed-off-by: Yang Hongyang <address@hidden>
> > Signed-off-by: Gonglei <address@hidden>
> > ---
>
> Just focusing on the interface.

Thank you very much for the review.

> > @@ -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 :)

Yes, it will help users to know if they can set this capability or not. I can't
figure out a better way to do this thing ;)


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

You are right, we should tell people colo is unsupported whether the set or 
unset
this capability, will fix in next version.

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

OK, will fix it.

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

OK, i will address all the problems in next version, thanks.