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

[Kevin Wolf <kwolf@redhat.com>]

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 <jcody@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.

Kevin