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 <f...@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
signature.asc
Description: OpenPGP digital signature
