Re: [Qemu-devel] [PATCH v7 33/39] blockdev: Implement change with basic operations

[Kevin Wolf <kwolf@redhat.com>]

Am 19.10.2015 um 17:53 hat Max Reitz geschrieben:
> Implement 'change' on block devices by calling blockdev-open-tray,
> blockdev-remove-medium, blockdev-insert-medium (a variation of that
> which does not need a node-name) and blockdev-close-tray.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  blockdev.c | 184 +++++++++++++++++++++++++------------------------------------
>  1 file changed, 74 insertions(+), 110 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 0481686..50e5e74 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1901,44 +1901,6 @@ exit:
>      }
>  }
>  
> -
> -static void eject_device(BlockBackend *blk, int force, Error **errp)
> -{
> -    BlockDriverState *bs = blk_bs(blk);
> -    AioContext *aio_context;
> -
> -    if (!bs) {
> -        /* No medium inserted, so there is nothing to do */
> -        return;
> -    }
> -
> -    aio_context = bdrv_get_aio_context(bs);
> -    aio_context_acquire(aio_context);
> -
> -    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_EJECT, errp)) {
> -        goto out;
> -    }
> -    if (!blk_dev_has_removable_media(blk)) {
> -        error_setg(errp, "Device '%s' is not removable",
> -                   bdrv_get_device_name(bs));
> -        goto out;
> -    }
> -
> -    if (blk_dev_is_medium_locked(blk) && !blk_dev_is_tray_open(blk)) {
> -        blk_dev_eject_request(blk, force);
> -        if (!force) {
> -            error_setg(errp, "Device '%s' is locked",
> -                       bdrv_get_device_name(bs));
> -            goto out;
> -        }
> -    }
> -
> -    bdrv_close(bs);
> -
> -out:
> -    aio_context_release(aio_context);
> -}
> -
>  void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
>  {
>      Error *local_err = NULL;
> @@ -1976,78 +1938,6 @@ void qmp_block_passwd(bool has_device, const char *device,
>      aio_context_release(aio_context);
>  }
>  
> -/* Assumes AioContext is held */
> -static void qmp_bdrv_open_encrypted(BlockDriverState **pbs,
> -                                    const char *filename,
> -                                    int bdrv_flags, const char *format,
> -                                    const char *password, Error **errp)
> -{
> -    Error *local_err = NULL;
> -    QDict *options = NULL;
> -    int ret;
> -
> -    if (format) {
> -        options = qdict_new();
> -        qdict_put(options, "driver", qstring_from_str(format));
> -    }
> -
> -    ret = bdrv_open(pbs, filename, NULL, options, bdrv_flags, &local_err);
> -    if (ret < 0) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> -
> -    bdrv_add_key(*pbs, password, errp);
> -}
> -
> -void qmp_change_blockdev(const char *device, const char *filename,
> -                         const char *format, Error **errp)
> -{
> -    BlockBackend *blk;
> -    BlockDriverState *bs;
> -    AioContext *aio_context;
> -    int bdrv_flags;
> -    bool new_bs;
> -    Error *err = NULL;
> -
> -    blk = blk_by_name(device);
> -    if (!blk) {
> -        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> -                  "Device '%s' not found", device);
> -        return;
> -    }
> -    bs = blk_bs(blk);
> -    new_bs = !bs;
> -
> -    aio_context = blk_get_aio_context(blk);
> -    aio_context_acquire(aio_context);
> -
> -    eject_device(blk, 0, &err);
> -    if (err) {
> -        error_propagate(errp, err);
> -        goto out;
> -    }
> -
> -    bdrv_flags = blk_is_read_only(blk) ? 0 : BDRV_O_RDWR;
> -    bdrv_flags |= blk_get_root_state(blk)->open_flags & ~BDRV_O_RDWR;
> -
> -    qmp_bdrv_open_encrypted(&bs, filename, bdrv_flags, format, NULL, &err);
> -    if (err) {
> -        error_propagate(errp, err);
> -        goto out;
> -    }
> -
> -    if (new_bs) {
> -        blk_insert_bs(blk, bs);
> -        /* Has been sent automatically by bdrv_open() if blk_bs(blk) was not
> -         * NULL */
> -        blk_dev_change_media_cb(blk, true);
> -    }
> -
> -out:
> -    aio_context_release(aio_context);
> -}
> -
>  void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
>                              Error **errp)
>  {
> @@ -2204,6 +2094,80 @@ void qmp_blockdev_insert_medium(const char *device, const char *node_name,
>      qmp_blockdev_insert_anon_medium(device, bs, errp);
>  }
>  
> +void qmp_change_blockdev(const char *device, const char *filename,
> +                         const char *format, Error **errp)
> +{
> +    BlockBackend *blk;
> +    BlockBackendRootState *blk_rs;
> +    BlockDriverState *medium_bs = NULL;
> +    int bdrv_flags, ret;
> +    QDict *options = NULL;
> +    Error *err = NULL;
> +
> +    blk = blk_by_name(device);
> +    if (!blk) {
> +        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> +                  "Device '%s' not found", device);
> +        goto fail;
> +    }
> +
> +    if (blk_bs(blk)) {
> +        blk_update_root_state(blk);
> +    }
> +
> +    blk_rs = blk_get_root_state(blk);
> +    bdrv_flags = blk_rs->read_only ? 0 : BDRV_O_RDWR;
> +    bdrv_flags |= blk_rs->open_flags & ~BDRV_O_RDWR;
> +
> +    if (format) {
> +        options = qdict_new();
> +        qdict_put(options, "driver", qstring_from_str(format));
> +    }
> +
> +    assert(!medium_bs);
> +    ret = bdrv_open(&medium_bs, filename, NULL, options, bdrv_flags, errp);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
> +    medium_bs->detect_zeroes = blk_rs->detect_zeroes;
> +    if (blk_rs->throttle_group) {
> +        bdrv_io_limits_enable(medium_bs, blk_rs->throttle_group);
> +    }

Would it make sense to have a blk_apply_root_state() that does these
updates? Otherwise you have to keep this place up to date if the struct
is ever extended. (Which would mean that we forgot something, so
hopefully not.)

Kevin