Re: [Qemu-devel] [PATCH 6/8] block: add the drive-reopen command

[Eric Blake]

On 04/13/2012 10:23 AM, Paolo Bonzini wrote:
> From: Federico Simoncelli <address@hidden>
> 
> Signed-off-by: Federico Simoncelli <address@hidden>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  blockdev.c       |   75 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hmp-commands.hx  |   16 ++++++++++++
>  hmp.c            |   11 ++++++++
>  hmp.h            |    1 +
>  qapi-schema.json |   22 ++++++++++++++++
>  qmp-commands.hx  |   30 ++++++++++++++++++++++
>  6 files changed, 155 insertions(+)
> 

>  
> +static void change_blockdev_image(BlockDriverState *bs, const char 
> *new_image_file,
> +                                  const char *format, Error **errp)
> +{
> +    BlockDriver *old_drv, *proto_drv;
> +    BlockDriver *drv = NULL;
> +    int ret = 0;
> +    int flags;
> +    char old_filename[1024];

Hard-coded limit.  Is this going to bite us later, or are we stuck with
this limit in other places for other reasons?  Why a magic number
instead of a macro name or enum value?

> +
> +    if (bdrv_in_use(bs)) {
> +        error_set(errp, QERR_DEVICE_IN_USE, bs->device_name);
> +        return;
> +    }
> +
> +    pstrcpy(old_filename, sizeof(old_filename), bs->filename);
> +
> +    old_drv = bs->drv;
> +    flags = bs->open_flags;

Should we be asserting that flags does not contain BDRV_O_NO_BACKING, so
that we know that we are reopening the entire chain?

> +
> +    bdrv_close(bs);
> +    ret = bdrv_open(bs, new_image_file, flags, drv);

Here's where it is hairy, and a future patch to do things right in a
transaction would be nice (open the new chain in prepare phase, then
close the old chain in commit phase, all while dealing with any possible
overlap between the two chains), but I can live with this for now.

> +    /*
> +     * If reopening the image file we just created fails, fall back
> +     * and try to re-open the original image. If that fails too, we
> +     * are in serious trouble.
> +     */
> +    if (ret != 0) {
> +        ret = bdrv_open(bs, old_filename, flags, old_drv);
> +        if (ret != 0) {
> +            error_set(errp, QERR_OPEN_FILE_FAILED, old_filename);
> +        } else {
> +            error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
> +        }
> +    }

In that worst-case scenario of failing to reopen the old file, should we
be halting the domain and/or propagating an event up to the user,
similar to how we behave on ENOSPC errors?

> +++ b/hmp-commands.hx
> @@ -922,6 +922,22 @@ using the specified target.
>  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",

Really?  I though if you didn't provide format, you get probing, not
forced qcow2.

> +++ b/qapi-schema.json
> @@ -1217,6 +1217,28 @@
>              '*mode': 'NewImageMode'} }
>  
>  ##
> +# @drive-reopen
> +#
> +# Assigns a new image file to a device.
> +#
> +# @device: the name of the device for which we are changing the image file.
> +#
> +# @new-image-file: the target of the new image. If the file doesn't exists 
> the
> +#                  command will fail.

s/exists/exist/

> +#
> +# @format: #optional the format of the new image, default is 'qcow2'.

again, default is to probe, not hard-code qcow2.

> +SQMP
> +drive-reopen
> +------------
> +
> +Assigns a new image file to a device. Except extremely rare cases where the

s/Except extremely/Except in extremely/

> +guest is expecting the drive to change its content, the new image should
> +contain the same data of the current one.  One use case is to terminate
> +a drive-mirror command.
> +

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

signature.asc
Description: OpenPGP digital signature