block: Fix open flags with BDRV_O_SNAPSHOT

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 revealed 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 <jan.kiszka@web.de>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

block.c

@@ -774,6 +774,16 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs)
     bs->copy_on_read--;
 }
 
+/*
+ * 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,
+                    flags, bdrv_qcow2, &local_err);
-                    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) {
+    if (snapshot_flags) {
-        bdrv_append_temp_snapshot(bs, &local_err);
+        bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             goto close_and_fail;

include/block/block.h

@@ -195,7 +195,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);

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

tests/qemu-iotests/051.out

@@ -356,6 +356,16 @@ wrote 4096/4096 bytes at offset 0
 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 (qemu) qququiquit
 
+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 "wriqemu-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 "write -P 0x33qemu-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