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

[Kevin Wolf]

Am 13.08.2012 13:57, schrieb Markus Armbruster:
> Kevin Wolf <address@hidden> writes:
> 
>> 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.
>>
>> Markus, how will this look in the -blockdev world? There seem to be
>> properties that belong to host state, but are not coupled to a medium.
> 
> Really?
> 
> Read-only is clearly a property of the medium.  There's a separate
> read-only belonging to the device.
> 
> A CD-ROM medium is read-only, even when loaded in an optical disk drive
> that can burn.
> 
> An optical disk drive that can't burn is read-only, even when writable
> medium is loaded.
> 
> 
> Here's how I believe it should work:
> 
> * BDS member read_only describes the medium.
> 
>   * block.h gets it right: you specify read-only with bdrv_open(), not
>     with bdrv_new().
> 
>   * -drive gets it right: parameter readonly gets ignored unless we're
>     defining media.  Not nice: it's ignored silently.
> 
>   * Monitor command change gets it wrong: it doesn't let you specify
>     the new medium's read-only-ness.
> 
>     Easy enough to fix for QMP, just add a suitable argument.  Even
>     better: create a new, non-multiplexed command, and let "change" rot
>     in peace.
> 
>     Not sure about the best way to fix it in the human monitor.

I think it's mostly the human monitor that needs this state. Of course,
users will always want to have it, but in the case of QMP this work is
offloaded to libvirt.

So people expect that if they have started with a block device
read-only, it remains read-only after they switch to a different image
file ("change the medium"). They also expect that if they initially
started with snapshot=on, the newly loaded image file will inherit this
flag and be snapshotted as well.

We know these expectations because 'change' didn't always retain these
properties (or probably did so initially, but was broken at some point),
and people complained, so it was fixed. So we can't just break it. This
works today.

Kevin's bug is about retaining the properties also when initially
there's no medium in the drive, and it would probably be reasonable
enough to say that we shouldn't lose them either when at runtime a
medium is first ejected so that the drive is empty, and then a new
medium is inserted.

As always ignoring requirements is the easiest way, but it doesn't make
users happy.

> * Device model has its own read-only predicate.
> 
>   * If a device model comes in both a read-only and a read-write
>     flavor, it should have a bool property "readonly".
> 
>   * A device model can only use a backend with a suitable media state
>     (has media, is read-only, ...).  For instance, ide-hd can't use a
>     read-only backend.
> 
>   * Media change disabled while backend is attached to a device model
>     with fixed media.
> 
> * Convenient defaults (optional)
> 
>   * A device model property "readonly" could default to the media's
>     read-only-ness.
> 
>   * Monitor command change could default to readonly when the backend
>     is attached to a device model that can't write.
> 
>   * Both -drive and change could default to readonly when the image
>     isn't writable.

This doesn't give us today's behaviour with devices that can do both
read-only and r/w media. Most common case for it are probably CD-ROMs
which can never write, so it comes relatively close at least (at least
until someone implements burning CDs in our emulation...)

It doesn't do anything about snapshot=on.

Kevin