Re: [Qemu-devel] [RFC PATCH 07/14] NBD client: 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/nbd.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 55 insertions(+)

So by now to me it looks like you're using bdrv_start_replication() on 
the primary VM to start transferring data through the NBD connection to 
the secondary VM.

I guess this is what you were discussing with Fam and John, whether 
there'd be a better way to do it by using functionality that is already 
(or is about to become) part of qemu, right?

> diff --git a/block/nbd.c b/block/nbd.c
> index 19b9200..1ff6ecf 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -445,6 +445,58 @@ static void nbd_refresh_filename(BlockDriverState *bs)
>       bs->full_open_options = opts;
>   }
>   
> +static int nbd_start_replication(BlockDriverState *bs, int mode)
> +{
> +    BDRVNBDState *s = bs->opaque;
> +    Error *local_err = NULL;
> +    int ret;
> +
> +    /*
> +     * TODO: support COLO_SECONDARY_MODE if we allow secondary
> +     * QEMU becoming primary QEMU.
> +     */
> +    if (mode != COLO_PRIMARY_MODE) {
> +        return -1;

Once again, I'd like -ENOTSUP more (or -EINVAL or whatever you prefer).

> +    }
> +
> +    if (s->connected) {
> +        return -1;
> +    }
> +
> +    /* TODO: NBD client should be one child of quorum, how to verify it? */

Again, why would you care about that? Other than "It's how it's intended 
to be used".

> +    ret = nbd_connect_server(bs, &local_err);
> +    if (local_err) {
> +        error_free(local_err);
> +    }

If you'd use the Error API you'd be able to propagate the error.

Max

> +
> +    return ret;
> +}
> +
> +static int nbd_do_checkpoint(BlockDriverState *bs)
> +{
> +    BDRVNBDState *s = bs->opaque;
> +
> +    if (!s->connected) {
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +static int nbd_stop_replication(BlockDriverState *bs)
> +{
> +    BDRVNBDState *s = bs->opaque;
> +
> +    if (!s->connected) {
> +        return -1;
> +    }
> +
> +    nbd_client_session_close(&s->client);
> +    s->connected = false;
> +
> +    return 0;
> +}
> +
>   static BlockDriver bdrv_nbd = {
>       .format_name                = "nbd",
>       .protocol_name              = "nbd",
> @@ -514,6 +566,9 @@ static BlockDriver bdrv_nbd_colo = {
>       .bdrv_detach_aio_context    = nbd_detach_aio_context,
>       .bdrv_attach_aio_context    = nbd_attach_aio_context,
>       .bdrv_refresh_filename      = nbd_refresh_filename,
> +    .bdrv_start_replication     = nbd_start_replication,
> +    .bdrv_do_checkpoint         = nbd_do_checkpoint,
> +    .bdrv_stop_replication      = nbd_stop_replication,
>   
>       .has_variable_length        = true,
>   };