Re: [Qemu-devel] [PATCH] block: Set cdrom device read only flag

[Kevin Wolf]

Am 13.08.2012 09:54, schrieb Kevin Wolf:
> Am 12.08.2012 04:48, schrieb Kevin Shanahan:
>> So qmp_change_blockdev uses bdrv_is_read_only() to check whether to
>> try and open the backing file read only, which uses the ->read_only
>> member of struct BlockDriverState to decide whether to pass the
>> BDRV_O_RDRW flag to qmp_bdrv_open_encypted() and then bdrv_open().
>>
>> I would assume we want to set this flag in drive_init() when the block
>> driver state is initialised. How about a patch like this instead?
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 8669142..ba22064 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -526,6 +526,7 @@ DriveInfo *drive_init(QemuOpts *opts, int 
>> default_to_scsi)
>>                       if_name[type], mediastr, unit_id);
>>      }
>>      dinfo->bdrv = bdrv_new(dinfo->id);
>> +    dinfo->bdrv->read_only = ro;
>>      dinfo->devaddr = devaddr;
>>      dinfo->type = type;
>>      dinfo->bus = bus_id;
> 
> Ah, yes, this looks much more like the proper fix. Basically we need to
> set everything that is retained after a 'change' command. We have this
> code in qmp_change_blockdev():
> 
>     bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
>     bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
> 
>     qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, errp);
> 
> bdrv_is_read_only() is covered by your patch, bdrv_is_snapshot()
> additionally requires bs->open_flags to be right.

Kevin, would you like to resend this as a proper patch with a
Signed-off-by line?

If you like, you can also add a dinfo->bdrv->open_flags = snapshot ?
BDRV_O_SNAPSHOT : 0. This would fix the second bug I mentioned above,
that snapshot=on is lost on empty drives.

Kevin