Re: [Qemu-devel] [PATCH 1/7] block: correctly set the keep_read_only flag

[Jeff Cody]

On 09/05/2012 08:47 AM, Kevin Wolf wrote:
> Am 30.08.2012 20:47, schrieb Jeff Cody:
>> I believe the bs->keep_read_only flag is supposed to reflect
>> the initial open state of the device. If the device is initially
>> opened R/O, then commit operations, or reopen operations changing
>> to R/W, are prohibited.
>>
>> Currently, the keep_read_only flag is only accurate for the active
>> layer, and its backing file. Subsequent images end up always having
>> the keep_read_only flag set.
>>
>> For instance, what happens now:
>>
>> [  base  ]  kro = 1, ro = 1
>>     |
>>     v
>> [ snap-1 ]  kro = 1, ro = 1
>>     |
>>     v
>> [ snap-2 ]  kro = 0, ro = 1
>>     |
>>     v
>> [ active ]  kro = 0, ro = 0
>>
>> What we want:
>>
>> [  base  ]  kro = 0, ro = 1
>>     |
>>     v
>> [ snap-1 ]  kro = 0, ro = 1
>>     |
>>     v
>> [ snap-2 ]  kro = 0, ro = 1
>>     |
>>     v
>> [ active ]  kro = 0, ro = 0
>>
>> Signed-off-by: Jeff Cody <address@hidden>
>> ---
>>  block.c | 16 +++++++---------
>>  block.h |  1 +
>>  2 files changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 470bdcc..e31b76f 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -655,7 +655,7 @@ static int bdrv_open_common(BlockDriverState *bs, const 
>> char *filename,
>>       * Clear flags that are internal to the block layer before opening the
>>       * image.
>>       */
>> -    open_flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING);
>> +    open_flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | 
>> BDRV_O_ALLOW_RDWR);
>>  
>>      /*
>>       * Snapshots should be writable.
>> @@ -664,8 +664,6 @@ static int bdrv_open_common(BlockDriverState *bs, const 
>> char *filename,
>>          open_flags |= BDRV_O_RDWR;
>>      }
>>  
>> -    bs->keep_read_only = bs->read_only = !(open_flags & BDRV_O_RDWR);
>> -
>>      /* Open the image, either directly or using a protocol */
>>      if (drv->bdrv_file_open) {
>>          ret = drv->bdrv_file_open(bs, filename, open_flags);
>> @@ -804,6 +802,12 @@ int bdrv_open(BlockDriverState *bs, const char 
>> *filename, int flags,
>>          goto unlink_and_fail;
>>      }
>>  
>> +    if (flags & BDRV_O_RDWR) {
>> +        flags |= BDRV_O_ALLOW_RDWR;
>> +    }
>> +
>> +    bs->keep_read_only = !(flags & BDRV_O_ALLOW_RDWR);
> 
> Do we still need bs->keep_read_only or is it duplicated in
> bs->open_flags now? We can convert all users in a follow-up patch.
>

I think we can ditch keep_read_only, and just use BDRV_O_ALLOW_RDWR.  The
only other user of keep_read_only is the original bdrv_commit(), in
block.c.  And, the natural way to convert that function would be to have
it use bdrv_reopen(), to change from r/o->r/w.