| Message ID | 1454645888-28826-8-git-send-email-xiecl.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On 2016/2/5 12:18, Changlong Xie wrote: > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com> > --- > 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 <wency@cn.fujitsu.com> > + * > + * 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. > + 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 <wency@cn.fujitsu.com> > + * > + * 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 */ >
On 02/15/2016 08:57 AM, Hailiang Zhang wrote: > On 2016/2/5 12:18, Changlong Xie wrote: >> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com> >> --- >> 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 <wency@cn.fujitsu.com> >> + * >> + * 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. Thanks Wen Congyang > >> + 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 <wency@cn.fujitsu.com> >> + * >> + * 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 */ >> > > > > > . >
On 02/15/2016 08:57 AM, Hailiang Zhang wrote: > On 2016/2/5 12:18, Changlong Xie wrote: >> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com> >> --- >> 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 <wency@cn.fujitsu.com> >> + * >> + * 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. > >> + 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. You are right, will fix in next version. Thanks -Xie > >> + 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 <wency@cn.fujitsu.com> >> + * >> + * 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 */ >> > > > > > . >
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 <wency@cn.fujitsu.com> >>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >>> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com> >>> --- >>> 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 <wency@cn.fujitsu.com> >>> + * >>> + * 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 <wency@cn.fujitsu.com> >>> + * >>> + * 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 */ >>> >> >> >> >> >> . >> > > > > > . >
On 02/19/2016 04:41 PM, Hailiang Zhang wrote: > 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 <wency@cn.fujitsu.com> >>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >>>> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com> >>>> --- >>>> 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 <wency@cn.fujitsu.com> >>>> + * >>>> + * 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. Where is do_checkpoint? > I'd like these public APIs with a 'colo_' prefix, I hope this API can also be used by other solution such as microcheckpoint. > > 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). This api is used by block driver, so qemu-nbd, qemu-img also needs it. Thanks Wen Congyang > > 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 <wency@cn.fujitsu.com> >>>> + * >>>> + * 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 */ >>>> >>> >>> >>> >>> >>> . >>> >> >> >> >> >> . >> > > > > > . >
On Fri, Feb 05, 2016 at 12:18:06PM +0800, Changlong Xie wrote: > 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 <wency@cn.fujitsu.com> > + * > + * 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); These typedefs are likely to collide with system headers. Please prefix the name. That said, I don't think Start, Stop, Checkpoint, GetError are necessary. Just declare the prototypes in struct ReplicationOps. No user passes single function pointers, they always pass ReplicationOps. Therefore it's not necessary to declare types for these functions at all. > + > +struct ReplicationState { > + void *opaque; > + ReplicationOps *ops; > + QLIST_ENTRY(ReplicationState) node; > +}; > + > +struct ReplicationOps{ > + Start start; > + Checkpoint checkpoint; > + GetError get_error; > + Stop stop; > +}; Please add doc comments that explain the semantics of these functions. For examples, see include/qom/object.h. This documentation should allow someone implementing a new replication listener to understand the constraints and the purpose of these functions.
On 03/05/2016 12:13 AM, Stefan Hajnoczi wrote: > On Fri, Feb 05, 2016 at 12:18:06PM +0800, Changlong Xie wrote: >> 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 <wency@cn.fujitsu.com> >> + * >> + * 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); > Hi Stefan Thanks for your comments. > These typedefs are likely to collide with system headers. Please prefix > the name. > > That said, I don't think Start, Stop, Checkpoint, GetError are > necessary. Just declare the prototypes in struct ReplicationOps. No > user passes single function pointers, they always pass ReplicationOps. > Therefore it's not necessary to declare types for these functions at > all. Will try that. > >> + >> +struct ReplicationState { >> + void *opaque; >> + ReplicationOps *ops; >> + QLIST_ENTRY(ReplicationState) node; >> +}; >> + >> +struct ReplicationOps{ >> + Start start; >> + Checkpoint checkpoint; >> + GetError get_error; >> + Stop stop; >> +}; > > Please add doc comments that explain the semantics of these functions. > For examples, see include/qom/object.h. This documentation should allow > someone implementing a new replication listener to understand the > constraints and the purpose of these functions. Surely Thanks -Xie >
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 <wency@cn.fujitsu.com> + * + * 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) +{ + 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) { + 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 <wency@cn.fujitsu.com> + * + * 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 */