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

[Kevin Wolf]

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 <address@hidden>
> ---
>  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