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

[Kevin Wolf <kwolf@redhat.com>]

Am 14.09.2016 um 17:52 hat Alberto Garcia geschrieben:
> 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.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  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);

Here I think we get different semantics now: bdrv_backing_options() only
affected the cloned QDict before, and now it affects bs->options, too.

Given that the flags are already updated and don't contain
BDRV_O_SNAPSHOT any more, I suspect that this is a silent bug fix. If
so, it might be better to split it out into a separate patch before this
one, or at the very least should be mentioned in the commit message.

If it's not a fix, however, it's probably a bug. :-)

> @@ -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) {

Kevin