Patchwork [12/14] qapi: introduce change-blockdev

login
register
mail settings
Submitter Anthony Liguori
Date Aug. 24, 2011, 6:43 p.m.
Message ID <1314211389-28915-13-git-send-email-aliguori@us.ibm.com>
Download mbox | patch
Permalink /patch/111408/
State New
Headers show

Comments

Anthony Liguori - Aug. 24, 2011, 6:43 p.m.
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.

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(-)
Kevin Wolf - Aug. 25, 2011, 12:46 p.m.
Am 24.08.2011 20:43, schrieb Anthony Liguori:
> 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.

Changes to semantics should be mentioned in the commit message, and in
more detail than just that they are sane (the documentation in the
schema file doesn't describe differences to the old command).

Just assuming that you are right about sanity, I wonder how sanity and
compatibility could possibly be achieved at once... I suspect the
semantics is clearer now in that it doesn't overload one command with
block and VNC subcommands, but I doubt that the semantics of the block
version is much better than before.

So please call the command drive_change, it's similar to drive_add and
drive_del, not to the future blockdev_add/remove.

> 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);
> +}

Can't this share code with the old command? I don't like this duplication.

Kevin
Anthony Liguori - Aug. 25, 2011, 12:56 p.m.
On 08/25/2011 07:46 AM, Kevin Wolf wrote:
> Am 24.08.2011 20:43, schrieb Anthony Liguori:
>> 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.
>
> Changes to semantics should be mentioned in the commit message, and in
> more detail than just that they are sane (the documentation in the
> schema file doesn't describe differences to the old command).

Right, but the schema describes the old command.

There are a couple major differences with the new command vs old:

1) If you had a block device named 'vnc', the old command is broken.

2) For an encrypted device, the old change command returns an error but 
has semantically completed the operation.  The usage is:

(qemu) change ide0 foo.img
Error: device ide0 is encrypted
(qemu) block_passwd ide0 secret

Because even though an error is returned, the change operation has 
actually succeeded.  The new command has transactional semantics.  If an 
error is returned, nothing has happened.  To set passwords, an 
additional option is supported to specify the password.  So you would do:

(qemu) change-blockdev ide0 foo.img
Error: device ide0 is encrypted
(qemu) change-blockdev ide0 foo.img secret
(qemu)

In addition, change-blockdev will not allow you to perform unsafe 
probing.  You can only change to a raw device if you specify a raw format.

>
> Just assuming that you are right about sanity, I wonder how sanity and
> compatibility could possibly be achieved at once... I suspect the
> semantics is clearer now in that it doesn't overload one command with
> block and VNC subcommands, but I doubt that the semantics of the block
> version is much better than before.

I don't see how we can change the behavior of the change command.

> So please call the command drive_change, it's similar to drive_add and
> drive_del, not to the future blockdev_add/remove.

Ack.

>> 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);
>> +}
>
> Can't this share code with the old command? I don't like this duplication.

qmp_bdrv_open_encrypted() is the code that can be shared.  The next 
patch I think changes the old command to use this function.

Regards,

Anthony Liguori

> Kevin
>
Kevin Wolf - Aug. 25, 2011, 1:47 p.m.
Am 25.08.2011 14:56, schrieb Anthony Liguori:
> On 08/25/2011 07:46 AM, Kevin Wolf wrote:
>> Am 24.08.2011 20:43, schrieb Anthony Liguori:
>>> 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.
>>
>> Changes to semantics should be mentioned in the commit message, and in
>> more detail than just that they are sane (the documentation in the
>> schema file doesn't describe differences to the old command).
> 
> Right, but the schema describes the old command.
> 
> There are a couple major differences with the new command vs old:
> 
> 1) If you had a block device named 'vnc', the old command is broken.
> 
> 2) For an encrypted device, the old change command returns an error but 
> has semantically completed the operation.  The usage is:
> 
> (qemu) change ide0 foo.img
> Error: device ide0 is encrypted
> (qemu) block_passwd ide0 secret
> 
> Because even though an error is returned, the change operation has 
> actually succeeded.  The new command has transactional semantics.  If an 
> error is returned, nothing has happened.  To set passwords, an 
> additional option is supported to specify the password.  So you would do:
> 
> (qemu) change-blockdev ide0 foo.img
> Error: device ide0 is encrypted
> (qemu) change-blockdev ide0 foo.img secret
> (qemu)

Does it really work like this? We need to improve the HMP implementation
to ask for the password when needed instead of requiring users to put it
as plain text in their command.

> In addition, change-blockdev will not allow you to perform unsafe 
> probing.  You can only change to a raw device if you specify a raw format.

Makes sense for QMP, but I would consider it a bug for HMP. So HMP code
should deal with it and allow using raw without specifying the format.

qemu is getting worse and worse to use without a management tool. :-(

Kevin
Anthony Liguori - Aug. 25, 2011, 1:50 p.m.
On 08/25/2011 08:47 AM, Kevin Wolf wrote:
> Am 25.08.2011 14:56, schrieb Anthony Liguori:
>> On 08/25/2011 07:46 AM, Kevin Wolf wrote:
>>> Am 24.08.2011 20:43, schrieb Anthony Liguori:
>>>> 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.
>>>
>>> Changes to semantics should be mentioned in the commit message, and in
>>> more detail than just that they are sane (the documentation in the
>>> schema file doesn't describe differences to the old command).
>>
>> Right, but the schema describes the old command.
>>
>> There are a couple major differences with the new command vs old:
>>
>> 1) If you had a block device named 'vnc', the old command is broken.
>>
>> 2) For an encrypted device, the old change command returns an error but
>> has semantically completed the operation.  The usage is:
>>
>> (qemu) change ide0 foo.img
>> Error: device ide0 is encrypted
>> (qemu) block_passwd ide0 secret
>>
>> Because even though an error is returned, the change operation has
>> actually succeeded.  The new command has transactional semantics.  If an
>> error is returned, nothing has happened.  To set passwords, an
>> additional option is supported to specify the password.  So you would do:
>>
>> (qemu) change-blockdev ide0 foo.img
>> Error: device ide0 is encrypted
>> (qemu) change-blockdev ide0 foo.img secret
>> (qemu)
>
> Does it really work like this? We need to improve the HMP implementation
> to ask for the password when needed instead of requiring users to put it
> as plain text in their command.

I didn't add an HMP version of change-blockdev.  change is fine for HMP.

And change does prompt for password both for VNC and block.  The main 
reason for introducing change-blockdev because the only way I could make 
the HMP change command work and be implemented in terms of QMP was to 
use the change-blockdev command.

>
>> In addition, change-blockdev will not allow you to perform unsafe
>> probing.  You can only change to a raw device if you specify a raw format.
>
> Makes sense for QMP, but I would consider it a bug for HMP. So HMP code
> should deal with it and allow using raw without specifying the format.

Yup, that's how change behaves.

> qemu is getting worse and worse to use without a management tool. :-(

One thing that's different now is that HMP and QMP are fundamentally 
different interfaces.  I hope this gives us the flexibility to implement 
better HMP commands that we would never expose in QMP and vice versa.

As long as HMP commands are implemented in terms of QMP commands, we 
should be very open and aggressive about improving HMP usablity.

Regards,

Anthony Liguori

>
> Kevin
>
Luiz Capitulino - Aug. 25, 2011, 2:09 p.m.
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
>  ------
Anthony Liguori - Aug. 25, 2011, 2:21 p.m.
On 08/25/2011 09:09 AM, Luiz Capitulino wrote:
> 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.

You mean:

     if (local_err) {
         qerror_report_err(local_err);
         error_free(local_err);
         return -1;
     }
     return 0;

That should behave exactly the right way as qerror_report_err() 
propagates sends the Error as a QError.

>> +
>> +##
>> +# @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.

We do, but that needs to be a command because it depends on the build 
options.

>> +#
>> +# @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.

Do we need to set args_type for QMP?

Regards,

Anthony Liguori


>> +        .params     = "device filename [format] [password]",
>> +        .help       = "change a removable medium, optional format",
>> +        .mhandler.cmd_new = qmp_marshal_input_change_blockdev,
>> +    },
>> +
>>   SQMP
>>   change
>>   ------
>
Luiz Capitulino - Aug. 25, 2011, 2:52 p.m.
On Thu, 25 Aug 2011 09:21:02 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> On 08/25/2011 09:09 AM, Luiz Capitulino wrote:
> > 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.
> 
> You mean:
> 
>      if (local_err) {
>          qerror_report_err(local_err);
>          error_free(local_err);
>          return -1;
>      }
>      return 0;
> 
> That should behave exactly the right way as qerror_report_err() 
> propagates sends the Error as a QError.

Oh that's right, what's happening is that qerror_report_err() is returning
before setting qerror in the Monitor object... I reported that in my
review of patch 01/14.

> 
> >> +
> >> +##
> >> +# @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.
> 
> We do, but that needs to be a command because it depends on the build 
> options.
> 
> >> +#
> >> +# @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.
> 
> Do we need to set args_type for QMP?

Yes.

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

Patch

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
+#
+# @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?",
+        .params     = "device filename [format] [password]",
+        .help       = "change a removable medium, optional format",
+        .mhandler.cmd_new = qmp_marshal_input_change_blockdev,
+    },
+
 SQMP
 change
 ------