Re: [Qemu-devel] [PATCH v20 07/10] Introduce new APIs to do replication operation

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2777 bytes --]

On 06/07/2016 07:11 PM, 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>

No mention of the API names in the commit message?  Grepping 'git log'
is easier if there is something useful to grep for.

> ---
>  Makefile.objs        |   1 +
>  qapi/block-core.json |  13 ++++
>  replication.c        | 105 ++++++++++++++++++++++++++++++
>  replication.h        | 176 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 295 insertions(+)
>  create mode 100644 replication.c
>  create mode 100644 replication.h
> 

> +++ b/qapi/block-core.json
> @@ -2032,6 +2032,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.7
> +##
> +{ 'enum' : 'ReplicationMode', 'data' : [ 'primary', 'secondary' ] }
> +

This part is fine, from an interface point of view. However, I have not
closely reviewed the rest of the patch or series.  That said, here's
some quick things that caught my eye.


> +++ b/replication.c
> @@ -0,0 +1,105 @@
> +/*
> + * Replication filter
> + *
> + * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
> + * Copyright (c) 2016 Intel Corporation
> + * Copyright (c) 2016 FUJITSU LIMITED
> + *
> + * Author:
> + *   Changlong Xie <xiecl.fnst@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"

All new .c files must include "qemu/osdep.h" first.


> +++ b/replication.h
> @@ -0,0 +1,176 @@
> +/*
> + * Replication filter
> + *
> + * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
> + * Copyright (c) 2016 Intel Corporation
> + * Copyright (c) 2016 FUJITSU LIMITED
> + *
> + * Author:
> + *   Changlong Xie <xiecl.fnst@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 "qemu/osdep.h"

And .h files must NOT include osdep.h.

> +#include "qapi/error.h"

Do you really need the full error.h, or is typedefs.h enough to get the
forward declaration of Error?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]