Re: [Qemu-devel] [PATCH 1/3] blockdev: Add read-only option to change-blockdev

[Eric Blake]

On 11/20/2014 05:44 AM, Max Reitz wrote:
> Add an option to qmp_change_blockdev() which allows changing the
> read-only status of the block device to be changed.
> 
> Some drives do not have a inherently fixed read-only status; for
> instance, floppy disks can be set read-only or writable independently of
> the drive. Some users may find it useful to be able to therefore change
> the read-only status of a block device when changing the medium.
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  blockdev.c                | 41 ++++++++++++++++++++++++++++++++++++++---
>  include/sysemu/blockdev.h |  3 ++-
>  qapi-schema.json          | 20 ++++++++++++++++++++
>  qmp.c                     |  3 ++-
>  4 files changed, 62 insertions(+), 5 deletions(-)
> 

>  
> -    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, errp);
> +    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, &err);
> +
> +    if (err) {
> +        if (read_only == BLOCKDEV_CHANGE_READ_ONLY_MODE_AUTO) {
> +            error_free(err);
> +            err = NULL;
> +
> +            /* RDWR did not work, try RO now */
> +            bdrv_flags &= ~BDRV_O_RDWR;
> +            qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, 
> errp);
> +        } else {
> +            error_propagate(errp, err);
> +        }

Umm, why are you propagating the error here manually, when it was
previously propagated as part of the fall-through into the out: label?
Particularly since the second open_encrypted call still relies on
fallthrough for propagating the error?  I think this should be
simplified to:

if (err && read_only == BLOCKDEV_CHANGE_READ_ONLY_MODE_AUTO) {
    error_free(err);
    err = NULL;
    /* RDWR did not work, try RO now */
    bdrv_flags &= ~BDRV_O_RDWR;
    qmp_bdrv_open_encrypted(...);
}

Otherwise, looks okay to me.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature