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

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

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