From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39736) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aWgeA-00031B-P1 for qemu-devel@nongnu.org; Fri, 19 Feb 2016 03:42:44 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aWge9-000495-Cd for qemu-devel@nongnu.org; Fri, 19 Feb 2016 03:42:42 -0500 References: <1454645888-28826-1-git-send-email-xiecl.fnst@cn.fujitsu.com> <1454645888-28826-8-git-send-email-xiecl.fnst@cn.fujitsu.com> <56C12264.1060207@huawei.com> <56C12621.3090902@cn.fujitsu.com> From: Hailiang Zhang Message-ID: <56C6D54B.10408@huawei.com> Date: Fri, 19 Feb 2016 16:41:47 +0800 MIME-Version: 1.0 In-Reply-To: <56C12621.3090902@cn.fujitsu.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v15 7/9] Introduce new APIs to do replication operation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wen Congyang , Changlong Xie Cc: Kevin Wolf , Fam Zheng , qemu block , qemu devel , Jiang Yunhong , Dong Eddie , peter.huangpeng@huawei.com, "Michael R. Hines" , Max Reitz , Gonglei , Stefan Hajnoczi , Paolo Bonzini , "Dr. David Alan Gilbert" Hi, On 2016/2/15 9:13, Wen Congyang wrote: > On 02/15/2016 08:57 AM, Hailiang Zhang wrote: >> On 2016/2/5 12:18, Changlong Xie wrote: >>> Signed-off-by: Wen Congyang >>> Signed-off-by: zhanghailiang >>> Signed-off-by: Gonglei >>> Signed-off-by: Changlong Xie >>> --- >>> Makefile.objs | 1 + >>> qapi/block-core.json | 13 ++++++++ >>> replication.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> replication.h | 53 +++++++++++++++++++++++++++++ >>> 4 files changed, 161 insertions(+) >>> create mode 100644 replication.c >>> create mode 100644 replication.h >>> >>> diff --git a/Makefile.objs b/Makefile.objs >>> index 06b95c7..a8c74b7 100644 >>> --- a/Makefile.objs >>> +++ b/Makefile.objs >>> @@ -15,6 +15,7 @@ block-obj-$(CONFIG_POSIX) += aio-posix.o >>> block-obj-$(CONFIG_WIN32) += aio-win32.o >>> block-obj-y += block/ >>> block-obj-y += qemu-io-cmds.o >>> +block-obj-y += replication.o >>> >>> block-obj-m = block/ >>> >>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>> index 7e9e8fe..12362b8 100644 >>> --- a/qapi/block-core.json >>> +++ b/qapi/block-core.json >>> @@ -2002,6 +2002,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.6 >>> +## >>> +{ 'enum' : 'ReplicationMode', 'data' : [ 'primary', 'secondary' ] } >>> + >>> +## >>> # @BlockdevOptions >>> # >>> # Options for creating a block device. >>> diff --git a/replication.c b/replication.c >>> new file mode 100644 >>> index 0000000..e8ac2f0 >>> --- /dev/null >>> +++ b/replication.c >>> @@ -0,0 +1,94 @@ >>> +/* >>> + * Replication filter >>> + * >>> + * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD. >>> + * Copyright (c) 2016 Intel Corporation >>> + * Copyright (c) 2016 FUJITSU LIMITED >>> + * >>> + * Author: >>> + * Wen Congyang >>> + * >>> + * 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" >>> + >>> +static QLIST_HEAD(, ReplicationState) replication_states; >>> + >>> +ReplicationState *replication_new(void *opaque, ReplicationOps *ops) >>> +{ >>> + ReplicationState *rs; >>> + >>> + rs = g_new0(ReplicationState, 1); >>> + rs->opaque = opaque; >>> + rs->ops = ops; >>> + QLIST_INSERT_HEAD(&replication_states, rs, node); >>> + >>> + return rs; >>> +} >>> + >>> +void replication_remove(ReplicationState *rs) >>> +{ >>> + QLIST_REMOVE(rs, node); >>> + g_free(rs); >>> +} >>> + >>> +/* >>> + * The caller of the function *MUST* make sure vm stopped >>> + */ >>> +void replication_start_all(ReplicationMode mode, Error **errp) >>> +{ >> >> Is this public API is only used for block ? >> If yes, I'd like it with a 'block_' prefix. > > No, we hope it can be used for nic too. > OK, i got why you designed these APIs, I like this idea that use the callback/notifier to notify the status of COLO for block/nic. But let's do something more graceful. For COLO, we can consider it has four states: Prepare/start checkpoint(with VM stopped)/finish checkpoint (with VM resumed)/failover. So here we need four operation functions according to these four states. I'd like these public APIs with a 'colo_' prefix, What about colo_replication_prepare()/colo_replication_start_checkpoint()/ colo_replication_finish_checkpoint()/colo_replication_failover() ? Besides, It's better to rename the replicaton.c, and move it to the 'migration' folder. Because it seems to be only used by COLO, maybe colo_ops.c or maybe another choice move all these codes to colo-comm.c. (If we do this, we need to merge colo-comm.c first as prerequisite). Thanks, Hailiang > >> >>> + ReplicationState *rs, *next; >>> + >>> + QLIST_FOREACH_SAFE(rs, &replication_states, node, next) { >>> + if (rs->ops && rs->ops->start) { >>> + rs->ops->start(rs, mode, errp); >>> + } >>> + if (*errp != NULL) { >> >> This is incorrect, you miss checking if errp is NULL, >> if errp is NULL, there will be an error that accessing memory at address 0x0. >> Same with other places in this patch. >> >>> + return; >>> + } >>> + } >>> +} >>> + >>> +void replication_do_checkpoint_all(Error **errp) >>> +{ >>> + ReplicationState *rs, *next; >>> + >>> + QLIST_FOREACH_SAFE(rs, &replication_states, node, next) { >>> + if (rs->ops && rs->ops->checkpoint) { >>> + rs->ops->checkpoint(rs, errp); >>> + } >>> + if (*errp != NULL) { >>> + return; >> >>> + } >>> + } >>> +} >>> + >>> +void replication_get_error_all(Error **errp) >>> +{ >>> + ReplicationState *rs, *next; >>> + >>> + QLIST_FOREACH_SAFE(rs, &replication_states, node, next) { >>> + if (rs->ops && rs->ops->get_error) { >>> + rs->ops->get_error(rs, errp); >>> + } >>> + if (*errp != NULL) { >> >>> + return; >>> + } >>> + } >>> +} >>> + >>> +void replication_stop_all(bool failover, Error **errp) >>> +{ >>> + ReplicationState *rs, *next; >>> + >>> + QLIST_FOREACH_SAFE(rs, &replication_states, node, next) { >>> + if (rs->ops && rs->ops->stop) { >>> + rs->ops->stop(rs, failover, errp); >>> + } >>> + if (*errp != NULL) { >>> + return; >>> + } >>> + } >>> +} >>> diff --git a/replication.h b/replication.h >>> new file mode 100644 >>> index 0000000..faea649 >>> --- /dev/null >>> +++ b/replication.h >>> @@ -0,0 +1,53 @@ >>> +/* >>> + * Replication filter >>> + * >>> + * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD. >>> + * Copyright (c) 2016 Intel Corporation >>> + * Copyright (c) 2016 FUJITSU LIMITED >>> + * >>> + * Author: >>> + * Wen Congyang >>> + * >>> + * 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 "sysemu/sysemu.h" >>> + >>> +typedef struct ReplicationOps ReplicationOps; >>> +typedef struct ReplicationState ReplicationState; >>> +typedef void (*Start)(ReplicationState *rs, ReplicationMode mode, Error **errp); >>> +typedef void (*Stop)(ReplicationState *rs, bool failover, Error **errp); >>> +typedef void (*Checkpoint)(ReplicationState *rs, Error **errp); >>> +typedef void (*GetError)(ReplicationState *rs, Error **errp); >>> + >>> +struct ReplicationState { >>> + void *opaque; >>> + ReplicationOps *ops; >>> + QLIST_ENTRY(ReplicationState) node; >>> +}; >>> + >>> +struct ReplicationOps{ >>> + Start start; >>> + Checkpoint checkpoint; >>> + GetError get_error; >>> + Stop stop; >>> +}; >>> + >>> + >>> +ReplicationState *replication_new(void *opaque, ReplicationOps *ops); >>> + >>> +void replication_remove(ReplicationState *rs); >>> + >>> +void replication_start_all(ReplicationMode mode, Error **errp); >>> + >>> +void replication_do_checkpoint_all(Error **errp); >>> + >>> +void replication_get_error_all(Error **errp); >>> + >>> +void replication_stop_all(bool failover, Error **errp); >>> + >>> +#endif /* REPLICATION_H */ >>> >> >> >> >> >> . >> > > > > > . >