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

[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.

* 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 leads to:

1. Replace monitor command change.

2. Optional: look for defaults to improve.