Re: [Qemu-devel] [PATCH v2 27/45] qmp: add drive-mirror command

[Kevin Wolf]

Am 26.09.2012 17:56, schrieb Paolo Bonzini:
> This adds the monitor commands that start the mirroring job.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  blockdev.c       | 125 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  hmp-commands.hx  |  21 ++++++++++
>  hmp.c            |  28 +++++++++++++
>  hmp.h            |   1 +
>  qapi-schema.json |  33 +++++++++++++++
>  qmp-commands.hx  |  42 +++++++++++++++++++
>  6 file modificati, 249 inserzioni(+). 1 rimozione(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 9069ca1..722aab5 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1118,6 +1117,130 @@ void qmp_block_stream(const char *device, bool 
> has_base,
>      trace_qmp_block_stream(bs, bs->job);
>  }
>  
> +void qmp_drive_mirror(const char *device, const char *target,
> +                      bool has_format, const char *format,
> +                      enum MirrorSyncMode sync,
> +                      bool has_mode, enum NewImageMode mode,
> +                      bool has_speed, int64_t speed, Error **errp)
> +{
> +    BlockDriverInfo bdi;
> +    BlockDriverState *bs;
> +    BlockDriverState *source, *target_bs;
> +    BlockDriver *proto_drv;
> +    BlockDriver *drv = NULL;
> +    Error *local_err = NULL;
> +    int flags;
> +    uint64_t size;
> +    int ret;
> +
> +    if (!has_speed) {
> +        speed = 0;
> +    }
> +    if (!has_mode) {
> +        mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
> +    }
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    if (!has_format) {
> +        format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : 
> bs->drv->format_name;

bs->drv can be NULL for removable media. (Worth a test case?)

> +    }
> +    if (format) {
> +        drv = bdrv_find_format(format);
> +        if (!drv) {
> +            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> +            return;
> +        }
> +    }
> +
> +    if (!bdrv_is_inserted(bs)) {
> +        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
> +        return;
> +    }
> +
> +    if (bdrv_in_use(bs)) {
> +        error_set(errp, QERR_DEVICE_IN_USE, device);
> +        return;
> +    }
> +
> +    flags = bs->open_flags | BDRV_O_RDWR;

The two questions from last time are still open:

Jeff's patches are in now, so we can do a bdrv_reopen() to remove
BDRV_O_RDWR again when completing the mirror job.

The other thing was the throttling. It's not entirely clear to me what
our conclusion was, but you suggested to remove it entirely from the
mirror because I/O throttling on the target is equivalent. Of course,
you can only throttle the target as soon as it has a name. What was your
plan with that?

> +    source = bs->backing_hd;
> +    if (!source && sync == MIRROR_SYNC_MODE_TOP) {
> +        sync = MIRROR_SYNC_MODE_FULL;
> +    }
> +
> +    proto_drv = bdrv_find_protocol(target);
> +    if (!proto_drv) {
> +        error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);

Not a great error message for this case.

Kevin