Re: [Qemu-devel] [PATCH v3 4/5] qmp: Add blockdev-mirror command

[Max Reitz <mreitz@redhat.com>]

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

On 23.12.2015 06:59, Fam Zheng wrote:
> This will start a mirror job from a named device to another named
> device, its relation with drive-mirror is similar with blockdev-backup
> to drive-backup.
> 
> In blockdev-mirror, the target node should be prepared by blockdev-add,
> which will be responsible for assigning a name to the new node, so
> we don't have 'node-name' parameter.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev.c           | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 47 +++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx      | 48 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 157 insertions(+)

It appears you haven't addressed the comments for v2. I only had a
single one (regarding documentation), but Markus had a couple ones, so
those may be worth addressing.

> 
> diff --git a/blockdev.c b/blockdev.c
> index f42e171..2df0c6d 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3345,6 +3345,10 @@ static void blockdev_mirror_common(BlockDriverState *bs,
>      if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_MIRROR_TARGET, errp)) {
>          return;
>      }
> +    if (target->blk) {
> +        error_setg(errp, "Cannot mirror to an attached block device");
> +        return;
> +    }
>  
>      if (!bs->backing && sync == MIRROR_SYNC_MODE_TOP) {
>          sync = MIRROR_SYNC_MODE_FULL;
> @@ -3518,6 +3522,64 @@ out:
>      aio_context_release(aio_context);
>  }
>  
> +void qmp_blockdev_mirror(const char *device, const char *target,
> +                         bool has_replaces, const char *replaces,
> +                         MirrorSyncMode sync,
> +                         bool has_speed, int64_t speed,
> +                         bool has_granularity, uint32_t granularity,
> +                         bool has_buf_size, int64_t buf_size,
> +                         bool has_on_source_error,
> +                         BlockdevOnError on_source_error,
> +                         bool has_on_target_error,
> +                         BlockdevOnError on_target_error,
> +                         Error **errp)
> +{
> +    BlockDriverState *bs;
> +    BlockBackend *blk;
> +    BlockDriverState *target_bs;
> +    AioContext *aio_context;
> +    Error *local_err = NULL;
> +
> +    blk = blk_by_name(device);
> +    if (!blk) {
> +        error_setg(errp, "Device '%s' not found", device);
> +        return;
> +    }
> +    bs = blk_bs(blk);
> +
> +    if (!bs) {
> +        error_setg(errp, "Device '%s' has no media", device);
> +        return;
> +    }
> +
> +    target_bs = bdrv_lookup_bs(target, target, errp);
> +    if (!target_bs) {
> +        return;
> +    }
> +
> +    aio_context = bdrv_get_aio_context(bs);
> +    aio_context_acquire(aio_context);
> +
> +    bdrv_ref(target_bs);
> +    bdrv_set_aio_context(target_bs, aio_context);
> +
> +    blockdev_mirror_common(bs, target_bs,
> +                           has_replaces, replaces, sync,
> +                           has_speed, speed,
> +                           has_granularity, granularity,
> +                           has_buf_size, buf_size,
> +                           has_on_source_error, on_source_error,
> +                           has_on_target_error, on_target_error,
> +                           true, true,

Shouldn't this be "false, false,", or, ideally, set by the user?

> +                           &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        bdrv_unref(target_bs);
> +    }
> +
> +    aio_context_release(aio_context);
> +}
> +

Max


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