Re: [Qemu-devel] [PATCH 2/2 v2] Add the blockdev-reopen and blockdev-migrate commands

[Eric Blake]

On 02/24/2012 09:49 AM, Federico Simoncelli wrote:
> Signed-off-by: Federico Simoncelli <address@hidden>

Pretty sparse on the commit message.

> +++ b/hmp-commands.hx
> @@ -886,6 +886,44 @@ Snapshot device, using snapshot file as target if 
> provided
>  ETEXI
>  
>      {
> +        .name       = "drive_reopen",
> +        .args_type  = "device:B,new-image-file:s?,format:s?",
> +        .params     = "device [new-image-file] [format]",
> +        .help       = "Assigns a new image file to a device.\n\t\t\t"
> +                      "The image will be opened using the format\n\t\t\t"
> +                      "specified or 'qcow2' by default.\n\t\t\t"
> +                      "This command is deprecated.",
> +        .mhandler.cmd = hmp_drive_reopen,

Libvirt will just use the QMP version, so I'm not reviewing the HMP
version very closely.

> +++ b/qapi-schema.json
> @@ -1136,6 +1136,60 @@
>    'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str' } }
>  
>  ##
> +# @drive-reopen
> +#
> +# Assigns a new image file to a device.
> +#
> +# @device: the name of the device for which we are chainging the image file.

s/chainging/changing/

> +#
> +# @new-image-file: the target of the new image. If the file doesn't exists 
> the
> +#                  command will fail.

I think you need to be more explicit that @new-image-file MUST have
identical contents as the current image file, for this to be useful, and
that qemu does not validate whether the new image met those conditions.
 Possible ways to achieve this:

1. Freeze all guest I/O, so that you know the current image file is
stable, copy the contents to new-image-file, drive-reopen, then unfreeze
guest I/O

2. Create an empty qcow2 file with backing image of the current image
file.  drive-reopen will then see the new image file as containing the
same contents as the previous image file.

3. Create the new image file via mirroring (blockdev-migrate with
incremental and new-image-file), and reopen to the mirror

> +#
> +# @format: #optional the format of the new image, default is 'qcow2'.
> +#
> +# Returns: nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#          If @new-image-file can't be opened, OpenFileFailed
> +#          If @format is invalid, InvalidBlockFormat
> +#
> +# Notes: This command is deprecated.

Generally, a deprecation note should list the preferred replacement; but
we don't have a replacement, so I don't see this note adding any
information.

> +#
> +# Since 1.1
> +##
> +
> +{ 'command': 'drive-reopen',
> +  'data': { 'device': 'str', 'new-image-file': 'str', '*format': 'str' } }
> +
> +##
> +# @blockdev-migrate
> +#
> +# Migrates the device to a new destination.
> +#
> +# @device: the name of the block device to migrate.
> +#
> +# @incremental: if true only the new writes are sent to the destination.
> +#               This method is particularly useful if used in conjunction
> +#               with new-image-file allowing the current image to be
> +#               transferred to the destination by an external manager.
> +#
> +# @destination: the destination of the block migration.

Where do you list what format the destination is?  Shouldn't this have
an optional format, defaulting to qcow2?  Does qemu create the
destination file, or must it already be existing?

> +#
> +# @new-image-file: #optional an existing image file that will replace
> +#                  the current one in the device.

Where do you list what format the new-image-file is?  Shouldn't this
have an optional format, defaulting to qcow2?  Does qemu create the
new-image-file, or can one already be existing?

I know that this patch is only implementing the case where incremental
is true and new-image-file is provided; but I'm not quite sure what
semantics are intended if incremental is false.  Is that still a case
where this sets up mirroring (writes go to two images) but additionally
the contents from the current image are (asynchronously) streamed into
the destination?  I guess I'm not sure what the intended semantics of
the modes that we aren't implementing, or why we don't just go with a
simpler command that only exposes the semantics that we are implementing.

> +#
> +# Returns: nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#          If @mode is not a valid migration mode, InvalidParameterValue
> +#          If @destination can't be opened, OpenFileFailed
> +#          If @new-image-file can't be opened, OpenFileFailed
> +#
> +# Since 1.1
> +##
> +{ 'command': 'blockdev-migrate',
> +  'data': { 'device': 'str', 'incremental' : 'bool',
> +            'destination': 'str', '*new-image-file': 'str' } }
> +
> +##
>  # @human-monitor-command:
>  #
>  # Execute a command on the human monitor and return the output.

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

signature.asc
Description: OpenPGP digital signature