[Qemu-devel] [PULL 00/10] Block layer fixes for 2.9.0-rc4

[Qemu-devel] [PULL 00/10] Block layer fixes for 2.9.0-rc4

[Kevin Wolf]

The following changes since commit 5fe2339e6b09da7d6f48b9bef0f1a7360392b489:

  Merge remote-tracking branch 'remotes/awilliam/tags/vfio-updates-20170406.0' into staging (2017-04-07 10:29:56 +0100)

are available in the git repository at:


  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 19dd29e8a77cd820515de5289f566508e0ed4926:

  mirror: Fix aio context of mirror_top_bs (2017-04-07 14:44:06 +0200)

----------------------------------------------------------------
Block layer fixes for 2.9.0-rc4

----------------------------------------------------------------
Fam Zheng (3):
      block: Fix unpaired aio_disable_external in external snapshot
      block: Assert attached child node has right aio context
      mirror: Fix aio context of mirror_top_bs

Jeff Cody (1):
      qemu-img: img_create does not support image-opts, fix docs

Kevin Wolf (4):
      block: Ignore guest dev permissions during incoming migration
      commit: Set commit_top_bs->aio_context
      commit: Set commit_top_bs->total_sectors
      block: Don't check permissions for copy on read

Max Reitz (2):
      block/mirror: Fix use-after-free
      iotests: Add mirror tests for orphaned source

 block.c                       |  4 ++++
 block/block-backend.c         | 40 ++++++++++++++++++++++++++++++++++++-
 block/commit.c                |  3 +++
 block/io.c                    |  9 ++++++++-
 block/mirror.c                | 13 ++++++++++--
 blockdev.c                    |  4 ++--
 include/block/block.h         |  2 ++
 migration/migration.c         |  8 ++++++++
 qemu-img-cmds.hx              |  4 ++--
 qmp.c                         |  6 ++++++
 tests/qemu-iotests/041        | 46 +++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/041.out    |  4 ++--
 tests/qemu-iotests/iotests.py | 15 ++++++++++++++
 13 files changed, 148 insertions(+), 10 deletions(-)

[Qemu-devel] [PULL 01/10] block: Ignore guest dev permissions during incoming migration

[Kevin Wolf]

Usually guest devices don't like other writers to the same image, so
they use blk_set_perm() to prevent this from happening. In the migration
phase before the VM is actually running, though, they don't have a
problem with writes to the image. On the other hand, storage migration
needs to be able to write to the image in this phase, so the restrictive
blk_set_perm() call of qdev devices breaks it.

This patch flags all BlockBackends with a qdev device as
blk->disable_perm during incoming migration, which means that the
requested permissions are stored in the BlockBackend, but not actually
applied to its root node yet.

Once migration has finished and the VM should be resumed, the
permissions are applied. If they cannot be applied (e.g. because the NBD
server used for block migration hasn't been shut down), resuming the VM
fails.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Tested-by: Kashyap Chamarthy <kchamart@redhat.com>
---
 block/block-backend.c | 40 +++++++++++++++++++++++++++++++++++++++-
 include/block/block.h |  2 ++
 migration/migration.c |  8 ++++++++
 qmp.c                 |  6 ++++++
 4 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 0b63773..18ece99 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -61,6 +61,7 @@ struct BlockBackend {
 
     uint64_t perm;
     uint64_t shared_perm;
+    bool disable_perm;
 
     bool allow_write_beyond_eof;
 
@@ -578,7 +579,7 @@ int blk_set_perm(BlockBackend *blk, uint64_t perm, uint64_t shared_perm,
 {
     int ret;
 
-    if (blk->root) {
+    if (blk->root && !blk->disable_perm) {
         ret = bdrv_child_try_set_perm(blk->root, perm, shared_perm, errp);
         if (ret < 0) {
             return ret;
@@ -597,15 +598,52 @@ void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm)
     *shared_perm = blk->shared_perm;
 }
 
+/*
+ * Notifies the user of all BlockBackends that migration has completed. qdev
+ * devices can tighten their permissions in response (specifically revoke
+ * shared write permissions that we needed for storage migration).
+ *
+ * If an error is returned, the VM cannot be allowed to be resumed.
+ */
+void blk_resume_after_migration(Error **errp)
+{
+    BlockBackend *blk;
+    Error *local_err = NULL;
+
+    for (blk = blk_all_next(NULL); blk; blk = blk_all_next(blk)) {
+        if (!blk->disable_perm) {
+            continue;
+        }
+
+        blk->disable_perm = false;
+
+        blk_set_perm(blk, blk->perm, blk->shared_perm, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            blk->disable_perm = true;
+            return;
+        }
+    }
+}
+
 static int blk_do_attach_dev(BlockBackend *blk, void *dev)
 {
     if (blk->dev) {
         return -EBUSY;
     }
+
+    /* While migration is still incoming, we don't need to apply the
+     * permissions of guest device BlockBackends. We might still have a block
+     * job or NBD server writing to the image for storage migration. */
+    if (runstate_check(RUN_STATE_INMIGRATE)) {
+        blk->disable_perm = true;
+    }
+
     blk_ref(blk);
     blk->dev = dev;
     blk->legacy_dev = false;
     blk_iostatus_reset(blk);
+
     return 0;
 }
 
diff --git a/include/block/block.h b/include/block/block.h
index 5149260..3e09222 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -366,6 +366,8 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp);
 void bdrv_invalidate_cache_all(Error **errp);
 int bdrv_inactivate_all(void);
 
+void blk_resume_after_migration(Error **errp);
+
 /* Ensure contents are flushed to disk.  */
 int bdrv_flush(BlockDriverState *bs);
 int coroutine_fn bdrv_co_flush(BlockDriverState *bs);
diff --git a/migration/migration.c b/migration/migration.c
index 54060f7..ad4036f 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -349,6 +349,14 @@ static void process_incoming_migration_bh(void *opaque)
         exit(EXIT_FAILURE);
     }
 
+    /* If we get an error here, just don't restart the VM yet. */
+    blk_resume_after_migration(&local_err);
+    if (local_err) {
+        error_free(local_err);
+        local_err = NULL;
+        autostart = false;
+    }
+
     /*
      * This must happen after all error conditions are dealt with and
      * we're sure the VM is going to be running on this host.
diff --git a/qmp.c b/qmp.c
index fa82b59..a744e44 100644
--- a/qmp.c
+++ b/qmp.c
@@ -207,6 +207,12 @@ void qmp_cont(Error **errp)
         }
     }
 
+    blk_resume_after_migration(&local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
     if (runstate_check(RUN_STATE_INMIGRATE)) {
         autostart = 1;
     } else {
-- 
1.8.3.1

[Qemu-devel] [PULL 02/10] commit: Set commit_top_bs->aio_context

[Kevin Wolf]

The filter driver that is inserted by the commit job needs to use the
same AioContext as its parent and child nodes.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
 block/commit.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/commit.c b/block/commit.c
index 2832482..4c38220 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -335,6 +335,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
     if (commit_top_bs == NULL) {
         goto fail;
     }
+    bdrv_set_aio_context(commit_top_bs, bdrv_get_aio_context(top));
 
     bdrv_set_backing_hd(commit_top_bs, top, &local_err);
     if (local_err) {
@@ -482,6 +483,7 @@ int bdrv_commit(BlockDriverState *bs)
         error_report_err(local_err);
         goto ro_cleanup;
     }
+    bdrv_set_aio_context(commit_top_bs, bdrv_get_aio_context(backing_file_bs));
 
     bdrv_set_backing_hd(commit_top_bs, backing_file_bs, &error_abort);
     bdrv_set_backing_hd(bs, commit_top_bs, &error_abort);
-- 
1.8.3.1

[Qemu-devel] [PULL 03/10] commit: Set commit_top_bs->total_sectors

[Kevin Wolf]

Like in the mirror filter driver, we also need to set the image size for
the commit filter driver. This is less likely to be a problem in
practice than for the mirror because we're not at the active layer here,
but attaching new parents to a node in the middle of the chain is
possible, so the size needs to be correct anyway.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
 block/commit.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/commit.c b/block/commit.c
index 4c38220..91d2c34 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -335,6 +335,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
     if (commit_top_bs == NULL) {
         goto fail;
     }
+    commit_top_bs->total_sectors = top->total_sectors;
     bdrv_set_aio_context(commit_top_bs, bdrv_get_aio_context(top));
 
     bdrv_set_backing_hd(commit_top_bs, top, &local_err);
-- 
1.8.3.1

[Qemu-devel] [PULL 04/10] block/mirror: Fix use-after-free

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

If @bs does not have any parents, the only reference to @mirror_top_bs
will be held by the BlockJob object after the bdrv_unref() following
block_job_create(). However, if block_job_create() fails, this reference
will not exist and @mirror_top_bs will have been deleted when we
goto fail.

The issue comes back at all later entries to the fail label: We delete
the BlockJob object before rolling back our changes to the node graph.
This means that we will delete @mirror_top_bs in the process.

All in all, whenever @bs does not have any parents and we go down the
fail path we will dereference @mirror_top_bs after it has been deleted.

Fix this by invoking bdrv_unref() only when block_job_create() was
successful and by bdrv_ref()'ing @mirror_top_bs in the fail path before
deleting the BlockJob object. Finally, bdrv_unref() it at the end of the
fail path after we actually no longer need it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/mirror.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 9e2fecc..46ecd38 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1150,7 +1150,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
     mirror_top_bs->total_sectors = bs->total_sectors;
 
     /* bdrv_append takes ownership of the mirror_top_bs reference, need to keep
-     * it alive until block_job_create() even if bs has no parent. */
+     * it alive until block_job_create() succeeds even if bs has no parent. */
     bdrv_ref(mirror_top_bs);
     bdrv_drained_begin(bs);
     bdrv_append(mirror_top_bs, bs, &local_err);
@@ -1168,10 +1168,12 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
                          BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
                          BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD, speed,
                          creation_flags, cb, opaque, errp);
-    bdrv_unref(mirror_top_bs);
     if (!s) {
         goto fail;
     }
+    /* The block job now has a reference to this node */
+    bdrv_unref(mirror_top_bs);
+
     s->source = bs;
     s->mirror_top_bs = mirror_top_bs;
 
@@ -1242,6 +1244,10 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
 
 fail:
     if (s) {
+        /* Make sure this BDS does not go away until we have completed the graph
+         * changes below */
+        bdrv_ref(mirror_top_bs);
+
         g_free(s->replaces);
         blk_unref(s->target);
         block_job_unref(&s->common);
@@ -1250,6 +1256,8 @@ fail:
     bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
                             &error_abort);
     bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
+
+    bdrv_unref(mirror_top_bs);
 }
 
 void mirror_start(const char *job_id, BlockDriverState *bs,
-- 
1.8.3.1

[Qemu-devel] [PULL 05/10] iotests: Add mirror tests for orphaned source

[Kevin Wolf]

From: Max Reitz <mreitz@redhat.com>

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/041        | 46 +++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/041.out    |  4 ++--
 tests/qemu-iotests/iotests.py | 15 ++++++++++++++
 3 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index bc6cf78..2f54986 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -966,5 +966,51 @@ class TestRepairQuorum(iotests.QMPTestCase):
         #       to check that this file is really driven by quorum
         self.vm.shutdown()
 
+# Test mirroring with a source that does not have any parents (not even a
+# BlockBackend)
+class TestOrphanedSource(iotests.QMPTestCase):
+    def setUp(self):
+        blk0 = { 'node-name': 'src',
+                 'driver': 'null-co' }
+
+        blk1 = { 'node-name': 'dest',
+                 'driver': 'null-co' }
+
+        blk2 = { 'node-name': 'dest-ro',
+                 'driver': 'null-co',
+                 'read-only': 'on' }
+
+        self.vm = iotests.VM()
+        self.vm.add_blockdev(self.qmp_to_opts(blk0))
+        self.vm.add_blockdev(self.qmp_to_opts(blk1))
+        self.vm.add_blockdev(self.qmp_to_opts(blk2))
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+
+    def test_no_job_id(self):
+        self.assert_no_active_block_jobs()
+
+        result = self.vm.qmp('blockdev-mirror', device='src', sync='full',
+                             target='dest')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+    def test_success(self):
+        self.assert_no_active_block_jobs()
+
+        result = self.vm.qmp('blockdev-mirror', job_id='job', device='src',
+                             sync='full', target='dest')
+        self.assert_qmp(result, 'return', {})
+
+        self.complete_and_wait('job')
+
+    def test_failing_permissions(self):
+        self.assert_no_active_block_jobs()
+
+        result = self.vm.qmp('blockdev-mirror', device='src', sync='full',
+                             target='dest-ro')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2', 'qed'])
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
index b67d050..e30fd3b 100644
--- a/tests/qemu-iotests/041.out
+++ b/tests/qemu-iotests/041.out
@@ -1,5 +1,5 @@
-............................................................................
+...............................................................................
 ----------------------------------------------------------------------
-Ran 76 tests
+Ran 79 tests
 
 OK
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index bec8eb4..abcf3c1 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -177,6 +177,14 @@ class VM(qtest.QEMUQtestMachine):
         self._num_drives += 1
         return self
 
+    def add_blockdev(self, opts):
+        self._args.append('-blockdev')
+        if isinstance(opts, str):
+            self._args.append(opts)
+        else:
+            self._args.append(','.join(opts))
+        return self
+
     def pause_drive(self, drive, event=None):
         '''Pause drive r/w operations'''
         if not event:
@@ -235,6 +243,13 @@ class QMPTestCase(unittest.TestCase):
             output[basestr[:-1]] = obj # Strip trailing '.'
         return output
 
+    def qmp_to_opts(self, obj):
+        obj = self.flatten_qmp_object(obj)
+        output_list = list()
+        for key in obj:
+            output_list += [key + '=' + obj[key]]
+        return ','.join(output_list)
+
     def assert_qmp_absent(self, d, path):
         try:
             result = self.dictpath(d, path)
-- 
1.8.3.1

[Qemu-devel] [PULL 06/10] qemu-img: img_create does not support image-opts, fix docs

[Kevin Wolf]

From: Jeff Cody <jcody@redhat.com>

The documentation and help for qemu-img claims that 'qemu-img create'
will take the '--image-opts' argument.  This is not true, so this
patch removes those claims.

Signed-off-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img-cmds.hx | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 9c9702c..8ac7822 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -22,9 +22,9 @@ STEXI
 ETEXI
 
 DEF("create", img_create,
-    "create [-q] [--object objectdef] [--image-opts] [-f fmt] [-o options] filename [size]")
+    "create [-q] [--object objectdef] [-f fmt] [-o options] filename [size]")
 STEXI
-@item create [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] [-o @var{options}] @var{filename} [@var{size}]
+@item create [--object @var{objectdef}] [-q] [-f @var{fmt}] [-o @var{options}] @var{filename} [@var{size}]
 ETEXI
 
 DEF("commit", img_commit,
-- 
1.8.3.1

[Qemu-devel] [PULL 07/10] block: Don't check permissions for copy on read

[Kevin Wolf]

The assertion is currently failing. We can't require callers to have
write permissions when all they are doing is a read, so comment it out.
Add a FIXME comment in the code so that the check is re-enabled when
copy on read is refactored into its own filter driver.

Reported-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
---
 block/io.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index 2709a70..7321dda 100644
--- a/block/io.c
+++ b/block/io.c
@@ -945,7 +945,14 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
     size_t skip_bytes;
     int ret;
 
-    assert(child->perm & (BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE));
+    /* FIXME We cannot require callers to have write permissions when all they
+     * are doing is a read request. If we did things right, write permissions
+     * would be obtained anyway, but internally by the copy-on-read code. As
+     * long as it is implemented here rather than in a separat filter driver,
+     * the copy-on-read code doesn't have its own BdrvChild, however, for which
+     * it could request permissions. Therefore we have to bypass the permission
+     * system for the moment. */
+    // assert(child->perm & (BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE));
 
     /* Cover entire cluster so no additional backing file I/O is required when
      * allocating cluster in the image file.
-- 
1.8.3.1

[Qemu-devel] [PULL 08/10] block: Fix unpaired aio_disable_external in external snapshot

[Kevin Wolf]

From: Fam Zheng <famz@redhat.com>

bdrv_replace_child_noperm tries to hand over the quiesce_counter state
from old bs to the new one, but if they are not on the same aio context
this causes unbalance.

Fix this by setting the correct aio context before calling
bdrv_append().

Reported-by: Ed Swierk <eswierk@skyportsystems.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 040c152..4927914 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1772,6 +1772,8 @@ static void external_snapshot_prepare(BlkActionState *common,
         return;
     }
 
+    bdrv_set_aio_context(state->new_bs, state->aio_context);
+
     /* This removes our old bs and adds the new bs. This is an operation that
      * can fail, so we need to do it in .prepare; undoing it for abort is
      * always possible. */
@@ -1789,8 +1791,6 @@ static void external_snapshot_commit(BlkActionState *common)
     ExternalSnapshotState *state =
                              DO_UPCAST(ExternalSnapshotState, common, common);
 
-    bdrv_set_aio_context(state->new_bs, state->aio_context);
-
     /* We don't need (or want) to use the transactional
      * bdrv_reopen_multiple() across all the entries at once, because we
      * don't want to abort all of them if one of them fails the reopen */
-- 
1.8.3.1

[Qemu-devel] [PULL 09/10] block: Assert attached child node has right aio context

[Kevin Wolf]

From: Fam Zheng <famz@redhat.com>

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block.c b/block.c
index 927ba89..b8a3011 100644
--- a/block.c
+++ b/block.c
@@ -1752,6 +1752,9 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
 {
     BlockDriverState *old_bs = child->bs;
 
+    if (old_bs && new_bs) {
+        assert(bdrv_get_aio_context(old_bs) == bdrv_get_aio_context(new_bs));
+    }
     if (old_bs) {
         if (old_bs->quiesce_counter && child->role->drained_end) {
             child->role->drained_end(child);
@@ -1852,6 +1855,7 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
     bdrv_get_cumulative_perm(parent_bs, &perm, &shared_perm);
 
     assert(parent_bs->drv);
+    assert(bdrv_get_aio_context(parent_bs) == bdrv_get_aio_context(child_bs));
     parent_bs->drv->bdrv_child_perm(parent_bs, NULL, child_role,
                                     perm, shared_perm, &perm, &shared_perm);
 
-- 
1.8.3.1

[Qemu-devel] [PULL 10/10] mirror: Fix aio context of mirror_top_bs

[Kevin Wolf]

From: Fam Zheng <famz@redhat.com>

It should be moved to the same context as source, before inserting to the
graph.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/mirror.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/mirror.c b/block/mirror.c
index 46ecd38..164438f 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1148,6 +1148,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
         return;
     }
     mirror_top_bs->total_sectors = bs->total_sectors;
+    bdrv_set_aio_context(mirror_top_bs, bdrv_get_aio_context(bs));
 
     /* bdrv_append takes ownership of the mirror_top_bs reference, need to keep
      * it alive until block_job_create() succeeds even if bs has no parent. */
-- 
1.8.3.1

Re: [Qemu-devel] [PULL 07/10] block: Don't check permissions for copy on read

[Eric Blake]

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

On 04/07/2017 08:47 AM, Kevin Wolf wrote:
> The assertion is currently failing. We can't require callers to have
> write permissions when all they are doing is a read, so comment it out.
> Add a FIXME comment in the code so that the check is re-enabled when
> copy on read is refactored into its own filter driver.
> 
> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
> ---
>  block/io.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 2709a70..7321dda 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -945,7 +945,14 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
>      size_t skip_bytes;
>      int ret;
>  
> -    assert(child->perm & (BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE));
> +    /* FIXME We cannot require callers to have write permissions when all they
> +     * are doing is a read request. If we did things right, write permissions
> +     * would be obtained anyway, but internally by the copy-on-read code. As
> +     * long as it is implemented here rather than in a separat filter driver,

Is there time to fix the typo before the pull happens?

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


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

Re: [Qemu-devel] [PULL 00/10] Block layer fixes for 2.9.0-rc4

[Peter Maydell]

On 7 April 2017 at 14:47, Kevin Wolf <kwolf@redhat.com> wrote:
> The following changes since commit 5fe2339e6b09da7d6f48b9bef0f1a7360392b489:
>
>   Merge remote-tracking branch 'remotes/awilliam/tags/vfio-updates-20170406.0' into staging (2017-04-07 10:29:56 +0100)
>
> are available in the git repository at:
>
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 19dd29e8a77cd820515de5289f566508e0ed4926:
>
>   mirror: Fix aio context of mirror_top_bs (2017-04-07 14:44:06 +0200)
>
> ----------------------------------------------------------------
> Block layer fixes for 2.9.0-rc4
>
> ----------------------------------------------------------------
> Fam Zheng (3):
>       block: Fix unpaired aio_disable_external in external snapshot
>       block: Assert attached child node has right aio context
>       mirror: Fix aio context of mirror_top_bs
>
> Jeff Cody (1):
>       qemu-img: img_create does not support image-opts, fix docs
>
> Kevin Wolf (4):
>       block: Ignore guest dev permissions during incoming migration
>       commit: Set commit_top_bs->aio_context
>       commit: Set commit_top_bs->total_sectors
>       block: Don't check permissions for copy on read
>
> Max Reitz (2):
>       block/mirror: Fix use-after-free
>       iotests: Add mirror tests for orphaned source

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PULL 00/10] Block layer fixes for 2.9.0-rc4

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 1491572865-8549-1-git-send-email-kwolf@redhat.com
Subject: [Qemu-devel] [PULL 00/10] Block layer fixes for 2.9.0-rc4
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
4a9e7db mirror: Fix aio context of mirror_top_bs
79767f5 block: Assert attached child node has right aio context
dfad3a5 block: Fix unpaired aio_disable_external in external snapshot
99be1dc block: Don't check permissions for copy on read
0e5e935 qemu-img: img_create does not support image-opts, fix docs
3afb9fa iotests: Add mirror tests for orphaned source
5ccc43d block/mirror: Fix use-after-free
e65d767 commit: Set commit_top_bs->total_sectors
2dddcd2 commit: Set commit_top_bs->aio_context
5770f38 block: Ignore guest dev permissions during incoming migration

=== OUTPUT BEGIN ===
Checking PATCH 1/10: block: Ignore guest dev permissions during incoming migration...
Checking PATCH 2/10: commit: Set commit_top_bs->aio_context...
Checking PATCH 3/10: commit: Set commit_top_bs->total_sectors...
Checking PATCH 4/10: block/mirror: Fix use-after-free...
Checking PATCH 5/10: iotests: Add mirror tests for orphaned source...
Checking PATCH 6/10: qemu-img: img_create does not support image-opts, fix docs...
Checking PATCH 7/10: block: Don't check permissions for copy on read...
ERROR: do not use C99 // comments
#32: FILE: block/io.c:955:
+    // assert(child->perm & (BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE));

total: 1 errors, 0 warnings, 15 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 8/10: block: Fix unpaired aio_disable_external in external snapshot...
Checking PATCH 9/10: block: Assert attached child node has right aio context...
Checking PATCH 10/10: mirror: Fix aio context of mirror_top_bs...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PULL 00/10] Block layer fixes for 2.9.0-rc4

[Fam Zheng]

On Wed, 04/12 16:51, no-reply@patchew.org wrote:
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Message-id: 1491572865-8549-1-git-send-email-kwolf@redhat.com
> Subject: [Qemu-devel] [PULL 00/10] Block layer fixes for 2.9.0-rc4
> Type: series
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> 
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
> 
> # Useful git options
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> 
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
>     echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>         failed=1
>         echo
>     fi
>     n=$((n+1))
> done
> 
> exit $failed
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> Switched to a new branch 'test'
> 4a9e7db mirror: Fix aio context of mirror_top_bs
> 79767f5 block: Assert attached child node has right aio context
> dfad3a5 block: Fix unpaired aio_disable_external in external snapshot
> 99be1dc block: Don't check permissions for copy on read
> 0e5e935 qemu-img: img_create does not support image-opts, fix docs
> 3afb9fa iotests: Add mirror tests for orphaned source
> 5ccc43d block/mirror: Fix use-after-free
> e65d767 commit: Set commit_top_bs->total_sectors
> 2dddcd2 commit: Set commit_top_bs->aio_context
> 5770f38 block: Ignore guest dev permissions during incoming migration
> 
> === OUTPUT BEGIN ===
> Checking PATCH 1/10: block: Ignore guest dev permissions during incoming migration...
> Checking PATCH 2/10: commit: Set commit_top_bs->aio_context...
> Checking PATCH 3/10: commit: Set commit_top_bs->total_sectors...
> Checking PATCH 4/10: block/mirror: Fix use-after-free...
> Checking PATCH 5/10: iotests: Add mirror tests for orphaned source...
> Checking PATCH 6/10: qemu-img: img_create does not support image-opts, fix docs...
> Checking PATCH 7/10: block: Don't check permissions for copy on read...
> ERROR: do not use C99 // comments
> #32: FILE: block/io.c:955:
> +    // assert(child->perm & (BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE));
> 
> total: 1 errors, 0 warnings, 15 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> Checking PATCH 8/10: block: Fix unpaired aio_disable_external in external snapshot...
> Checking PATCH 9/10: block: Assert attached child node has right aio context...
> Checking PATCH 10/10: mirror: Fix aio context of mirror_top_bs...
> === OUTPUT END ===
> 
> Test command exited with code: 1
> 
> 
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-devel@freelists.org

Hmm, patchew has stalled and a long queue of series is now being processed. I'm
going to disable email notifications for now until it catches up.

Fam