Re: [Qemu-devel] [PATCH 2/3] block: set bs->read_only before .bdrv_open()

[Kevin Wolf]

Am 27.10.2011 12:45, schrieb Stefan Hajnoczi:
> On Thu, Oct 27, 2011 at 11:18 AM, Kevin Wolf <address@hidden> wrote:
>> Am 27.10.2011 11:54, schrieb Stefan Hajnoczi:
>>> Several block drivers set bs->read_only in .bdrv_open() but
>>> block.c:bdrv_open_common() clobbers its value.  Additionally, QED uses
>>> bdrv_is_read_only() in .bdrv_open() to decide whether to perform
>>> consistency checks.
>>>
>>> The correct ordering is to initialize bs->read_only from the open flags
>>> before calling .bdrv_open().  This way block drivers can override it if
>>> necessary and can use bdrv_is_read_only() in .bdrv_open().
>>>
>>> Signed-off-by: Stefan Hajnoczi <address@hidden>
>>> ---
>>>  block.c |    4 ++--
>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 70aab63..3207e99 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -500,6 +500,8 @@ static int bdrv_open_common(BlockDriverState *bs, const 
>>> char *filename,
>>>          open_flags |= BDRV_O_RDWR;
>>>      }
>>
>> Not directly related, but the context made me wonder when we're making a
>> BlockkDriverState writeable unconditionally. This is the full context:
>>
>>
>>    /*
>>     * Snapshots should be writable.
>>     */
>>    if (bs->is_temporary) {
>>        open_flags |= BDRV_O_RDWR;
>>    }
>>
>> Does anyone understand what the point of this is? If the user requested
>> read-only, he certainly wants to have read-only, even if he specified
>> -snapshot as well.
> 
> Perhaps this is an attempt to support -drive
> file=pristine.img,readonly=on,snapshot=on.  The idea being that the
> user absolutely wants to keep pristine.img unmodified.  But the nature
> of backing files means we should automatically get this.

I would have said that it breaks this command line. It all depends on
what your expectation of the semantics of these options is. Mine would
be that the disk is presented read-only to the guest (and the snapshot
is done but useless).

Kevin