Re: [Qemu-devel] [Qemu-block] [PATCH 2/7] block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags

[Jeff Cody]

On Wed, Sep 14, 2016 at 06:52:15PM +0300, Alberto Garcia wrote:
> If an image is opened with snapshot=on, its flags are modified by
> bdrv_backing_options() and then bs->open_flags is updated accordingly.
> This last step is unnecessary if we calculate the new flags before
> setting bs->open_flags.
> 
> Soon we'll introduce the "read-only" option, and then we'll need to be
> able to modify its value in the QDict when snapshot=on. This is more
> cumbersome if bs->options is already set. This patch simplifies that.
> 
> The code that sets BDRV_O_ALLOW_RDWR is also moved for the same
> reason.
> 

Before, we would not set BDRV_O_ALLOW_RDWR for protocols, but this will
change that.  Is that side-affect intentional?

> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>  block.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 1c75a6f..7cae841 100644
> --- a/block.c
> +++ b/block.c
> @@ -1627,6 +1627,17 @@ static BlockDriverState *bdrv_open_inherit(const char 
> *filename,
>          goto fail;
>      }
>  
> +    if (flags & BDRV_O_RDWR) {
> +        flags |= BDRV_O_ALLOW_RDWR;
> +    }
> +
> +    if (flags & BDRV_O_SNAPSHOT) {
> +        snapshot_options = qdict_new();
> +        bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
> +                                   flags, options);
> +        bdrv_backing_options(&flags, options, flags, options);
> +    }
> +
>      bs->open_flags = flags;
>      bs->options = options;
>      options = qdict_clone_shallow(options);
> @@ -1651,18 +1662,6 @@ static BlockDriverState *bdrv_open_inherit(const char 
> *filename,
>  
>      /* Open image file without format layer */
>      if ((flags & BDRV_O_PROTOCOL) == 0) {
> -        if (flags & BDRV_O_RDWR) {
> -            flags |= BDRV_O_ALLOW_RDWR;
> -        }
> -        if (flags & BDRV_O_SNAPSHOT) {
> -            snapshot_options = qdict_new();
> -            bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
> -                                       flags, options);
> -            bdrv_backing_options(&flags, options, flags, options);
> -        }
> -
> -        bs->open_flags = flags;
> -
>          file = bdrv_open_child(filename, options, "file", bs,
>                                 &child_file, true, &local_err);
>          if (local_err) {