Re: [Qemu-devel] [RFC PATCH 05/14] quorom: implement block driver interfaces for block replication

[Max Reitz <mreitz@redhat.com>]

On 2015-02-11 at 22:07, Wen Congyang 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>
> ---
>   block/quorum.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 69 insertions(+)
>
> diff --git a/block/quorum.c b/block/quorum.c
> index e6aff5f..c8479b4 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -1070,6 +1070,71 @@ static void quorum_refresh_filename(BlockDriverState *bs)
>       bs->full_open_options = opts;
>   }
>   
> +static int quorum_stop_replication(BlockDriverState *bs);
> +static int quorum_start_replication(BlockDriverState *bs, int mode)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    int ret = -1, i;

Again, I'd prefer it if you used -errno (or the Error API).

> +
> +    /*
> +     * TODO: support COLO_SECONDARY_MODE if we allow secondary
> +     * QEMU becoming primary QEMU.
> +     */
> +    if (mode != COLO_PRIMARY_MODE) {
> +        return -1;
> +    }
> +
> +    if (s->read_pattern != QUORUM_READ_PATTERN_FIRST) {
> +        return -1;
> +    }
> +
> +    /* NBD client should not be the first child */
> +    if (bdrv_start_replication(s->bs[0], mode) == 0) {

If you allow the NBD client to be the first child you can probably drop 
this block (and start from "i = 0" in the for loop).

> +        bdrv_stop_replication(s->bs[0]);
> +        return -1;
> +    }
> +
> +    for (i = 1; i < s->num_children; i++) {
> +        if (bdrv_start_replication(s->bs[i], mode) == 0) {
> +            ret++;
> +        }
> +    }
> +
> +    if (ret > 0) {
> +        quorum_stop_replication(bs);
> +    }

I think it would be easier to read if you had an additional "count" 
variable which is set to 0 before the for loop and then incremented 
(instead of ret). This would then be "if (count > 1)".

> +
> +    return ret ? -1 : 0;

And this would be "return count == 0 ? 0 : -ENOTSUP" or something.

But apart from that, what's so bad about having multiple children which 
support bdrv_start_replication()? I mean, other than "It's not what we 
intended".

Max

> +}
> +
> +static int quorum_do_checkpoint(BlockDriverState *bs)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    int i;
> +
> +    for (i = 1; i < s->num_children; i++) {
> +        if (bdrv_do_checkpoint(s->bs[i]) == 0) {
> +            return 0;
> +        }
> +    }
> +
> +    return -1;
> +}
> +
> +static int quorum_stop_replication(BlockDriverState *bs)
> +{
> +    BDRVQuorumState *s = bs->opaque;
> +    int ret = -1, i;
> +
> +    for (i = 0; i < s->num_children; i++) {
> +        if (bdrv_stop_replication(s->bs[i]) == 0) {
> +            ret++;
> +        }
> +    }
> +
> +    return ret ? -1 : 0;
> +}
> +
>   static BlockDriver bdrv_quorum = {
>       .format_name                        = "quorum",
>       .protocol_name                      = "quorum",
> @@ -1093,6 +1158,10 @@ static BlockDriver bdrv_quorum = {
>   
>       .is_filter                          = true,
>       .bdrv_recurse_is_first_non_filter   = quorum_recurse_is_first_non_filter,
> +
> +    .bdrv_start_replication             = quorum_start_replication,
> +    .bdrv_do_checkpoint                 = quorum_do_checkpoint,
> +    .bdrv_stop_replication              = quorum_stop_replication,
>   };
>   
>   static void bdrv_quorum_init(void)