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

[Changlong Xie <xiecl.fnst@cn.fujitsu.com>]

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