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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6285 bytes --]

On 23.10.2015 16:11, Kevin Wolf wrote:
> 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.)

Makes sense. I'll probably write one with an annotation that it's only
to be used by this function.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]