From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45351) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWEib-000744-B3 for qemu-devel@nongnu.org; Sun, 30 Aug 2015 22:21:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZWEiY-0002zy-3Z for qemu-devel@nongnu.org; Sun, 30 Aug 2015 22:21:09 -0400 Received: from dfwrgout.huawei.com ([206.16.17.72]:5338) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWEiX-0002up-S8 for qemu-devel@nongnu.org; Sun, 30 Aug 2015 22:21:06 -0400 References: <1438159544-6224-1-git-send-email-zhang.zhanghailiang@huawei.com> <1438159544-6224-3-git-send-email-zhang.zhanghailiang@huawei.com> <55E0D894.4050408@redhat.com> From: zhanghailiang Message-ID: <55E3B97F.4000404@huawei.com> Date: Mon, 31 Aug 2015 10:18:39 +0800 MIME-Version: 1.0 In-Reply-To: <55E0D894.4050408@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH COLO-Frame v8 02/34] migration: Introduce capability 'colo' to migration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: lizhijian@cn.fujitsu.com, quintela@redhat.com, Markus Armbruster , yunhong.jiang@intel.com, eddie.dong@intel.com, peter.huangpeng@huawei.com, dgilbert@redhat.com, arei.gonglei@huawei.com, amit.shah@redhat.com, Yang Hongyang 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 >> Cc: Amit Shah >> Cc: Eric Blake >> Cc: Markus Armbruster >> Signed-off-by: zhanghailiang >> Signed-off-by: Yang Hongyang >> Signed-off-by: Gonglei >> --- > > 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.