From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56503) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCHEL-0002Rx-L1 for qemu-devel@nongnu.org; Sun, 12 Jun 2016 22:03:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bCHEK-0008An-Df for qemu-devel@nongnu.org; Sun, 12 Jun 2016 22:03:57 -0400 Message-ID: <575E157E.2080503@cn.fujitsu.com> Date: Mon, 13 Jun 2016 10:07:58 +0800 From: Changlong Xie MIME-Version: 1.0 References: <1465348292-26750-1-git-send-email-xiecl.fnst@cn.fujitsu.com> <1465348292-26750-8-git-send-email-xiecl.fnst@cn.fujitsu.com> <57582070.6020508@redhat.com> In-Reply-To: <57582070.6020508@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v20 07/10] Introduce new APIs to do replication operation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu devel , Stefan Hajnoczi , Fam Zheng , Max Reitz , Kevin Wolf , Jeff Cody Cc: qemu block , Paolo Bonzini , John Snow , Markus Armbruster , "Dr. David Alan Gilbert" , Dong Eddie , Jiang Yunhong , zhanghailiang , Gonglei , Wen Congyang On 06/08/2016 09:41 PM, Eric Blake wrote: > On 06/07/2016 07:11 PM, Changlong Xie wrote: >> Signed-off-by: Wen Congyang >> Signed-off-by: zhanghailiang >> Signed-off-by: Gonglei >> Signed-off-by: Changlong Xie > > No mention of the API names in the commit message? Grepping 'git log' > is easier if there is something useful to grep for. I'll use a brief description, such as below: This commit introduces six replication interfaces(for block, network etc). Firstly we can use replication_(new/remove) to create/destroy replication instances, then in migration we can use replication_(start/stop/do_checkpoint/get_error)_all to handle all replication operations. More detail please refer to replication.h > >> --- >> Makefile.objs | 1 + >> qapi/block-core.json | 13 ++++ >> replication.c | 105 ++++++++++++++++++++++++++++++ >> replication.h | 176 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 295 insertions(+) >> create mode 100644 replication.c >> create mode 100644 replication.h >> > >> +++ b/qapi/block-core.json >> @@ -2032,6 +2032,19 @@ >> '*read-pattern': 'QuorumReadPattern' } } >> >> ## >> +# @ReplicationMode >> +# >> +# An enumeration of replication modes. >> +# >> +# @primary: Primary mode, the vm's state will be sent to secondary QEMU. >> +# >> +# @secondary: Secondary mode, receive the vm's state from primary QEMU. >> +# >> +# Since: 2.7 >> +## >> +{ 'enum' : 'ReplicationMode', 'data' : [ 'primary', 'secondary' ] } >> + > > This part is fine, from an interface point of view. However, I have not > closely reviewed the rest of the patch or series. That said, here's > some quick things that caught my eye. Appreciate. > > >> +++ b/replication.c >> @@ -0,0 +1,105 @@ >> +/* >> + * Replication filter >> + * >> + * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD. >> + * Copyright (c) 2016 Intel Corporation >> + * Copyright (c) 2016 FUJITSU LIMITED >> + * >> + * Author: >> + * Changlong Xie >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2 or later. >> + * See the COPYING file in the top-level directory. >> + */ >> + >> +#include "replication.h" > > All new .c files must include "qemu/osdep.h" first. > > >> +++ b/replication.h >> @@ -0,0 +1,176 @@ >> +/* >> + * Replication filter >> + * >> + * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD. >> + * Copyright (c) 2016 Intel Corporation >> + * Copyright (c) 2016 FUJITSU LIMITED >> + * >> + * Author: >> + * Changlong Xie >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2 or later. >> + * See the COPYING file in the top-level directory. >> + */ >> + >> +#ifndef REPLICATION_H >> +#define REPLICATION_H >> + >> +#include "qemu/osdep.h" > > And .h files must NOT include osdep.h. > >> +#include "qapi/error.h" > > Do you really need the full error.h, or is typedefs.h enough to get the > forward declaration of Error? It's for error_propagate() in replication.c, and in summary to your comments on replication.h/replication.c, i'll 1) remove all uncorrelated *.h in replication.h 2) use following header files in replication.c #include "qemu/osdep.h" #include "qapi/error.h" #include "qemu/queue.h" #include "replication.h" Thanks >