From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54932) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWLaY-0002s1-NF for qemu-devel@nongnu.org; Mon, 31 Aug 2015 05:41:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZWLaU-0003BH-Dn for qemu-devel@nongnu.org; Mon, 31 Aug 2015 05:41:18 -0400 Received: from szxga02-in.huawei.com ([119.145.14.65]:36548) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWLaT-0003AW-P2 for qemu-devel@nongnu.org; Mon, 31 Aug 2015 05:41:14 -0400 References: <1438159544-6224-1-git-send-email-zhang.zhanghailiang@huawei.com> <1438159544-6224-20-git-send-email-zhang.zhanghailiang@huawei.com> <55E0DCFB.2070402@redhat.com> From: zhanghailiang Message-ID: <55E41E1D.7020500@huawei.com> Date: Mon, 31 Aug 2015 17:27:57 +0800 MIME-Version: 1.0 In-Reply-To: <55E0DCFB.2070402@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH COLO-Frame v8 19/34] qmp event: Add event notification for COLO error 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, Michael Roth On 2015/8/29 6:13, Eric Blake wrote: > On 07/29/2015 02:45 AM, zhanghailiang wrote: >> If some errors happen during VM's COLO FT stage, it's import to notify the users > > s/import/important/ > >> this event, Togehter with 'colo_lost_heartbeat', users can intervene in COLO's > > s/this event,/of this event./ > s/Togehter/Together/ > >> failover work immediately. >> If users don't want to get involved in COLO's failover verdict, >> it is still necessary to notify users that we exit COLO mode. >> >> Cc: Markus Armbruster >> Cc: Michael Roth >> Signed-off-by: zhanghailiang >> Signed-off-by: Li Zhijian >> --- >> docs/qmp/qmp-events.txt | 16 ++++++++++++++++ >> migration/colo.c | 11 ++++++++++- >> qapi/event.json | 15 +++++++++++++++ >> 3 files changed, 41 insertions(+), 1 deletion(-) >> > > Interface review: > >> +++ b/docs/qmp/qmp-events.txt >> @@ -488,6 +488,22 @@ Example: >> {"timestamp": {"seconds": 1432121972, "microseconds": 744001}, >> "event": "MIGRATION", "data": {"status": "completed"}} >> >> +COLO_EXIT >> +--------- > > This file is alphabetical prior to your patch; please insert your event > prior to DEVICE_DELETED. > >> + >> +Emitted when VM finish COLO mode due to some errors happening or > > s/finish/finishes/ > >> +the request of users. >> + >> +Data: None. >> + >> + - "mode": COLO mode, 'primary' or 'secondary' >> + - "error": Error message (json-string, optional) >> + >> +Example: >> + >> +{"timestamp": {"seconds": 2032141960, "microseconds": 417172}, >> + "event": "COLO_EXIT", "data": {"mode": "primary"}} > > It might also be useful to provide a machine-parseable parameter on > whether the exit was due to an internal error vs. an external request. > Maybe by adding 'flag':'bool', since presence or absence of 'error' is > not as nice. > Good idea, but i think the type of 'flag' changes to 'str' maybe better, which can be 'error' or 'request', 'bool' is not so self-explanatory. The original reason for adding 'error' member is to help users to know what exactly happen, but it seems to be difficult to exactly class all errors, so i will remove it. >> +++ b/migration/colo.c > >> @@ -581,7 +590,7 @@ out: >> error_report("Secondary VM will take over work"); >> break; >> } >> - usleep(200*1000); >> + usleep(200 * 1000); >> } > > Unrelated whitespace tweak. Please squash this hunk into the patch that > introduced the problem. > OK, will fix in next version. >> +++ b/qapi/event.json >> @@ -255,6 +255,21 @@ >> 'data': {'status': 'MigrationStatus'}} >> >> ## >> +# @COLO_EXIT >> +# >> +# Emitted when VM finish COLO mode due to some errors happening or > > s/finish/finishes/ > >> +# the request of users. >> +# >> +# @mode: 'primary' or 'secondeary'. > > s/secondeary/secondary/ > >> +# >> +# @error: #optional, error message. Only present on error happening. >> +# >> +# Since: 2.4 > > 2.5 > >> +## >> +{ 'event': 'COLO_EXIT', >> + 'data': {'mode': 'str', '*error':'str'}} > > Please use 'mode':'COLOMode' (we already have an enum; for a finite set > of strings, it's nicer to use the enum than the open-coded 'str'). > I will fix all the above typos and other problems in next version, thanks very much.