block: Fix two minor reopen issues

These are fixes for issues I found when looking after something Berto has reported. The second patch fixes that issue Berto found, the first one is only kind of related.

For the first patch: bdrv_reopen_abort() or bdrv_reopen_commit() are called only if the respective bdrv_reopen_prepare() has succeeded. However, bdrv_reopen_prepare() can fail even if the block driver’s .bdrv_reopen_prepare() succeeded. In that case, the block driver is expecting a .bdrv_reopen_abort() or .bdrv_reopen_commit(), but will never get it.

This is fixed by making bdrv_reopen_prepare() call .bdrv_reopen_abort() if an error occurs after .bdrv_reopen_prepare() has already succeeded.

In practice this just prevents resource leaks, so nothing too dramatic (fine for qemu-next). It also means I’ve been incapable of writing a test, sorry.

The second issue is more severe: file-posix applies the inverse share locks when reopening. Now the user can only directly do reopens from the monitor for now, so that wouldn’t be so bad. But of course there are other places in qemu that implicitly reopen nodes, like dropping R/W to RO or gaining R/W from RO. Most of these places are symmetrical at least (they’ll get R/W on an RO image for a short period of time and then drop back to RO), so you’ll see the wrong shared permission locks only for a short duration. But at least blockdev-snapshot and blockdev-snapshot-sync are one way; they drop R/W to RO and stay there. So if you do a blockdev-snapshot*, the shared permission bits will be flipped. This is therefore very much user-visible.

It’s not catastrophic, though, so I’m not sure whether we need it in 3.1. It’s definitely a bugfix, and I think we have patches queued for the next RC already, so I think it makes sense to include at least patches 2 and 3 as well.

v2:
- Patch 1: Reuse the existing error path instead of creating a new one

           [Berto]

git-backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch [down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/3:[0025] [FC] 'block: Always abort reopen after prepare succeeded' 002/3:[----] [--] 'file-posix: Fix shared locks on reopen commit' 003/3:[----] [--] 'iotests: Test file-posix locking and reopen'

Max Reitz (3):
block: Always abort reopen after prepare succeeded
file-posix: Fix shared locks on reopen commit
iotests: Test file-posix locking and reopen

 block.c                    | 12 +++++++
 block/file-posix.c         |  2 +-
 tests/qemu-iotests/182     | 71 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/182.out |  9 +++++
 4 files changed, 93 insertions(+), 1 deletion(-)

2.17.2

bdrv_reopen_multiple() does not invoke bdrv_reopen_abort() for the element of the reopen queue for which bdrv_reopen_prepare() failed, because it assumes that the prepare function will have rolled back all changes already.

However, bdrv_reopen_prepare() does not do this in every case: It may notice an error after BlockDriver.bdrv_reopen_prepare() succeeded, and it will not invoke BlockDriver.bdrv_reopen_abort() then; and neither will bdrv_reopen_multiple(), as explained above.

This is wrong because we must always call .bdrv_reopen_commit() or .bdrv_reopen_abort() after .bdrv_reopen_prepare() has succeeded. Otherwise, the block driver has no chance to undo what it has done in its implementation of .bdrv_reopen_prepare().

To fix this, bdrv_reopen_prepare() has to call .bdrv_reopen_abort() if it wants to return an error after .bdrv_reopen_prepare() has succeeded.

---
 block.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

index fd67e14dfa..3feac08535 100644
@@ -3201,6 +3201,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
     QDict *orig_reopen_opts;
     char *discard = NULL;
     bool read_only;
+    bool drv_prepared = false;
 
     assert(reopen_state != NULL);
     assert(reopen_state->bs->drv != NULL);

@@ -3285,6 +3286,8 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
         goto error;
     }
 
+    drv_prepared = true;
+
     /* Options that are not handled are only okay if they are unchanged
      * compared to the old state. It is expected that some options are only
      * used for the initial open, but not reopen (e.g. filename) */

@@ -3350,6 +3353,15 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
     reopen_state->options = qobject_ref(orig_reopen_opts);
 
 error:
+    if (ret < 0 && drv_prepared) {
+        /* drv->bdrv_reopen_prepare() has succeeded, so we need to
+         * call drv->bdrv_reopen_abort() before signaling an error
+         * (bdrv_reopen_multiple() will not call bdrv_reopen_abort()
+         * when the respective bdrv_reopen_prepare() has failed) */
+        if (drv->bdrv_reopen_abort) {
+            drv->bdrv_reopen_abort(reopen_state);
+        }
+    }
     qemu_opts_del(opts);
     qobject_unref(orig_reopen_opts);
     g_free(discard);

2.17.2

Berto

s->locked_shared_perm is the set of bits locked in the file, which is the inverse of the permissions actually shared. So we need to pass them as they are to raw_apply_lock_bytes() instead of inverting them again.

---
 block/file-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index df3a8d7cdf..8460d003f0 100644
@@ -963,7 +963,7 @@ static void raw_reopen_commit(BDRVReopenState *state)
 
     /* Copy locks to the new fd before closing the old one. */
     raw_apply_lock_bytes(NULL, rs->fd, s->locked_perm,
-                         ~s->locked_shared_perm, false, &local_err);
+                         s->locked_shared_perm, false, &local_err);
     if (local_err) {
         /* shouldn't fail in a sane host, but report it just in case. */
         error_report_err(local_err);

2.17.2

Berto

---
 tests/qemu-iotests/182     | 71 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/182.out |  9 +++++
 2 files changed, 80 insertions(+)

index 4b31592fb8..3b7689c1d5 100755
@@ -31,6 +31,7 @@ status=1	# failure is the default!
 _cleanup()
 {
     _cleanup_test_img
+    rm -f "$TEST_IMG.overlay"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 

@@ -71,6 +72,76 @@ echo 'quit' | $QEMU -nographic -monitor stdio \
 
 _cleanup_qemu
 
+echo
+echo '=== Testing reopen ==='
+echo
+
+# This tests that reopening does not unshare any permissions it should
+# not unshare
+# (There was a bug where reopening shared exactly the opposite of the
+# permissions it was supposed to share)
+
+_launch_qemu
+
+_send_qemu_cmd $QEMU_HANDLE \
+    "{'execute': 'qmp_capabilities'}" \
+    'return'
+
+# Open the image without any format layer (we are not going to access
+# it, so that is fine)
+# This should keep all permissions shared.
+success_or_failure=y _send_qemu_cmd $QEMU_HANDLE \
+    "{'execute': 'blockdev-add',
+      'arguments': {
+          'node-name': 'node0',
+          'driver': 'file',
+          'filename': '$TEST_IMG',
+          'locking': 'on'
+          } }" \
+    'return' \
+    'error'
+
+# This snapshot will perform a reopen to drop R/W to RO.
+# It should still keep all permissions shared.
+success_or_failure=y _send_qemu_cmd $QEMU_HANDLE \
+    "{'execute': 'blockdev-snapshot-sync',
+      'arguments': {
+          'node-name': 'node0',
+          'snapshot-file': '$TEST_IMG.overlay',
+          'snapshot-node-name': 'node1'
+      } }" \
+    'return' \
+    'error'
+
+# Now open the same file again
+# This does not require any permissions (and does not unshare any), so
+# this will not conflict with node0.
+success_or_failure=y _send_qemu_cmd $QEMU_HANDLE \
+    "{'execute': 'blockdev-add',
+      'arguments': {
+          'node-name': 'node1',
+          'driver': 'file',
+          'filename': '$TEST_IMG',
+          'locking': 'on'
+          } }" \
+    'return' \
+    'error'
+
+# Now we attach the image to a virtio-blk device.  This device does
+# require some permissions (at least WRITE and READ_CONSISTENT), so if
+# reopening node0 unshared any (which it should not have), this will
+# fail (but it should not).
+success_or_failure=y _send_qemu_cmd $QEMU_HANDLE \
+    "{'execute': 'device_add',
+      'arguments': {
+          'driver': 'virtio-blk',
+          'drive': 'node1'
+      } }" \
+    'return' \
+    'error'
+
+_cleanup_qemu
+
 # success, all done
 echo "*** done"
 rm -f $seq.full

index f1463c8862..af501ca3f3 100644
@@ -5,4 +5,13 @@ Starting QEMU
 Starting a second QEMU using the same image should fail
 QEMU_PROG: -drive file=TEST_DIR/t.qcow2,if=none,id=drive0,file.locking=on: Failed to get "write" lock
 Is another process using the image [TEST_DIR/t.qcow2]?
+
+=== Testing reopen ===
+
+{"return": {}}
+{"return": {}}
+Formatting 'TEST_DIR/t.qcow2.overlay', fmt=qcow2 size=197120 backing_file=TEST_DIR/t.qcow2 backing_fmt=file cluster_size=65536 lazy_refcounts=off refcount_bits=16
+{"return": {}}
+{"return": {}}
+{"return": {}}
 *** done

2.17.2

Berto

Am 16.11.2018 um 17:45 hat Max Reitz geschrieben:

Thanks, applied to the block branch.

Kevin