Re: [Qemu-devel] [PATCH v15 7/9] Introduce new APIs to do replication operation

[Hailiang Zhang <zhang.zhanghailiang@huawei.com>]

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 */
>