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

[Max Reitz <mreitz@redhat.com>]

On 2014-11-26 at 17:24, Eric Blake wrote:
> 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 <mreitz@redhat.com>
>> ---
>>   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?

Is it? I don't see any error_propagate() after that 
qmp_bdrv_open_encrypted_call()... And also, that call takes "errp" as a 
parameter, so having error_propagate() afterwards would be kind of strange.

The tree I'm looking at is master, commit 
3ef4ebcc5c0360629bb05f49d9b961774247d6ca. In my block-next tree, which 
this patch is actually based against, commit 
656ddfaafd67af2b9234567e51cd3418588b2ddd.

> 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(...);
> }

But then you're not propagating the error anymore (in case of !err || 
read_only != BLOCKDEV_CHANGE_READ_ONLY_MODE_AUTO). As I said, at least 
in my tree, there is no error_propagate(errp, err); after this block. I 
may add it, though.

(to be completely sure: 
http://git.qemu.org/?p=qemu.git;a=blob;f=blockdev.c;h=57910b82c7adc3ce59173afeeebcd37ff2a3dfd0;hb=3ef4ebcc5c0360629bb05f49d9b961774247d6ca#l1727 
and https://github.com/XanClic/qemu/blob/block-next/blockdev.c#L1760)

Max

> Otherwise, looks okay to me.