Re: [Qemu-devel] [PATCH 12/14] qapi: introduce change-blockdev

[Luiz Capitulino <lcapitulino@redhat.com>]

On Wed, 24 Aug 2011 13:43:07 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> A new QMP only command to change the blockdev associated with a block device.
> The semantics of change right now are just plain scary.  This command introduces
> sane semantics.  For more details, see the documentation in the schema file.

IMO, this has to be split into two commits. First you introduce the new command
and then you fix do_change_block().

Also, there's a small problem with the generated code: it has to return -1 on
errors. The monitor expects a -1 return _and_ a qerror object on errors. The
generated code in middle mode is returning 0 even on errors, which makes QMP to
emit a UndefinedError error by default.

(it's probably not worth it to discuss the merits of having two ways of
signaling errors in the monitor, as this is all going to be dropped pretty soon).

> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  blockdev.c       |  108 +++++++++++++++++++++++++++++++++++++++++++++++++++---
>  qapi-schema.json |   30 +++++++++++++++
>  qmp-commands.hx  |    8 ++++
>  3 files changed, 140 insertions(+), 6 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 37b2f29..c00c69d 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -697,12 +697,101 @@ void qmp_block_passwd(const char *device, const char *password, Error **err)
>      qmp_set_blockdev_password(device, password, err);
>  }
>  
> +static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
> +                                    int bdrv_flags, BlockDriver *drv,
> +                                    const char *password, Error **errp)
> +{
> +    if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) {
> +        error_set(errp, QERR_OPEN_FILE_FAILED, filename);
> +        return;
> +    }
> +
> +    if (bdrv_key_required(bs)) {
> +        if (password) {
> +            if (bdrv_set_key(bs, password) < 0) {
> +                error_set(errp, QERR_INVALID_PASSWORD);
> +            }
> +        } else {
> +            error_set(errp, QERR_DEVICE_ENCRYPTED, bdrv_get_device_name(bs),
> +                      bdrv_get_encrypted_filename(bs));
> +        }
> +    } else if (password) {
> +        error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs));
> +    }
> +}
> +
> +void qmp_change_blockdev(const char *device, const char *filename,
> +                         bool has_format, const char *format,
> +                         bool has_password, const char *password,
> +                         Error **errp)
> +{
> +    BlockDriverState *bs, *bs1;
> +    BlockDriver *drv = NULL;
> +    int bdrv_flags;
> +    Error *err = NULL;
> +    bool probed_raw = false;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    if (has_format) {
> +        drv = bdrv_find_whitelisted_format(format);
> +        if (!drv) {
> +            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
> +            return;
> +        }
> +    }
> +
> +    bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
> +    bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
> +
> +    if (!has_password) {
> +        password = NULL;
> +    }
> +
> +    /* Try to open once with a temporary block device to make sure that
> +     * the disk isn't encrypted and we lack a key.  This also helps keep
> +     * this operation as a transaction.  That is, if we fail, we try very
> +     * hard to make sure that the state is the same as before the operation
> +     * was started.
> +     */
> +    bs1 = bdrv_new("");
> +    qmp_bdrv_open_encrypted(bs1, filename, bdrv_flags, drv, password, &err);
> +    if (!has_format && bs1->drv->unsafe_probe) {
> +        probed_raw = true;
> +    }
> +    bdrv_delete(bs1);
> +
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    if (probed_raw) {
> +        /* We will not auto probe raw files. */
> +        error_set(errp, QERR_MISSING_PARAMETER, "format");
> +        return;
> +    }
> +
> +    eject_device(bs, 0, &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, password, errp);
> +}
> +
>  int do_change_block(Monitor *mon, const char *device,
>                      const char *filename, const char *fmt)
>  {
>      BlockDriverState *bs;
>      BlockDriver *drv = NULL;
>      int bdrv_flags;
> +    Error *err = NULL;
>  
>      bs = bdrv_find(device);
>      if (!bs) {
> @@ -716,16 +805,23 @@ int do_change_block(Monitor *mon, const char *device,
>              return -1;
>          }
>      }
> -    if (eject_device(bs, 0, NULL) < 0) {
> -        return -1;
> -    }
> +
>      bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
>      bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
> -    if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) {
> -        qerror_report(QERR_OPEN_FILE_FAILED, filename);
> +
> +    eject_device(bs, 0, &err);
> +    if (err) {
> +        qerror_report_err(err);
> +        return -1;
> +    }
> +
> +    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, &err);
> +    if (err) {
> +        qerror_report_err(err);
>          return -1;
>      }
> -    return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
> +
> +    return 0;
>  }
>  
>  int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 211200a..139c6e3 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -123,3 +123,33 @@
>  #         when this command is executed is undefined.
>  ##
>  { 'command': 'change-vnc-listen', 'data': {'target': 'str'} }
> +
> +##
> +# @change-blockdev:
> +#
> +# This is the preferred interface for changing a block device.
> +#
> +# @device:   The block device name.
> +#
> +# @filename: The new filename for the block device.  This may contain options
> +#            encoded in a format specified by @format.
> +#
> +# @format:   #optional The format to use open the block device

We need a list (or a pointers) of valid formats.

> +#
> +# @password: #optional The password to use if the block device is encrypted
> +#
> +# Returns:  Nothing on success.
> +#          If @device is not a valid block device, DeviceNotFound
> +#          If @format is not a valid block format, InvalidBlockFormat
> +#          If @filename is encrypted and @password isn't supplied,
> +#            DeviceEncrypted.  The call should be repeated with @password
> +#            supplied.
> +#          If @format is not specified and @filename is a format that cannot
> +#            be safely probed, MissingParameter.
> +#          If @filename cannot be opened, OpenFileFailed
> +#
> +# Since: 1.0
> +##
> +{ 'command': 'change-blockdev',
> +  'data': {'device': 'str', 'filename': 'str', '*format': 'str',
> +           '*password': 'str'} }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index c0d0ca3..cec7135 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -121,6 +121,14 @@ EQMP
>          .mhandler.cmd_new = do_change,
>      },
>  
> +    {
> +        .name       = "change-blockdev",
> +        .args_type  = "device:B,target:F,arg:s?pass:s?",

The arguments names don't match the schema.

> +        .params     = "device filename [format] [password]",
> +        .help       = "change a removable medium, optional format",
> +        .mhandler.cmd_new = qmp_marshal_input_change_blockdev,
> +    },
> +
>  SQMP
>  change
>  ------