Re: [Qemu-devel] [PATCH 5/8] block: add drive-mirror command

[Eric Blake]

On 04/13/2012 10:23 AM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  blockdev.c       |  102 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  hmp-commands.hx  |   21 +++++++++++
>  hmp.c            |   26 ++++++++++++++
>  hmp.h            |    1 +
>  qapi-schema.json |   30 ++++++++++++++++
>  qmp-commands.hx  |   36 +++++++++++++++++++
>  trace-events     |    2 +-
>  7 files changed, 214 insertions(+), 4 deletions(-)
> 

> +    if (!has_format) {
> +        format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : "qcow2";
> +    }

Do we always want to default to qcow2, or should we be defaulting to the
same format as the file we are copying?  For example, if I have a raw
source, and don't pass in format, wouldn't it make more sense for the
destination to also default to raw, instead of qcow2?  Same if the
source is qed, shouldn't this default to a qed copy?

> +        case NEW_IMAGE_MODE_ABSOLUTE_PATHS:
> +            ret = bdrv_img_create(target, format,
> +                                  source->filename,
> +                                  source->drv->format_name,
> +                                  NULL, -1, flags);
> +            break;
> +        default:
> +            abort();
> +        }
> +    }
> +
> +    if (ret) {
> +        error_set(errp, QERR_OPEN_FILE_FAILED, target);
> +        return;
> +    }

In this first failure, we've leaked nothing...

> +
> +    /* Grab a reference so hotplug does not delete the BlockDriverState
> +     * from underneath us.
> +     */
> +    drive_get_ref(drive_get_by_blockdev(bs));
> +    ret = mirror_start(bs, target, drv, flags,
> +                       block_job_cb, bs, full);
> +    if (ret < 0) {
> +        error_set(errp, QERR_OPEN_FILE_FAILED, target);
> +    }

But here, you have the same error message, and I don't see anything that
closed target (unless mirror_start does that).  Leak?

> +++ b/hmp-commands.hx
> @@ -901,6 +901,27 @@ Snapshot device, using snapshot file as target if 
> provided
>  ETEXI
>  
>      {
> +        .name       = "drive_mirror",
> +        .args_type  = "reuse:-n,full:-f,device:B,target:s,format:s?",
> +        .params     = "[-n] [-f] device target [format]",
> +        .help       = "initiates live storage\n\t\t\t"
> +                      "migration for a device. New writes are mirrored to 
> the\n\t\t\t"
> +                      "specified new image file, and the 
> block_stream\n\t\t\t"
> +                      "command can then be started.\n\t\t\t"

Outdated help message.

> +++ b/qapi-schema.json
> @@ -1245,6 +1245,36 @@
>    'returns': 'str' } 
>  
>  ##
> +# @drive-mirror
> +#
> +# Start mirroring a block device's writes to a new destination.
> +#
> +# @device:  the name of the device whose writes should be mirrored.
> +#
> +# @target: the target of the new image. If the file exists, or if it
> +#          is a device, the existing file/device will be used as the new
> +#          destination.  If it does not exist, a new file will be created.
> +#
> +# @format: the format of the new destination.

Mark this #optional, and mention how it defaults to probing for
'mode':'existing' and to qcow2 for all other modes.

> +#
> +# @mode: #optional whether and how QEMU should create a new image, default is
> +# 'absolute-paths'.
> +#
> +# @full: whether the whole disk should be copied to the destination, or
> +#        only the topmost image.
> +#
> +# Returns: nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#          If @target can't be opened, OpenFileFailed
> +#          If @format is invalid, InvalidBlockFormat
> +#
> +# Since 1.1
> +##
> +{ 'command': 'drive-mirror',
> +  'data': { 'device': 'str', 'target': 'str', '*format': 'str',
> +            'full': 'bool', '*mode': 'NewImageMode'} }

I'm wondering if we should make 'full' optional (default to false).
After all, in the original blkmirror design for adding 'drive-mirror' to
'transaction', there was no 'full' parameter, and a transaction of
blockdev-snapshot-sync/drive-mirror on the same device depended on a
shallow copy.  I can live with it being mandatory, though.

Per my comments on 0/8, should we be adding an optional '*base': 'str'
(even if we don't implement it yet) so that you can get the full
functionality of 'block-stream' in a single 'drive-mirror' command
rather than having to do two separate pulls to achieve a partial copy?

> +SQMP
> +drive-mirror
> +------------
> +
> +Start mirroring a block device's writes to a new destination. target
> +specifies the target of the new image. If the file exists, or if it is
> +a device, it will be used as the new destination for writes. If does not
> +exist, a new file will be created. format specifies the format of the
> +mirror image, default is to probe if mode='existing', else qcow2.
> +
> +Arguments:
> +
> +- "device": device name to operate on (json-string)
> +- "target": name of new image file (json-string)
> +- "format": format of new image (json-string)

(json-string, optional)

-- 
Eric Blake   address@hidden    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature