Re: [Qemu-devel] [PATCH] block: Fix open flags with BDRV_O_SNAPSHOT

[Jan Kiszka]

On 2014-05-06 12:19, Kevin Wolf wrote:
> The immediately visible effect of this patch is that it fixes committing
> a temporary snapshot to its backing file. Previously, it would fail with
> a "permission denied" error because bdrv_inherited_flags() forced the
> backing file to be read-only, ignoring the r/w reopen of bdrv_commit().
> 
> The bigger problem this releaved is that the original open flags must
> actually only be applied to the temporary snapshot, and the original
> image file must be treated as a backing file of the temporary snapshot
> and get the right flags for that.
> 
> Reported-by: Jan Kiszka <address@hidden>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block.c                    | 34 +++++++++++++++++++---------------
>  include/block/block.h      |  2 +-
>  tests/qemu-iotests/051     |  4 ++++
>  tests/qemu-iotests/051.out | 10 ++++++++++
>  4 files changed, 34 insertions(+), 16 deletions(-)
> 
> diff --git a/block.c b/block.c
> index b749d31..c90c71a 100644
> --- a/block.c
> +++ b/block.c
> @@ -775,6 +775,16 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs)
>  }
>  
>  /*
> + * Returns the flags that a temporary snapshot should get, based on the
> + * originally requested flags (the originally requested image will have flags
> + * like a backing file)
> + */
> +static int bdrv_temp_snapshot_flags(int flags)
> +{
> +    return (flags & ~BDRV_O_SNAPSHOT) | BDRV_O_TEMPORARY;
> +}
> +
> +/*
>   * Returns the flags that bs->file should get, based on the given flags for
>   * the parent BDS
>   */
> @@ -787,11 +797,6 @@ static int bdrv_inherited_flags(int flags)
>       * so we can enable both unconditionally on lower layers. */
>      flags |= BDRV_O_CACHE_WB | BDRV_O_UNMAP;
>  
> -    /* The backing file of a temporary snapshot is read-only */
> -    if (flags & BDRV_O_SNAPSHOT) {
> -        flags &= ~BDRV_O_RDWR;
> -    }
> -
>      /* Clear flags that only apply to the top layer */
>      flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_COPY_ON_READ);
>  
> @@ -817,11 +822,6 @@ static int bdrv_open_flags(BlockDriverState *bs, int 
> flags)
>  {
>      int open_flags = flags | BDRV_O_CACHE_WB;
>  
> -    /* The backing file of a temporary snapshot is read-only */
> -    if (flags & BDRV_O_SNAPSHOT) {
> -        open_flags &= ~BDRV_O_RDWR;
> -    }
> -
>      /*
>       * Clear flags that are internal to the block layer before opening the
>       * image.
> @@ -1206,7 +1206,7 @@ done:
>      return ret;
>  }
>  
> -void bdrv_append_temp_snapshot(BlockDriverState *bs, Error **errp)
> +void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
>  {
>      /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
>      char *tmp_filename = g_malloc0(PATH_MAX + 1);
> @@ -1262,8 +1262,7 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, 
> Error **errp)
>      bs_snapshot = bdrv_new("", &error_abort);
>  
>      ret = bdrv_open(&bs_snapshot, NULL, NULL, snapshot_options,
> -                    (bs->open_flags & ~BDRV_O_SNAPSHOT) | BDRV_O_TEMPORARY,
> -                    bdrv_qcow2, &local_err);
> +                    flags, bdrv_qcow2, &local_err);
>      if (ret < 0) {
>          error_propagate(errp, local_err);
>          goto out;
> @@ -1298,6 +1297,7 @@ int bdrv_open(BlockDriverState **pbs, const char 
> *filename,
>      BlockDriverState *file = NULL, *bs;
>      const char *drvname;
>      Error *local_err = NULL;
> +    int snapshot_flags = 0;
>  
>      assert(pbs);
>  
> @@ -1358,6 +1358,10 @@ int bdrv_open(BlockDriverState **pbs, const char 
> *filename,
>      if (flags & BDRV_O_RDWR) {
>          flags |= BDRV_O_ALLOW_RDWR;
>      }
> +    if (flags & BDRV_O_SNAPSHOT) {
> +        snapshot_flags = bdrv_temp_snapshot_flags(flags);
> +        flags = bdrv_backing_flags(flags);
> +    }
>  
>      assert(file == NULL);
>      ret = bdrv_open_image(&file, filename, options, "file",
> @@ -1417,8 +1421,8 @@ int bdrv_open(BlockDriverState **pbs, const char 
> *filename,
>  
>      /* For snapshot=on, create a temporary qcow2 overlay. bs points to the
>       * temporary snapshot afterwards. */
> -    if (flags & BDRV_O_SNAPSHOT) {
> -        bdrv_append_temp_snapshot(bs, &local_err);
> +    if (snapshot_flags) {
> +        bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
>          if (local_err) {
>              error_propagate(errp, local_err);
>              goto close_and_fail;
> diff --git a/include/block/block.h b/include/block/block.h
> index 467fb2b..2fda81c 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -191,7 +191,7 @@ int bdrv_open_image(BlockDriverState **pbs, const char 
> *filename,
>                      QDict *options, const char *bdref_key, int flags,
>                      bool allow_none, Error **errp);
>  int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error 
> **errp);
> -void bdrv_append_temp_snapshot(BlockDriverState *bs, Error **errp);
> +void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error 
> **errp);
>  int bdrv_open(BlockDriverState **pbs, const char *filename,
>                const char *reference, QDict *options, int flags,
>                BlockDriver *drv, Error **errp);
> diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
> index 073dc7a..c4af131 100755
> --- a/tests/qemu-iotests/051
> +++ b/tests/qemu-iotests/051
> @@ -233,6 +233,10 @@ echo 'qemu-io ide0-hd0 "write -P 0x22 0 4k"' | run_qemu 
> -drive file="$TEST_IMG",
>  
>  $QEMU_IO -c "read -P 0x22 0 4k" "$TEST_IMG" | _filter_qemu_io
>  
> +echo -e 'qemu-io ide0-hd0 "write -P 0x33 0 4k"\ncommit ide0-hd0' | run_qemu 
> -drive file="$TEST_IMG",snapshot=on | _filter_qemu_io
> +
> +$QEMU_IO -c "read -P 0x33 0 4k" "$TEST_IMG" | _filter_qemu_io
> +
>  # success, all done
>  echo "*** done"
>  rm -f $seq.full
> diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
> index 01b0384..31e329e 100644
> --- a/tests/qemu-iotests/051.out
> +++ b/tests/qemu-iotests/051.out
> @@ -358,4 +358,14 @@ wrote 4096/4096 bytes at offset 0
>  
>  read 4096/4096 bytes at offset 0
>  4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +Testing: -drive file=TEST_DIR/t.qcow2,snapshot=on
> +QEMU X.Y.Z monitor - type 'help' for more information
> +(qemu) 
> qqeqemqemuqemu-qemu-iqemu-ioqemu-io
>  qemu-io iqemu-io 
> idqemu-io 
> ideqemu-io 
> ide0qemu-io 
> ide0-qemu-io 
> ide0-hqemu-io 
> ide0-hdqemu-io 
> ide0-hd0qemu-io ide0-hd0 
> qemu-io ide0-hd0 
> "qemu-io ide0-hd0 
> "wqemu-io 
> ide0-hd0 
> "wrqemu-io 
> ide0-hd0 "wri!
>  qemu-io ide0-hd0 
> "writqemu-io
>  ide0-hd0 
> "writeqemu-io
>  ide0-hd0 "write 
> qemu-io
>  ide0-hd0 "write 
> -qemu-io
>  ide0-hd0 "write 
> -Pqemu-io
>  ide0-hd0 "write -P 
> qemu-io
>  ide0-hd0 "write -P 
> 0qemu-io
>  ide0-hd0 "write -P 
> 0xqemu-io
>  ide0-hd0 "write -P 
> 0x3qemu-io
>  ide0-hd0 "w!
>  rite -P 0x33
> [Dqemu-io ide0-hd0 "write -P 0x33 
> qemu-io
>  ide0-hd0 "write -P 0x33 
> 0qemu-io
>  ide0-hd0 "write -P 0x33 0 
> qemu-io
>  ide0-hd0 "write -P 0x33 0 
> 4qemu-io
>  ide0-hd0 "write -P 0x33 0 
> 4kqemu-io
>  ide0-hd0 "write -P 0x33 0 4k"
> +wrote 4096/4096 bytes at offset 0
> +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +(qemu) 
> ccocomcommcommicommitcommit
>  commit icommit 
> idcommit 
> idecommit 
> ide0commit 
> ide0-commit 
> ide0-hcommit 
> ide0-hdcommit ide0-hd0
> +(qemu) qququiquit
> +
> +read 4096/4096 bytes at offset 0
> +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  *** done
> 

Works fine here!

(For unknown reason, applying the iotest hunk didn't work for me, though.)

Thanks,
Jan

signature.asc
Description: OpenPGP digital signature