[Qemu-devel] [PATCH v2 00/10] file-posix: Make auto-read-only dynamic

[Qemu-devel] [PATCH v2 00/10] file-posix: Make auto-read-only dynamic

[Kevin Wolf]

We introduced the auto-read-only option to fix the problem that block
jobs that reopen a backing file read-write don't work any more when all
nodes are created individually with -blockdev. The reason is that
bs->file of these backing files doesn't inherit the read-only option
from the format layer node any more if it's created separately.

The way auto-read-only was designed to fix this is that it just always
opens the file node read-write if it can, so reopening the format layer
node is enough to make the backing file writable when necessary.

This works in principle, but not when libvirt uses sVirt: Then QEMU
doesn't even have the permissions to open the image file read-write
until libvirt performs an operation where write access is needed.

This series changes auto-read-only so that it works dynamically and
automatically reopens the file read-only or read-write depending on the
permissions that users attached to the node requested.

See also: https://bugzilla.redhat.com/show_bug.cgi?id=1685989

v2:
- Added test for and fixed snapshot=on,read-only=on regression [Berto]
- Added test for and fixed commit regression [Peter]

Kevin Wolf (10):
  tests/virtio-blk-test: Disable auto-read-only
  qemu-iotests: commit to backing file with auto-read-only
  block: Avoid useless local_err
  block: Make permission changes in reopen less wrong
  file-posix: Fix bdrv_open_flags() for snapshot=on
  file-posix: Factor out raw_reconfigure_getfd()
  file-posix: Store BDRVRawState.reopen_state during reopen
  file-posix: Lock new fd in raw_reopen_prepare()
  file-posix: Prepare permission code for fd switching
  file-posix: Make auto-read-only dynamic

 block.c                       |  46 ++++---
 block/file-posix.c            | 248 ++++++++++++++++++++++++----------
 tests/virtio-blk-test.c       |   2 +-
 tests/qemu-iotests/051        |   7 +
 tests/qemu-iotests/051.out    |   9 ++
 tests/qemu-iotests/051.pc.out |   9 ++
 tests/qemu-iotests/232        |  31 +++++
 tests/qemu-iotests/232.out    |  32 ++++-
 8 files changed, 284 insertions(+), 100 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v2 01/10] tests/virtio-blk-test: Disable auto-read-only

[Kevin Wolf]

tests/virtio-blk-test uses a temporary image file that it deletes while
QEMU is still running, so it can't be reopened when writers are
attached or detached. Disable auto-read-only to keep it always writable.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/virtio-blk-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index b02be0274e..b65365934b 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -751,7 +751,7 @@ static void *virtio_blk_test_setup(GString *cmd_line, void *arg)
     char *tmp_path = drive_create();
 
     g_string_append_printf(cmd_line,
-                           " -drive if=none,id=drive0,file=%s,format=raw "
+                           " -drive if=none,id=drive0,file=%s,format=raw,auto-read-only=off "
                            "-drive if=none,id=drive1,file=null-co://,format=raw ",
                            tmp_path);
 
-- 
2.20.1

[Qemu-devel] [PATCH v2 02/10] qemu-iotests: commit to backing file with auto-read-only

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/232     | 31 +++++++++++++++++++++++++++++++
 tests/qemu-iotests/232.out | 20 ++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/tests/qemu-iotests/232 b/tests/qemu-iotests/232
index 71fd48eff0..0de097fc88 100755
--- a/tests/qemu-iotests/232
+++ b/tests/qemu-iotests/232
@@ -29,6 +29,7 @@ status=1	# failure is the default!
 _cleanup()
 {
     _cleanup_test_img
+    rm -f $TEST_IMG.[01234]
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
@@ -143,6 +144,36 @@ run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,a
 run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0,auto-read-only=on
 run_qemu_info_block -blockdev driver=file,filename="$TEST_IMG",node-name=node0
 
+echo
+echo "=== Try commit to backing file with auto-read-only ==="
+echo
+
+TEST_IMG="$TEST_IMG.0" _make_test_img $size
+TEST_IMG="$TEST_IMG.1" _make_test_img $size
+TEST_IMG="$TEST_IMG.2" _make_test_img $size
+TEST_IMG="$TEST_IMG.3" _make_test_img $size
+TEST_IMG="$TEST_IMG.4" _make_test_img $size
+
+(cat <<EOF
+{"execute":"qmp_capabilities"}
+{"execute":"block-commit",
+ "arguments":{"device":"format-4", "top-node": "format-2", "base-node":"format-0", "job-id":"job0"}}
+EOF
+sleep 1
+echo '{"execute":"quit"}'
+) | $QEMU -qmp stdio -nographic -nodefaults \
+    -blockdev file,node-name=file-0,filename=$TEST_IMG.0,auto-read-only=on \
+    -blockdev qcow2,node-name=format-0,file=file-0,read-only=on \
+    -blockdev file,node-name=file-1,filename=$TEST_IMG.1,auto-read-only=on \
+    -blockdev qcow2,node-name=format-1,file=file-1,read-only=on,backing=format-0 \
+    -blockdev file,node-name=file-2,filename=$TEST_IMG.2,auto-read-only=on \
+    -blockdev qcow2,node-name=format-2,file=file-2,read-only=on,backing=format-1 \
+    -blockdev file,node-name=file-3,filename=$TEST_IMG.3,auto-read-only=on \
+    -blockdev qcow2,node-name=format-3,file=file-3,read-only=on,backing=format-2 \
+    -blockdev file,node-name=file-4,filename=$TEST_IMG.4,auto-read-only=on \
+    -blockdev qcow2,node-name=format-4,file=file-4,read-only=on,backing=format-3 |
+    _filter_qmp
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/232.out b/tests/qemu-iotests/232.out
index dcb683afa3..5036ef13d4 100644
--- a/tests/qemu-iotests/232.out
+++ b/tests/qemu-iotests/232.out
@@ -56,4 +56,24 @@ QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0,read
 QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0,auto-read-only=off: Could not open 'TEST_DIR/t.IMGFMT': Permission denied
 node0: TEST_DIR/t.IMGFMT (file, read-only)
 QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0: Could not open 'TEST_DIR/t.IMGFMT': Permission denied
+
+=== Try commit to backing file with auto-read-only ===
+
+Formatting 'TEST_DIR/t.IMGFMT.0', fmt=IMGFMT size=134217728
+Formatting 'TEST_DIR/t.IMGFMT.1', fmt=IMGFMT size=134217728
+Formatting 'TEST_DIR/t.IMGFMT.2', fmt=IMGFMT size=134217728
+Formatting 'TEST_DIR/t.IMGFMT.3', fmt=IMGFMT size=134217728
+Formatting 'TEST_DIR/t.IMGFMT.4', fmt=IMGFMT size=134217728
+QMP_VERSION
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "job0"}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "job0"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "job0"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "job0", "len": 134217728, "offset": 134217728, "speed": 0, "type": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "job0"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "job0"}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
 *** done
-- 
2.20.1

[Qemu-devel] [PATCH v2 03/10] block: Avoid useless local_err

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/block.c b/block.c
index ccf008c177..e18bd5eefd 100644
--- a/block.c
+++ b/block.c
@@ -3155,14 +3155,12 @@ int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error **er
 {
     int ret = -1;
     BlockReopenQueueEntry *bs_entry, *next;
-    Error *local_err = NULL;
 
     assert(bs_queue != NULL);
 
     QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
         assert(bs_entry->state.bs->quiesce_counter > 0);
-        if (bdrv_reopen_prepare(&bs_entry->state, bs_queue, &local_err)) {
-            error_propagate(errp, local_err);
+        if (bdrv_reopen_prepare(&bs_entry->state, bs_queue, errp)) {
             goto cleanup;
         }
         bs_entry->prepared = true;
-- 
2.20.1

[Qemu-devel] [PATCH v2 04/10] block: Make permission changes in reopen less wrong

[Kevin Wolf]

The way that reopen interacts with permission changes has one big
problem: Both operations are recursive, and the permissions are changes
for each node in the reopen queue.

For a simple graph that consists just of parent and child,
.bdrv_check_perm will be called twice for the child, once recursively
when adjusting the permissions of parent, and once again when the child
itself is reopened.

Even worse, the first .bdrv_check_perm call happens before
.bdrv_reopen_prepare was called for the child and the second one is
called afterwards.

Making sure that .bdrv_check_perm (and the other permission callbacks)
are called only once is hard. We can cope with multiple calls right now,
but as soon as file-posix gets a dynamic auto-read-only that may need to
open a new file descriptor, we get the additional requirement that all
of them are after the .bdrv_reopen_prepare call.

So reorder things in bdrv_reopen_multiple() to first call
.bdrv_reopen_prepare for all involved nodes and only then adjust
permissions.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index e18bd5eefd..9b9d25e843 100644
--- a/block.c
+++ b/block.c
@@ -1698,6 +1698,7 @@ static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared);
 
 typedef struct BlockReopenQueueEntry {
      bool prepared;
+     bool perms_checked;
      BDRVReopenState state;
      QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
 } BlockReopenQueueEntry;
@@ -3166,6 +3167,16 @@ int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error **er
         bs_entry->prepared = true;
     }
 
+    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+        BDRVReopenState *state = &bs_entry->state;
+        ret = bdrv_check_perm(state->bs, bs_queue, state->perm,
+                              state->shared_perm, NULL, errp);
+        if (ret < 0) {
+            goto cleanup_perm;
+        }
+        bs_entry->perms_checked = true;
+    }
+
     /* If we reach this point, we have success and just need to apply the
      * changes
      */
@@ -3174,7 +3185,20 @@ int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error **er
     }
 
     ret = 0;
+cleanup_perm:
+    QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
+        BDRVReopenState *state = &bs_entry->state;
+
+        if (!bs_entry->perms_checked) {
+            continue;
+        }
 
+        if (ret == 0) {
+            bdrv_set_perm(state->bs, state->perm, state->shared_perm);
+        } else {
+            bdrv_abort_perm_update(state->bs);
+        }
+    }
 cleanup:
     QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
         if (ret) {
@@ -3428,12 +3452,6 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
         } while ((entry = qdict_next(reopen_state->options, entry)));
     }
 
-    ret = bdrv_check_perm(reopen_state->bs, queue, reopen_state->perm,
-                          reopen_state->shared_perm, NULL, errp);
-    if (ret < 0) {
-        goto error;
-    }
-
     ret = 0;
 
     /* Restore the original reopen_state->options QDict */
@@ -3500,9 +3518,6 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
 
     bdrv_refresh_limits(bs, NULL);
 
-    bdrv_set_perm(reopen_state->bs, reopen_state->perm,
-                  reopen_state->shared_perm);
-
     new_can_write =
         !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
     if (!old_can_write && new_can_write && drv->bdrv_reopen_bitmaps_rw) {
@@ -3534,8 +3549,6 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
     if (drv->bdrv_reopen_abort) {
         drv->bdrv_reopen_abort(reopen_state);
     }
-
-    bdrv_abort_perm_update(reopen_state->bs);
 }
 
 
-- 
2.20.1

[Qemu-devel] [PATCH v2 05/10] file-posix: Fix bdrv_open_flags() for snapshot=on

[Kevin Wolf]

Using a different read-only setting for bs->open_flags than for the
flags to the driver's open function is just inconsistent and a bad idea.
After this patch, the temporary snapshot keeps being opened read-only if
read-only=on,snapshot=on is passed.

If we wanted to change this behaviour to make only the orginal image
file read-only, but the temporary overlay read-write (as the comment in
the removed code suggests), that change would have to be made in
bdrv_temp_snapshot_options() (where the comment suggests otherwise).

Addressing this inconsistency before introducing dynamic auto-read-only
is important because otherwise we would immediately try to reopen the
temporary overlay even though the file is already unlinked.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                       | 7 -------
 tests/qemu-iotests/051        | 7 +++++++
 tests/qemu-iotests/051.out    | 9 +++++++++
 tests/qemu-iotests/051.pc.out | 9 +++++++++
 4 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index 9b9d25e843..33804cdcaa 100644
--- a/block.c
+++ b/block.c
@@ -1163,13 +1163,6 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
      */
     open_flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_PROTOCOL);
 
-    /*
-     * Snapshots should be writable.
-     */
-    if (flags & BDRV_O_TEMPORARY) {
-        open_flags |= BDRV_O_RDWR;
-    }
-
     return open_flags;
 }
 
diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index 3b50c7f188..151b850a8b 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -356,6 +356,13 @@ $QEMU_IO -c "read -P 0x33 0 4k" "$TEST_IMG" | _filter_qemu_io
 # Using snapshot=on with a non-existent TMPDIR
 TMPDIR=/nonexistent run_qemu -drive driver=null-co,snapshot=on
 
+# Using snapshot=on together with read-only=on
+echo "info block" |
+    run_qemu -drive file="$TEST_IMG",snapshot=on,read-only=on,if=none,id=$device_id |
+    _filter_qemu_io |
+    sed -e 's#"/[^"]*/vl\.[A-Za-z]\{6\}"#SNAPSHOT_PATH#g'
+
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index b900935fbc..9f1cf22608 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -458,4 +458,13 @@ read 4096/4096 bytes at offset 0
 Testing: -drive driver=null-co,snapshot=on
 QEMU_PROG: -drive driver=null-co,snapshot=on: Could not get temporary filename: No such file or directory
 
+Testing: -drive file=TEST_DIR/t.qcow2,snapshot=on,read-only=on,if=none,id=drive0
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) info block
+drive0 (NODE_NAME): json:{"backing": {"driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2"}}, "driver": "qcow2", "file": {"driver": "file", "filename": SNAPSHOT_PATH}} (qcow2, read-only)
+    Removable device: not locked, tray closed
+    Cache mode:       writeback, ignore flushes
+    Backing file:     TEST_DIR/t.qcow2 (chain depth: 1)
+(qemu) quit
+
 *** done
diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out
index 8c5c735dfd..c4743cc31c 100644
--- a/tests/qemu-iotests/051.pc.out
+++ b/tests/qemu-iotests/051.pc.out
@@ -530,4 +530,13 @@ read 4096/4096 bytes at offset 0
 Testing: -drive driver=null-co,snapshot=on
 QEMU_PROG: -drive driver=null-co,snapshot=on: Could not get temporary filename: No such file or directory
 
+Testing: -drive file=TEST_DIR/t.qcow2,snapshot=on,read-only=on,if=none,id=drive0
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) info block
+drive0 (NODE_NAME): json:{"backing": {"driver": "qcow2", "file": {"driver": "file", "filename": "TEST_DIR/t.qcow2"}}, "driver": "qcow2", "file": {"driver": "file", "filename": SNAPSHOT_PATH}} (qcow2, read-only)
+    Removable device: not locked, tray closed
+    Cache mode:       writeback, ignore flushes
+    Backing file:     TEST_DIR/t.qcow2 (chain depth: 1)
+(qemu) quit
+
 *** done
-- 
2.20.1

[Qemu-devel] [PATCH v2 06/10] file-posix: Factor out raw_reconfigure_getfd()

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 107 ++++++++++++++++++++++++++-------------------
 1 file changed, 62 insertions(+), 45 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index ba6ab62a38..ae57ba1fc6 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -842,6 +842,62 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
     return ret;
 }
 
+static int raw_reconfigure_getfd(BlockDriverState *bs, int flags,
+                                 int *open_flags, Error **errp)
+{
+    BDRVRawState *s = bs->opaque;
+    int fd = -1;
+    int ret;
+    int fcntl_flags = O_APPEND | O_NONBLOCK;
+#ifdef O_NOATIME
+    fcntl_flags |= O_NOATIME;
+#endif
+
+    *open_flags = 0;
+    if (s->type == FTYPE_CD) {
+        *open_flags |= O_NONBLOCK;
+    }
+
+    raw_parse_flags(flags, open_flags);
+
+#ifdef O_ASYNC
+    /* Not all operating systems have O_ASYNC, and those that don't
+     * will not let us track the state into rs->open_flags (typically
+     * you achieve the same effect with an ioctl, for example I_SETSIG
+     * on Solaris). But we do not use O_ASYNC, so that's fine.
+     */
+    assert((s->open_flags & O_ASYNC) == 0);
+#endif
+
+    if ((*open_flags & ~fcntl_flags) == (s->open_flags & ~fcntl_flags)) {
+        /* dup the original fd */
+        fd = qemu_dup(s->fd);
+        if (fd >= 0) {
+            ret = fcntl_setfl(fd, *open_flags);
+            if (ret) {
+                qemu_close(fd);
+                fd = -1;
+            }
+        }
+    }
+
+    /* If we cannot use fcntl, or fcntl failed, fall back to qemu_open() */
+    if (fd == -1) {
+        const char *normalized_filename = bs->filename;
+        ret = raw_normalize_devicepath(&normalized_filename, errp);
+        if (ret >= 0) {
+            assert(!(*open_flags & O_CREAT));
+            fd = qemu_open(normalized_filename, *open_flags);
+            if (fd == -1) {
+                error_setg_errno(errp, errno, "Could not reopen file");
+                return -1;
+            }
+        }
+    }
+
+    return fd;
+}
+
 static int raw_reopen_prepare(BDRVReopenState *state,
                               BlockReopenQueue *queue, Error **errp)
 {
@@ -858,7 +914,6 @@ static int raw_reopen_prepare(BDRVReopenState *state,
 
     state->opaque = g_new0(BDRVRawReopenState, 1);
     rs = state->opaque;
-    rs->fd = -1;
 
     /* Handle options changes */
     opts = qemu_opts_create(&raw_runtime_opts, NULL, 0, &error_abort);
@@ -877,50 +932,12 @@ static int raw_reopen_prepare(BDRVReopenState *state,
      * bdrv_reopen_prepare() will detect changes and complain. */
     qemu_opts_to_qdict(opts, state->options);
 
-    if (s->type == FTYPE_CD) {
-        rs->open_flags |= O_NONBLOCK;
-    }
-
-    raw_parse_flags(state->flags, &rs->open_flags);
-
-    int fcntl_flags = O_APPEND | O_NONBLOCK;
-#ifdef O_NOATIME
-    fcntl_flags |= O_NOATIME;
-#endif
-
-#ifdef O_ASYNC
-    /* Not all operating systems have O_ASYNC, and those that don't
-     * will not let us track the state into rs->open_flags (typically
-     * you achieve the same effect with an ioctl, for example I_SETSIG
-     * on Solaris). But we do not use O_ASYNC, so that's fine.
-     */
-    assert((s->open_flags & O_ASYNC) == 0);
-#endif
-
-    if ((rs->open_flags & ~fcntl_flags) == (s->open_flags & ~fcntl_flags)) {
-        /* dup the original fd */
-        rs->fd = qemu_dup(s->fd);
-        if (rs->fd >= 0) {
-            ret = fcntl_setfl(rs->fd, rs->open_flags);
-            if (ret) {
-                qemu_close(rs->fd);
-                rs->fd = -1;
-            }
-        }
-    }
-
-    /* If we cannot use fcntl, or fcntl failed, fall back to qemu_open() */
-    if (rs->fd == -1) {
-        const char *normalized_filename = state->bs->filename;
-        ret = raw_normalize_devicepath(&normalized_filename, errp);
-        if (ret >= 0) {
-            assert(!(rs->open_flags & O_CREAT));
-            rs->fd = qemu_open(normalized_filename, rs->open_flags);
-            if (rs->fd == -1) {
-                error_setg_errno(errp, errno, "Could not reopen file");
-                ret = -1;
-            }
-        }
+    rs->fd = raw_reconfigure_getfd(state->bs, state->flags, &rs->open_flags,
+                                   &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -1;
+        goto out;
     }
 
     /* Fail already reopen_prepare() if we can't get a working O_DIRECT
-- 
2.20.1

[Qemu-devel] [PATCH v2 07/10] file-posix: Store BDRVRawState.reopen_state during reopen

[Kevin Wolf]

We'll want to access the file descriptor in the reopen_state while
processing permission changes in the context of the repoen.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index ae57ba1fc6..6aaee1df16 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -144,6 +144,8 @@ typedef struct BDRVRawState {
     uint64_t locked_perm;
     uint64_t locked_shared_perm;
 
+    BDRVReopenState *reopen_state;
+
 #ifdef CONFIG_XFS
     bool is_xfs:1;
 #endif
@@ -952,6 +954,7 @@ static int raw_reopen_prepare(BDRVReopenState *state,
         }
     }
 
+    s->reopen_state = state;
 out:
     qemu_opts_del(opts);
     return ret;
@@ -978,12 +981,16 @@ static void raw_reopen_commit(BDRVReopenState *state)
 
     g_free(state->opaque);
     state->opaque = NULL;
+
+    assert(s->reopen_state == state);
+    s->reopen_state = NULL;
 }
 
 
 static void raw_reopen_abort(BDRVReopenState *state)
 {
     BDRVRawReopenState *rs = state->opaque;
+    BDRVRawState *s = state->bs->opaque;
 
      /* nothing to do if NULL, we didn't get far enough */
     if (rs == NULL) {
@@ -996,6 +1003,9 @@ static void raw_reopen_abort(BDRVReopenState *state)
     }
     g_free(state->opaque);
     state->opaque = NULL;
+
+    assert(s->reopen_state == state);
+    s->reopen_state = NULL;
 }
 
 static int hdev_get_max_transfer_length(BlockDriverState *bs, int fd)
-- 
2.20.1

[Qemu-devel] [PATCH v2 08/10] file-posix: Lock new fd in raw_reopen_prepare()

[Kevin Wolf]

There is no reason why we can take locks on the new file descriptor only
in raw_reopen_commit() where error handling isn't possible any more.
Instead, we can already do this in raw_reopen_prepare().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 6aaee1df16..932cc8e58c 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -906,7 +906,7 @@ static int raw_reopen_prepare(BDRVReopenState *state,
     BDRVRawState *s;
     BDRVRawReopenState *rs;
     QemuOpts *opts;
-    int ret = 0;
+    int ret;
     Error *local_err = NULL;
 
     assert(state != NULL);
@@ -947,14 +947,27 @@ static int raw_reopen_prepare(BDRVReopenState *state,
     if (rs->fd != -1) {
         raw_probe_alignment(state->bs, rs->fd, &local_err);
         if (local_err) {
-            qemu_close(rs->fd);
-            rs->fd = -1;
             error_propagate(errp, local_err);
             ret = -EINVAL;
+            goto out_fd;
+        }
+
+        /* Copy locks to the new fd */
+        ret = raw_apply_lock_bytes(NULL, rs->fd, s->locked_perm,
+                                   s->locked_shared_perm, false, errp);
+        if (ret < 0) {
+            ret = -EINVAL;
+            goto out_fd;
         }
     }
 
     s->reopen_state = state;
+    ret = 0;
+out_fd:
+    if (ret < 0) {
+        qemu_close(rs->fd);
+        rs->fd = -1;
+    }
 out:
     qemu_opts_del(opts);
     return ret;
@@ -964,18 +977,10 @@ static void raw_reopen_commit(BDRVReopenState *state)
 {
     BDRVRawReopenState *rs = state->opaque;
     BDRVRawState *s = state->bs->opaque;
-    Error *local_err = NULL;
 
     s->check_cache_dropped = rs->check_cache_dropped;
     s->open_flags = rs->open_flags;
 
-    /* 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);
-    if (local_err) {
-        /* shouldn't fail in a sane host, but report it just in case. */
-        error_report_err(local_err);
-    }
     qemu_close(s->fd);
     s->fd = rs->fd;
 
-- 
2.20.1

[Qemu-devel] [PATCH v2 09/10] file-posix: Prepare permission code for fd switching

[Kevin Wolf]

In order to be able to dynamically reopen the file read-only or
read-write, depending on the users that are attached, we need to be able
to switch to a different file descriptor during the permission change.

This interacts with reopen, which also creates a new file descriptor and
performs permission changes internally. In this case, the permission
change code must reuse the reopen file descriptor instead of creating a
third one.

In turn, reopen can drop its code to copy file locks to the new file
descriptor because that is now done when applying the new permissions.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c | 96 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 85 insertions(+), 11 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 932cc8e58c..e41e0779c6 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -144,6 +144,7 @@ typedef struct BDRVRawState {
     uint64_t locked_perm;
     uint64_t locked_shared_perm;
 
+    int perm_change_fd;
     BDRVReopenState *reopen_state;
 
 #ifdef CONFIG_XFS
@@ -845,7 +846,8 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
 }
 
 static int raw_reconfigure_getfd(BlockDriverState *bs, int flags,
-                                 int *open_flags, Error **errp)
+                                 int *open_flags, bool force_dup,
+                                 Error **errp)
 {
     BDRVRawState *s = bs->opaque;
     int fd = -1;
@@ -871,6 +873,11 @@ static int raw_reconfigure_getfd(BlockDriverState *bs, int flags,
     assert((s->open_flags & O_ASYNC) == 0);
 #endif
 
+    if (!force_dup && *open_flags == s->open_flags) {
+        /* We're lucky, the existing fd is fine */
+        return s->fd;
+    }
+
     if ((*open_flags & ~fcntl_flags) == (s->open_flags & ~fcntl_flags)) {
         /* dup the original fd */
         fd = qemu_dup(s->fd);
@@ -935,7 +942,7 @@ static int raw_reopen_prepare(BDRVReopenState *state,
     qemu_opts_to_qdict(opts, state->options);
 
     rs->fd = raw_reconfigure_getfd(state->bs, state->flags, &rs->open_flags,
-                                   &local_err);
+                                   true, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         ret = -1;
@@ -951,14 +958,6 @@ static int raw_reopen_prepare(BDRVReopenState *state,
             ret = -EINVAL;
             goto out_fd;
         }
-
-        /* Copy locks to the new fd */
-        ret = raw_apply_lock_bytes(NULL, rs->fd, s->locked_perm,
-                                   s->locked_shared_perm, false, errp);
-        if (ret < 0) {
-            ret = -EINVAL;
-            goto out_fd;
-        }
     }
 
     s->reopen_state = state;
@@ -2696,12 +2695,78 @@ static QemuOptsList raw_create_opts = {
 static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared,
                           Error **errp)
 {
-    return raw_handle_perm_lock(bs, RAW_PL_PREPARE, perm, shared, errp);
+    BDRVRawState *s = bs->opaque;
+    BDRVRawReopenState *rs = NULL;
+    int open_flags;
+    int ret;
+
+    if (s->perm_change_fd) {
+        /*
+         * In the context of reopen, this function may be called several times
+         * (directly and recursively while change permissions of the parent).
+         * This is even true for children that don't inherit from the original
+         * reopen node, so s->reopen_state is not set.
+         *
+         * Ignore all but the first call.
+         */
+        return 0;
+    }
+
+    if (s->reopen_state) {
+        /* We already have a new file descriptor to set permissions for */
+        assert(s->reopen_state->perm == perm);
+        assert(s->reopen_state->shared_perm == shared);
+        rs = s->reopen_state->opaque;
+        s->perm_change_fd = rs->fd;
+    } else {
+        /* We may need a new fd if auto-read-only switches the mode */
+        ret = raw_reconfigure_getfd(bs, bs->open_flags, &open_flags,
+                                    false, errp);
+        if (ret < 0) {
+            return ret;
+        } else if (ret != s->fd) {
+            s->perm_change_fd = ret;
+        }
+    }
+
+    /* Prepare permissions on old fd to avoid conflicts between old and new,
+     * but keep everything locked that new will need. */
+    ret = raw_handle_perm_lock(bs, RAW_PL_PREPARE, perm, shared, errp);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    /* Copy locks to the new fd */
+    if (s->perm_change_fd) {
+        ret = raw_apply_lock_bytes(NULL, s->perm_change_fd, perm, ~shared,
+                                   false, errp);
+        if (ret < 0) {
+            raw_handle_perm_lock(bs, RAW_PL_ABORT, 0, 0, NULL);
+            goto fail;
+        }
+    }
+    return 0;
+
+fail:
+    if (s->perm_change_fd && !s->reopen_state) {
+        qemu_close(s->perm_change_fd);
+    }
+    s->perm_change_fd = 0;
+    return ret;
 }
 
 static void raw_set_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared)
 {
     BDRVRawState *s = bs->opaque;
+
+    /* For reopen, we have already switched to the new fd (.bdrv_set_perm is
+     * called after .bdrv_reopen_commit) */
+    if (s->perm_change_fd && s->fd != s->perm_change_fd) {
+        qemu_close(s->fd);
+        s->fd = s->perm_change_fd;
+    }
+    s->perm_change_fd = 0;
+
     raw_handle_perm_lock(bs, RAW_PL_COMMIT, perm, shared, NULL);
     s->perm = perm;
     s->shared_perm = shared;
@@ -2709,6 +2774,15 @@ static void raw_set_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared)
 
 static void raw_abort_perm_update(BlockDriverState *bs)
 {
+    BDRVRawState *s = bs->opaque;
+
+    /* For reopen, .bdrv_reopen_abort is called afterwards and will close
+     * the file descriptor. */
+    if (s->perm_change_fd && !s->reopen_state) {
+        qemu_close(s->perm_change_fd);
+    }
+    s->perm_change_fd = 0;
+
     raw_handle_perm_lock(bs, RAW_PL_ABORT, 0, 0, NULL);
 }
 
-- 
2.20.1

[Qemu-devel] [PATCH v2 10/10] file-posix: Make auto-read-only dynamic

[Kevin Wolf]

Until now, with auto-read-only=on we tried to open the file read-write
first and if that failed, read-only was tried. This is actually not good
enough for libvirt, which gives QEMU SELinux permissions for read-write
only as soon as it actually intends to write to the image. So we need to
be able to switch between read-only and read-write at runtime.

This patch makes auto-read-only dynamic, i.e. the file is opened
read-only as long as no user of the node has requested write
permissions, but it is automatically reopened read-write as soon as the
first writer is attached. Conversely, if the last writer goes away, the
file is reopened read-only again.

bs->read_only is no longer set for auto-read-only=on files even if the
file descriptor is opened read-only because it will be transparently
upgraded as soon as a writer is attached. This changes the output of
qemu-iotests 232.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/file-posix.c         | 36 +++++++++++++++++-------------------
 tests/qemu-iotests/232.out | 12 ++++++------
 2 files changed, 23 insertions(+), 25 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index e41e0779c6..d41059d139 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -376,13 +376,21 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
     }
 }
 
-static void raw_parse_flags(int bdrv_flags, int *open_flags)
+static void raw_parse_flags(int bdrv_flags, int *open_flags, bool has_writers)
 {
+    bool read_write = false;
     assert(open_flags != NULL);
 
     *open_flags |= O_BINARY;
     *open_flags &= ~O_ACCMODE;
-    if (bdrv_flags & BDRV_O_RDWR) {
+
+    if (bdrv_flags & BDRV_O_AUTO_RDONLY) {
+        read_write = has_writers;
+    } else if (bdrv_flags & BDRV_O_RDWR) {
+        read_write = true;
+    }
+
+    if (read_write) {
         *open_flags |= O_RDWR;
     } else {
         *open_flags |= O_RDONLY;
@@ -518,24 +526,12 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
                                                false);
 
     s->open_flags = open_flags;
-    raw_parse_flags(bdrv_flags, &s->open_flags);
+    raw_parse_flags(bdrv_flags, &s->open_flags, false);
 
     s->fd = -1;
     fd = qemu_open(filename, s->open_flags, 0644);
     ret = fd < 0 ? -errno : 0;
 
-    if (ret == -EACCES || ret == -EROFS) {
-        /* Try to degrade to read-only, but if it doesn't work, still use the
-         * normal error message. */
-        if (bdrv_apply_auto_read_only(bs, NULL, NULL) == 0) {
-            bdrv_flags &= ~BDRV_O_RDWR;
-            raw_parse_flags(bdrv_flags, &s->open_flags);
-            assert(!(s->open_flags & O_CREAT));
-            fd = qemu_open(filename, s->open_flags);
-            ret = fd < 0 ? -errno : 0;
-        }
-    }
-
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not open '%s'", filename);
         if (ret == -EROFS) {
@@ -846,12 +842,14 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
 }
 
 static int raw_reconfigure_getfd(BlockDriverState *bs, int flags,
-                                 int *open_flags, bool force_dup,
+                                 int *open_flags, uint64_t perm, bool force_dup,
                                  Error **errp)
 {
     BDRVRawState *s = bs->opaque;
     int fd = -1;
     int ret;
+    bool has_writers = perm &
+        (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED | BLK_PERM_RESIZE);
     int fcntl_flags = O_APPEND | O_NONBLOCK;
 #ifdef O_NOATIME
     fcntl_flags |= O_NOATIME;
@@ -862,7 +860,7 @@ static int raw_reconfigure_getfd(BlockDriverState *bs, int flags,
         *open_flags |= O_NONBLOCK;
     }
 
-    raw_parse_flags(flags, open_flags);
+    raw_parse_flags(flags, open_flags, has_writers);
 
 #ifdef O_ASYNC
     /* Not all operating systems have O_ASYNC, and those that don't
@@ -942,7 +940,7 @@ static int raw_reopen_prepare(BDRVReopenState *state,
     qemu_opts_to_qdict(opts, state->options);
 
     rs->fd = raw_reconfigure_getfd(state->bs, state->flags, &rs->open_flags,
-                                   true, &local_err);
+                                   state->perm, true, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         ret = -1;
@@ -2720,7 +2718,7 @@ static int raw_check_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared,
         s->perm_change_fd = rs->fd;
     } else {
         /* We may need a new fd if auto-read-only switches the mode */
-        ret = raw_reconfigure_getfd(bs, bs->open_flags, &open_flags,
+        ret = raw_reconfigure_getfd(bs, bs->open_flags, &open_flags, perm,
                                     false, errp);
         if (ret < 0) {
             return ret;
diff --git a/tests/qemu-iotests/232.out b/tests/qemu-iotests/232.out
index 5036ef13d4..5bcc44bb62 100644
--- a/tests/qemu-iotests/232.out
+++ b/tests/qemu-iotests/232.out
@@ -22,12 +22,12 @@ NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only)
 NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only)
 
 QEMU_PROG: -drive driver=file,file=TEST_DIR/t.IMGFMT,if=none,read-only=off,auto-read-only=off: Could not open 'TEST_DIR/t.IMGFMT': Permission denied
-NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only)
-NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only)
+NODE_NAME: TEST_DIR/t.IMGFMT (file)
+NODE_NAME: TEST_DIR/t.IMGFMT (file)
 
 QEMU_PROG: -drive driver=file,file=TEST_DIR/t.IMGFMT,if=none,auto-read-only=off: Could not open 'TEST_DIR/t.IMGFMT': Permission denied
-NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only)
-NODE_NAME: TEST_DIR/t.IMGFMT (file, read-only)
+NODE_NAME: TEST_DIR/t.IMGFMT (file)
+NODE_NAME: TEST_DIR/t.IMGFMT (file)
 
 === -blockdev with read-write image: read-only/auto-read-only combinations ===
 
@@ -50,11 +50,11 @@ node0: TEST_DIR/t.IMGFMT (file, read-only)
 node0: TEST_DIR/t.IMGFMT (file, read-only)
 
 QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0,read-only=off,auto-read-only=off: Could not open 'TEST_DIR/t.IMGFMT': Permission denied
-node0: TEST_DIR/t.IMGFMT (file, read-only)
+node0: TEST_DIR/t.IMGFMT (file)
 QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0,read-only=off: Could not open 'TEST_DIR/t.IMGFMT': Permission denied
 
 QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0,auto-read-only=off: Could not open 'TEST_DIR/t.IMGFMT': Permission denied
-node0: TEST_DIR/t.IMGFMT (file, read-only)
+node0: TEST_DIR/t.IMGFMT (file)
 QEMU_PROG: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=node0: Could not open 'TEST_DIR/t.IMGFMT': Permission denied
 
 === Try commit to backing file with auto-read-only ===
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 03/10] block: Avoid useless local_err

[Kevin Wolf]

Am 11.03.2019 um 17:50 hat Kevin Wolf geschrieben:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Forgot to apply a Reviewed-by from Berto. I've added it locally now.

Kevin

Re: [Qemu-devel] [PATCH v2 10/10] file-posix: Make auto-read-only dynamic

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1751 bytes --]

On 3/11/19 11:50 AM, Kevin Wolf wrote:
> Until now, with auto-read-only=on we tried to open the file read-write
> first and if that failed, read-only was tried. This is actually not good
> enough for libvirt, which gives QEMU SELinux permissions for read-write
> only as soon as it actually intends to write to the image. So we need to
> be able to switch between read-only and read-write at runtime.
> 
> This patch makes auto-read-only dynamic, i.e. the file is opened
> read-only as long as no user of the node has requested write
> permissions, but it is automatically reopened read-write as soon as the
> first writer is attached. Conversely, if the last writer goes away, the
> file is reopened read-only again.
> 
> bs->read_only is no longer set for auto-read-only=on files even if the
> file descriptor is opened read-only because it will be transparently
> upgraded as soon as a writer is attached. This changes the output of
> qemu-iotests 232.

auto-read-only was introduced in 3.1, at which point we intentionally
had sufficiently loose wording to permit (but not require) dynamic state
checking; so you are not breaking the interface.  On the other hand, is
libvirt going to have problems introspecting whether it can use
auto-read-only and get the dynamic behavior it needs?  Or is there
enough else in the way of libvirt's switch to -blockdev that it won't
attempt anything that needs auto-read-only without other 4.0 interfaces
anyway, at which point detecting the presence of the field (but not
whether the field has a guarantee of dynamic behavior) on 3.1 doesn't
matter?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 10/10] file-posix: Make auto-read-only dynamic

[Peter Krempa]

[-- Attachment #1: Type: text/plain, Size: 1897 bytes --]

On Mon, Mar 11, 2019 at 12:26:08 -0500, Eric Blake wrote:
> On 3/11/19 11:50 AM, Kevin Wolf wrote:
> > Until now, with auto-read-only=on we tried to open the file read-write
> > first and if that failed, read-only was tried. This is actually not good
> > enough for libvirt, which gives QEMU SELinux permissions for read-write
> > only as soon as it actually intends to write to the image. So we need to
> > be able to switch between read-only and read-write at runtime.
> > 
> > This patch makes auto-read-only dynamic, i.e. the file is opened
> > read-only as long as no user of the node has requested write
> > permissions, but it is automatically reopened read-write as soon as the
> > first writer is attached. Conversely, if the last writer goes away, the
> > file is reopened read-only again.
> > 
> > bs->read_only is no longer set for auto-read-only=on files even if the
> > file descriptor is opened read-only because it will be transparently
> > upgraded as soon as a writer is attached. This changes the output of
> > qemu-iotests 232.
> 
> auto-read-only was introduced in 3.1, at which point we intentionally
> had sufficiently loose wording to permit (but not require) dynamic state
> checking; so you are not breaking the interface.  On the other hand, is
> libvirt going to have problems introspecting whether it can use
> auto-read-only and get the dynamic behavior it needs?  Or is there
> enough else in the way of libvirt's switch to -blockdev that it won't
> attempt anything that needs auto-read-only without other 4.0 interfaces
> anyway, at which point detecting the presence of the field (but not
> whether the field has a guarantee of dynamic behavior) on 3.1 doesn't
> matter?

I think we can use Stefan's capability detection mechanism he introduced
for the migration with cache enabled for local files to add a flag for
this as well.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v2 10/10] file-posix: Make auto-read-only dynamic

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1449 bytes --]

On 3/11/19 2:59 PM, Peter Krempa wrote:

>> auto-read-only was introduced in 3.1, at which point we intentionally
>> had sufficiently loose wording to permit (but not require) dynamic state
>> checking; so you are not breaking the interface.  On the other hand, is
>> libvirt going to have problems introspecting whether it can use
>> auto-read-only and get the dynamic behavior it needs?  Or is there
>> enough else in the way of libvirt's switch to -blockdev that it won't
>> attempt anything that needs auto-read-only without other 4.0 interfaces
>> anyway, at which point detecting the presence of the field (but not
>> whether the field has a guarantee of dynamic behavior) on 3.1 doesn't
>> matter?
> 
> I think we can use Stefan's capability detection mechanism he introduced
> for the migration with cache enabled for local files to add a flag for
> this as well.

Except I thought we decided that the most recent version of his QMP
changes was now fully-introspectible, thanks to using conditional
compilation.

https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg02510.html

Well, that may prove to be a short-lived hiatus, if libvirt would
happily attempt to use qemu 3.1 and fail without some other
introspectible hook to know whether auto-read-only has required semantics.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 10/10] file-posix: Make auto-read-only dynamic

[Peter Krempa]

[-- Attachment #1: Type: text/plain, Size: 1814 bytes --]

On Mon, Mar 11, 2019 at 15:10:36 -0500, Eric Blake wrote:
> On 3/11/19 2:59 PM, Peter Krempa wrote:
> 
> >> auto-read-only was introduced in 3.1, at which point we intentionally
> >> had sufficiently loose wording to permit (but not require) dynamic state
> >> checking; so you are not breaking the interface.  On the other hand, is
> >> libvirt going to have problems introspecting whether it can use
> >> auto-read-only and get the dynamic behavior it needs?  Or is there
> >> enough else in the way of libvirt's switch to -blockdev that it won't
> >> attempt anything that needs auto-read-only without other 4.0 interfaces
> >> anyway, at which point detecting the presence of the field (but not
> >> whether the field has a guarantee of dynamic behavior) on 3.1 doesn't
> >> matter?
> > 
> > I think we can use Stefan's capability detection mechanism he introduced
> > for the migration with cache enabled for local files to add a flag for
> > this as well.
> 
> Except I thought we decided that the most recent version of his QMP
> changes was now fully-introspectible, thanks to using conditional
> compilation.
> 
> https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg02510.html

Oh, bummer, I missed that it was no longer needed. I still think it's
worth adding that for future use once it will be necessary to detect
that certain things were patched and require libvirt to change behaviour
if that's the case.

> Well, that may prove to be a short-lived hiatus, if libvirt would
> happily attempt to use qemu 3.1 and fail without some other
> introspectible hook to know whether auto-read-only has required semantics.
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
> 




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v2 00/10] file-posix: Make auto-read-only dynamic

[Peter Krempa]

[-- Attachment #1: Type: text/plain, Size: 1419 bytes --]

On Mon, Mar 11, 2019 at 17:50:07 +0100, Kevin Wolf wrote:
> We introduced the auto-read-only option to fix the problem that block
> jobs that reopen a backing file read-write don't work any more when all
> nodes are created individually with -blockdev. The reason is that
> bs->file of these backing files doesn't inherit the read-only option
> from the format layer node any more if it's created separately.
> 
> The way auto-read-only was designed to fix this is that it just always
> opens the file node read-write if it can, so reopening the format layer
> node is enough to make the backing file writable when necessary.
> 
> This works in principle, but not when libvirt uses sVirt: Then QEMU
> doesn't even have the permissions to open the image file read-write
> until libvirt performs an operation where write access is needed.
> 
> This series changes auto-read-only so that it works dynamically and
> automatically reopens the file read-only or read-write depending on the
> permissions that users attached to the node requested.
> 
> See also: https://bugzilla.redhat.com/show_bug.cgi?id=1685989
> 
> v2:
> - Added test for and fixed snapshot=on,read-only=on regression [Berto]
> - Added test for and fixed commit regression [Peter]

I've quickly tested it with non-active commit and it works in this
scenario as expected.

I'll give it a bit more thorough test in the morning.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v2 10/10] file-posix: Make auto-read-only dynamic

[Markus Armbruster]

Peter Krempa <pkrempa@redhat.com> writes:

> On Mon, Mar 11, 2019 at 15:10:36 -0500, Eric Blake wrote:
>> On 3/11/19 2:59 PM, Peter Krempa wrote:
>> 
>> >> auto-read-only was introduced in 3.1, at which point we intentionally
>> >> had sufficiently loose wording to permit (but not require) dynamic state
>> >> checking; so you are not breaking the interface.  On the other hand, is
>> >> libvirt going to have problems introspecting whether it can use
>> >> auto-read-only and get the dynamic behavior it needs?  Or is there
>> >> enough else in the way of libvirt's switch to -blockdev that it won't
>> >> attempt anything that needs auto-read-only without other 4.0 interfaces
>> >> anyway, at which point detecting the presence of the field (but not
>> >> whether the field has a guarantee of dynamic behavior) on 3.1 doesn't
>> >> matter?
>> > 
>> > I think we can use Stefan's capability detection mechanism he introduced
>> > for the migration with cache enabled for local files to add a flag for
>> > this as well.
>> 
>> Except I thought we decided that the most recent version of his QMP
>> changes was now fully-introspectible, thanks to using conditional
>> compilation.
>> 
>> https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg02510.html
>
> Oh, bummer, I missed that it was no longer needed. I still think it's
> worth adding that for future use once it will be necessary to detect
> that certain things were patched and require libvirt to change behaviour
> if that's the case.

We'll add it when we have a compelling use for it.

>> Well, that may prove to be a short-lived hiatus, if libvirt would
>> happily attempt to use qemu 3.1 and fail without some other
>> introspectible hook to know whether auto-read-only has required semantics.

Re: [Qemu-devel] [PATCH v2 01/10] tests/virtio-blk-test: Disable auto-read-only

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1345 bytes --]

On 3/11/19 11:50 AM, Kevin Wolf wrote:
> tests/virtio-blk-test uses a temporary image file that it deletes while
> QEMU is still running, so it can't be reopened when writers are
> attached or detached. Disable auto-read-only to keep it always writable.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/virtio-blk-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Looks like I started reviewing v1 even though I see v2 has been posted.
R-b carries forward:

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
> index b02be0274e..b65365934b 100644
> --- a/tests/virtio-blk-test.c
> +++ b/tests/virtio-blk-test.c
> @@ -751,7 +751,7 @@ static void *virtio_blk_test_setup(GString *cmd_line, void *arg)
>      char *tmp_path = drive_create();
>  
>      g_string_append_printf(cmd_line,
> -                           " -drive if=none,id=drive0,file=%s,format=raw "
> +                           " -drive if=none,id=drive0,file=%s,format=raw,auto-read-only=off "
>                             "-drive if=none,id=drive1,file=null-co://,format=raw ",
>                             tmp_path);
>  
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 02/10] qemu-iotests: commit to backing file with auto-read-only

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 441 bytes --]

On 3/11/19 11:50 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/232     | 31 +++++++++++++++++++++++++++++++
>  tests/qemu-iotests/232.out | 20 ++++++++++++++++++++
>  2 files changed, 51 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 03/10] block: Avoid useless local_err

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1124 bytes --]

On 3/11/19 11:50 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

> diff --git a/block.c b/block.c
> index ccf008c177..e18bd5eefd 100644
> --- a/block.c
> +++ b/block.c
> @@ -3155,14 +3155,12 @@ int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error **er
>  {
>      int ret = -1;
>      BlockReopenQueueEntry *bs_entry, *next;
> -    Error *local_err = NULL;
>  
>      assert(bs_queue != NULL);
>  
>      QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
>          assert(bs_entry->state.bs->quiesce_counter > 0);
> -        if (bdrv_reopen_prepare(&bs_entry->state, bs_queue, &local_err)) {
> -            error_propagate(errp, local_err);
> +        if (bdrv_reopen_prepare(&bs_entry->state, bs_queue, errp)) {
>              goto cleanup;
>          }
>          bs_entry->prepared = true;
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v2 10/10] file-posix: Make auto-read-only dynamic

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2660 bytes --]

On 11.03.19 17:50, Kevin Wolf wrote:
> Until now, with auto-read-only=on we tried to open the file read-write
> first and if that failed, read-only was tried. This is actually not good
> enough for libvirt, which gives QEMU SELinux permissions for read-write
> only as soon as it actually intends to write to the image. So we need to
> be able to switch between read-only and read-write at runtime.
> 
> This patch makes auto-read-only dynamic, i.e. the file is opened
> read-only as long as no user of the node has requested write
> permissions, but it is automatically reopened read-write as soon as the
> first writer is attached. Conversely, if the last writer goes away, the
> file is reopened read-only again.
> 
> bs->read_only is no longer set for auto-read-only=on files even if the
> file descriptor is opened read-only because it will be transparently
> upgraded as soon as a writer is attached. This changes the output of
> qemu-iotests 232.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/file-posix.c         | 36 +++++++++++++++++-------------------
>  tests/qemu-iotests/232.out | 12 ++++++------
>  2 files changed, 23 insertions(+), 25 deletions(-)

https://bugzilla.redhat.com/show_bug.cgi?id=1703793 seems to be caused
by this patch: When the mirror job completes, it drops all permissions
on its target BB with an &error_abort.  As of this patch, this may
result in file-posix attempting to reopen the FD, which may fail.

There are two problems I can see: First, the previous patch should have
updated s->open_flags along with s->fd when the FD is switched.  As it
is now, s->open_flags is not updated, so it stays on O_RDONLY and every
time the permissions are checked, the FD is reconfigured and then switched.

That's simple to fix, just add BDRVRawState.perm_change_flags and set it
to open_flags after raw_reconfigure_getfd() returned a ret != s->fd
(when s->perm_change_fd is set).

That fixes the problem of file-posix attempting to reopen the FD to
O_RDWR all the time, which caused the crash.

But that gives us another crash, because now dropping the permissions
(correctly) reopens the FD to O_RDONLY, with the exact same implications
as above: If the target becomes unavailable, opening the new FD will
fail, and qemu will crash.

I don't know what to do about this.  In the spirit of "dropping
permissions should always work", I presume raw_reconfigure_getfd()
should just return the old FD if it had more permissions than the new
one would have.  But if the user issues an explicit reopen command, they
probably want such an error to be reported.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v2 10/10] file-posix: Make auto-read-only dynamic

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 3341 bytes --]

Am 29.04.2019 um 22:16 hat Max Reitz geschrieben:
> On 11.03.19 17:50, Kevin Wolf wrote:
> > Until now, with auto-read-only=on we tried to open the file read-write
> > first and if that failed, read-only was tried. This is actually not good
> > enough for libvirt, which gives QEMU SELinux permissions for read-write
> > only as soon as it actually intends to write to the image. So we need to
> > be able to switch between read-only and read-write at runtime.
> > 
> > This patch makes auto-read-only dynamic, i.e. the file is opened
> > read-only as long as no user of the node has requested write
> > permissions, but it is automatically reopened read-write as soon as the
> > first writer is attached. Conversely, if the last writer goes away, the
> > file is reopened read-only again.
> > 
> > bs->read_only is no longer set for auto-read-only=on files even if the
> > file descriptor is opened read-only because it will be transparently
> > upgraded as soon as a writer is attached. This changes the output of
> > qemu-iotests 232.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/file-posix.c         | 36 +++++++++++++++++-------------------
> >  tests/qemu-iotests/232.out | 12 ++++++------
> >  2 files changed, 23 insertions(+), 25 deletions(-)
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1703793 seems to be caused
> by this patch: When the mirror job completes, it drops all permissions
> on its target BB with an &error_abort.  As of this patch, this may
> result in file-posix attempting to reopen the FD, which may fail.

Hm, yes. But we already agreed that dropping permissions should never
fail, so maybe we should just implement that now (in block.c).

> There are two problems I can see: First, the previous patch should have
> updated s->open_flags along with s->fd when the FD is switched.  As it
> is now, s->open_flags is not updated, so it stays on O_RDONLY and every
> time the permissions are checked, the FD is reconfigured and then switched.
> 
> That's simple to fix, just add BDRVRawState.perm_change_flags and set it
> to open_flags after raw_reconfigure_getfd() returned a ret != s->fd
> (when s->perm_change_fd is set).

...and actually do s->open_flags = s->perm_change_flags in
raw_set_perm(), otherwise it doesn't help much. :-)

> That fixes the problem of file-posix attempting to reopen the FD to
> O_RDWR all the time, which caused the crash.

Good.

> But that gives us another crash, because now dropping the permissions
> (correctly) reopens the FD to O_RDONLY, with the exact same implications
> as above: If the target becomes unavailable, opening the new FD will
> fail, and qemu will crash.
> 
> I don't know what to do about this.  In the spirit of "dropping
> permissions should always work", I presume raw_reconfigure_getfd()
> should just return the old FD if it had more permissions than the new
> one would have.  But if the user issues an explicit reopen command, they
> probably want such an error to be reported.

Yes, I think file-posix should let the operation fail (because it
actually failed) and the permission functions in block.c should ignore
the error for dropping permissions. This way, reopen will still
correctly return the error because it has a different call chain.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH v2 10/10] file-posix: Make auto-read-only dynamic

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 3341 bytes --]

Am 29.04.2019 um 22:16 hat Max Reitz geschrieben:
> On 11.03.19 17:50, Kevin Wolf wrote:
> > Until now, with auto-read-only=on we tried to open the file read-write
> > first and if that failed, read-only was tried. This is actually not good
> > enough for libvirt, which gives QEMU SELinux permissions for read-write
> > only as soon as it actually intends to write to the image. So we need to
> > be able to switch between read-only and read-write at runtime.
> > 
> > This patch makes auto-read-only dynamic, i.e. the file is opened
> > read-only as long as no user of the node has requested write
> > permissions, but it is automatically reopened read-write as soon as the
> > first writer is attached. Conversely, if the last writer goes away, the
> > file is reopened read-only again.
> > 
> > bs->read_only is no longer set for auto-read-only=on files even if the
> > file descriptor is opened read-only because it will be transparently
> > upgraded as soon as a writer is attached. This changes the output of
> > qemu-iotests 232.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/file-posix.c         | 36 +++++++++++++++++-------------------
> >  tests/qemu-iotests/232.out | 12 ++++++------
> >  2 files changed, 23 insertions(+), 25 deletions(-)
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1703793 seems to be caused
> by this patch: When the mirror job completes, it drops all permissions
> on its target BB with an &error_abort.  As of this patch, this may
> result in file-posix attempting to reopen the FD, which may fail.

Hm, yes. But we already agreed that dropping permissions should never
fail, so maybe we should just implement that now (in block.c).

> There are two problems I can see: First, the previous patch should have
> updated s->open_flags along with s->fd when the FD is switched.  As it
> is now, s->open_flags is not updated, so it stays on O_RDONLY and every
> time the permissions are checked, the FD is reconfigured and then switched.
> 
> That's simple to fix, just add BDRVRawState.perm_change_flags and set it
> to open_flags after raw_reconfigure_getfd() returned a ret != s->fd
> (when s->perm_change_fd is set).

...and actually do s->open_flags = s->perm_change_flags in
raw_set_perm(), otherwise it doesn't help much. :-)

> That fixes the problem of file-posix attempting to reopen the FD to
> O_RDWR all the time, which caused the crash.

Good.

> But that gives us another crash, because now dropping the permissions
> (correctly) reopens the FD to O_RDONLY, with the exact same implications
> as above: If the target becomes unavailable, opening the new FD will
> fail, and qemu will crash.
> 
> I don't know what to do about this.  In the spirit of "dropping
> permissions should always work", I presume raw_reconfigure_getfd()
> should just return the old FD if it had more permissions than the new
> one would have.  But if the user issues an explicit reopen command, they
> probably want such an error to be reported.

Yes, I think file-posix should let the operation fail (because it
actually failed) and the permission functions in block.c should ignore
the error for dropping permissions. This way, reopen will still
correctly return the error because it has a different call chain.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]