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 <jc...@redhat.com>
>> ---
>> 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.
