From 9b6abdcfae4401f8078f883d6eca6463c0bfbd68 Mon Sep 17 00:00:00 2001 From: Pan Nengyuan Date: Tue, 10 Mar 2020 14:46:40 +0800 Subject: [PATCH 01/13] qom-qmp-cmds: fix two memleaks in qmp_object_add MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 'type/id' forgot to free in qmp_object_add, this patch fix that. The leak stack: Direct leak of 84 byte(s) in 6 object(s) allocated from: #0 0x7fe2a5ebf768 in __interceptor_malloc (/lib64/libasan.so.5+0xef768) #1 0x7fe2a5044445 in g_malloc (/lib64/libglib-2.0.so.0+0x52445) #2 0x7fe2a505dd92 in g_strdup (/lib64/libglib-2.0.so.0+0x6bd92) #3 0x56344954e692 in qmp_object_add /mnt/sdb/qemu-new/qemu_test/qemu/qom/qom-qmp-cmds.c:258 #4 0x563449960f5a in do_qmp_dispatch /mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:132 #5 0x563449960f5a in qmp_dispatch /mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:175 #6 0x563449498a30 in monitor_qmp_dispatch /mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:145 #7 0x56344949a64f in monitor_qmp_bh_dispatcher /mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:234 #8 0x563449a92a3a in aio_bh_call /mnt/sdb/qemu-new/qemu_test/qemu/util/async.c:136 Direct leak of 54 byte(s) in 6 object(s) allocated from: #0 0x7fe2a5ebf768 in __interceptor_malloc (/lib64/libasan.so.5+0xef768) #1 0x7fe2a5044445 in g_malloc (/lib64/libglib-2.0.so.0+0x52445) #2 0x7fe2a505dd92 in g_strdup (/lib64/libglib-2.0.so.0+0x6bd92) #3 0x56344954e6c4 in qmp_object_add /mnt/sdb/qemu-new/qemu_test/qemu/qom/qom-qmp-cmds.c:267 #4 0x563449960f5a in do_qmp_dispatch /mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:132 #5 0x563449960f5a in qmp_dispatch /mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:175 #6 0x563449498a30 in monitor_qmp_dispatch /mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:145 #7 0x56344949a64f in monitor_qmp_bh_dispatcher /mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:234 #8 0x563449a92a3a in aio_bh_call /mnt/sdb/qemu-new/qemu_test/qemu/util/async.c:136 Fixes: 5f07c4d60d091320186e7b0edaf9ed2cc16b2d1e Reported-by: Euler Robot Signed-off-by: Pan Nengyuan Message-Id: <20200310064640.5059-1-pannengyuan@huawei.com> Reviewed-by: Daniel P. Berrangé Acked-by: Igor Mammedov Signed-off-by: Kevin Wolf --- qom/qom-qmp-cmds.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c index 49db926fcc41..435193b03656 100644 --- a/qom/qom-qmp-cmds.c +++ b/qom/qom-qmp-cmds.c @@ -247,26 +247,22 @@ void qmp_object_add(QDict *qdict, QObject **ret_data, Error **errp) QDict *pdict; Visitor *v; Object *obj; - const char *type; - const char *id; + g_autofree char *type = NULL; + g_autofree char *id = NULL; - type = qdict_get_try_str(qdict, "qom-type"); + type = g_strdup(qdict_get_try_str(qdict, "qom-type")); if (!type) { error_setg(errp, QERR_MISSING_PARAMETER, "qom-type"); return; - } else { - type = g_strdup(type); - qdict_del(qdict, "qom-type"); } + qdict_del(qdict, "qom-type"); - id = qdict_get_try_str(qdict, "id"); + id = g_strdup(qdict_get_try_str(qdict, "id")); if (!id) { error_setg(errp, QERR_MISSING_PARAMETER, "id"); return; - } else { - id = g_strdup(id); - qdict_del(qdict, "id"); } + qdict_del(qdict, "id"); props = qdict_get(qdict, "props"); if (props) { From c7a0f2be8f95b220cdadbba9a9236eaf115951dc Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 10 Mar 2020 12:38:25 +0100 Subject: [PATCH 02/13] block: Make bdrv_get_cumulative_perm() public Signed-off-by: Kevin Wolf Message-Id: <20200310113831.27293-2-kwolf@redhat.com> Reviewed-by: Peter Krempa Signed-off-by: Kevin Wolf --- block.c | 6 ++---- include/block/block_int.h | 3 +++ 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/block.c b/block.c index 957630b1c5d5..79a5a2770f71 100644 --- a/block.c +++ b/block.c @@ -1872,8 +1872,6 @@ static int bdrv_child_check_perm(BdrvChild *c, BlockReopenQueue *q, bool *tighten_restrictions, Error **errp); static void bdrv_child_abort_perm_update(BdrvChild *c); static void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared); -static void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm, - uint64_t *shared_perm); typedef struct BlockReopenQueueEntry { bool prepared; @@ -2097,8 +2095,8 @@ static void bdrv_set_perm(BlockDriverState *bs, uint64_t cumulative_perms, } } -static void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm, - uint64_t *shared_perm) +void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm, + uint64_t *shared_perm) { BdrvChild *c; uint64_t cumulative_perms = 0; diff --git a/include/block/block_int.h b/include/block/block_int.h index 3f70a98b2d58..d8d13700a966 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -1224,6 +1224,9 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs, void *opaque, Error **errp); void bdrv_root_unref_child(BdrvChild *child); +void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm, + uint64_t *shared_perm); + /** * Sets a BdrvChild's permissions. Avoid if the parent is a BDS; use * bdrv_child_refresh_perms() instead and make the parent's From d29d3d1f80b3947fb26e7139645c83de66d146a9 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 10 Mar 2020 12:38:26 +0100 Subject: [PATCH 03/13] block: Relax restrictions for blockdev-snapshot blockdev-snapshot returned an error if the overlay was already in use, which it defined as having any BlockBackend parent. This is in fact both too strict (some parents can tolerate the change of visible data caused by attaching a backing file) and too loose (some non-BlockBackend parents may not be happy with it). One important use case that is prevented by the too strict check is live storage migration with blockdev-mirror. Here, the target node is usually opened without a backing file so that the active layer is mirrored while its backing chain can be copied in the background. The backing chain should be attached to the mirror target node when finalising the job, just before switching the users of the source node to the new copy (at which point the mirror job still has a reference to the node). drive-mirror did this automatically, but with blockdev-mirror this is the job of the QMP client, so it needs a way to do this. blockdev-snapshot is the obvious way, so this patch makes it work in this scenario. The new condition is that no parent uses CONSISTENT_READ permissions. This will ensure that the operation will still be blocked when the node is attached to the guest device, so blockdev-snapshot remains safe. (For the sake of completeness, x-blockdev-reopen can be used to achieve the same, however it is a big hammer, performs the graph change completely unchecked and is still experimental. So even with the option of using x-blockdev-reopen, there are reasons why blockdev-snapshot should be able to perform this operation.) Signed-off-by: Kevin Wolf Message-Id: <20200310113831.27293-3-kwolf@redhat.com> Reviewed-by: Peter Krempa Tested-by: Peter Krempa Signed-off-by: Kevin Wolf --- blockdev.c | 14 ++++++++------ tests/qemu-iotests/085.out | 4 ++-- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/blockdev.c b/blockdev.c index 257cb376825e..ea89896f2712 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1471,6 +1471,7 @@ static void external_snapshot_prepare(BlkActionState *common, TransactionAction *action = common->action; AioContext *aio_context; AioContext *old_context; + uint64_t perm, shared; int ret; /* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar @@ -1586,16 +1587,17 @@ static void external_snapshot_prepare(BlkActionState *common, goto out; } - if (bdrv_has_blk(state->new_bs)) { + /* + * Allow attaching a backing file to an overlay that's already in use only + * if the parents don't assume that they are already seeing a valid image. + * (Specifically, allow it as a mirror target, which is write-only access.) + */ + bdrv_get_cumulative_perm(state->new_bs, &perm, &shared); + if (perm & BLK_PERM_CONSISTENT_READ) { error_setg(errp, "The overlay is already in use"); goto out; } - if (bdrv_op_is_blocked(state->new_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, - errp)) { - goto out; - } - if (state->new_bs->backing != NULL) { error_setg(errp, "The overlay already has a backing image"); goto out; diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out index d94ad22f70ba..fd11aae67822 100644 --- a/tests/qemu-iotests/085.out +++ b/tests/qemu-iotests/085.out @@ -82,7 +82,7 @@ Formatting 'TEST_DIR/12-snapshot-v0.IMGFMT', fmt=IMGFMT size=134217728 backing_f === Invalid command - cannot create a snapshot using a file BDS === { 'execute': 'blockdev-snapshot', 'arguments': { 'node':'virtio0', 'overlay':'file_12' } } -{"error": {"class": "GenericError", "desc": "The overlay does not support backing images"}} +{"error": {"class": "GenericError", "desc": "The overlay is already in use"}} === Invalid command - snapshot node used as active layer === @@ -96,7 +96,7 @@ Formatting 'TEST_DIR/12-snapshot-v0.IMGFMT', fmt=IMGFMT size=134217728 backing_f === Invalid command - snapshot node used as backing hd === { 'execute': 'blockdev-snapshot', 'arguments': { 'node': 'virtio0', 'overlay':'snap_11' } } -{"error": {"class": "GenericError", "desc": "Node 'snap_11' is busy: node is used as backing hd of 'snap_12'"}} +{"error": {"class": "GenericError", "desc": "The overlay is already in use"}} === Invalid command - snapshot node has a backing image === From b31b532122ec6f68d17168449c034d2197bf96ec Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 10 Mar 2020 12:38:27 +0100 Subject: [PATCH 04/13] iotests: Fix run_job() with use_log=False The 'job-complete' QMP command should be run with qmp() rather than qmp_log() if use_log=False is passed. Signed-off-by: Kevin Wolf Message-Id: <20200310113831.27293-4-kwolf@redhat.com> Reviewed-by: Peter Krempa Signed-off-by: Kevin Wolf --- tests/qemu-iotests/iotests.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 8815052eb538..23043baa26b3 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -624,7 +624,10 @@ def run_job(self, job, auto_finalize=True, auto_dismiss=False, if use_log: log('Job failed: %s' % (j['error'])) elif status == 'ready': - self.qmp_log('job-complete', id=job) + if use_log: + self.qmp_log('job-complete', id=job) + else: + self.qmp('job-complete', id=job) elif status == 'pending' and not auto_finalize: if pre_finalize: pre_finalize() From 8bdee9f10eac2aefdcc5095feef756354c87bdec Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 10 Mar 2020 12:38:28 +0100 Subject: [PATCH 05/13] iotests: Test mirror with temporarily disabled target backing file The newly tested scenario is a common live storage migration scenario: The target node is opened without a backing file so that the active layer is mirrored while its backing chain can be copied in the background. The backing chain should be attached to the mirror target node when finalising the job, just before switching the users of the source node to the new copy (at which point the mirror job still has a reference to the node). drive-mirror did this automatically, but with blockdev-mirror this is the job of the QMP client. This patch adds test cases for two ways to achieve the desired result, using either x-blockdev-reopen or blockdev-snapshot. Signed-off-by: Kevin Wolf Message-Id: <20200310113831.27293-5-kwolf@redhat.com> Reviewed-by: Peter Krempa Signed-off-by: Kevin Wolf --- tests/qemu-iotests/155 | 56 ++++++++++++++++++++++++++++++++++---- tests/qemu-iotests/155.out | 4 +-- 2 files changed, 53 insertions(+), 7 deletions(-) diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155 index f237868710e0..74ddefc849f5 100755 --- a/tests/qemu-iotests/155 +++ b/tests/qemu-iotests/155 @@ -45,10 +45,15 @@ target_img = os.path.join(iotests.test_dir, 'target.' + iotests.imgfmt) # image during runtime, only makes sense if # target_blockdev_backing is not None # (None: same as target_backing) +# target_open_with_backing: If True, the target image is added with its backing +# chain opened right away. If False, blockdev-add +# opens it without a backing file and job completion +# is supposed to open the backing chain. class BaseClass(iotests.QMPTestCase): target_blockdev_backing = None target_real_backing = None + target_open_with_backing = True def setUp(self): qemu_img('create', '-f', iotests.imgfmt, back0_img, '1440K') @@ -80,9 +85,13 @@ class BaseClass(iotests.QMPTestCase): options = { 'node-name': 'target', 'driver': iotests.imgfmt, 'file': { 'driver': 'file', + 'node-name': 'target-file', 'filename': target_img } } - if self.target_blockdev_backing: - options['backing'] = self.target_blockdev_backing + + if not self.target_open_with_backing: + options['backing'] = None + elif self.target_blockdev_backing: + options['backing'] = self.target_blockdev_backing result = self.vm.qmp('blockdev-add', **options) self.assert_qmp(result, 'return', {}) @@ -147,10 +156,14 @@ class BaseClass(iotests.QMPTestCase): # cmd: Mirroring command to execute, either drive-mirror or blockdev-mirror class MirrorBaseClass(BaseClass): + def openBacking(self): + pass + def runMirror(self, sync): if self.cmd == 'blockdev-mirror': result = self.vm.qmp(self.cmd, job_id='mirror-job', device='source', - sync=sync, target='target') + sync=sync, target='target', + auto_finalize=False) else: if self.existing: mode = 'existing' @@ -159,11 +172,12 @@ class MirrorBaseClass(BaseClass): result = self.vm.qmp(self.cmd, job_id='mirror-job', device='source', sync=sync, target=target_img, format=iotests.imgfmt, mode=mode, - node_name='target') + node_name='target', auto_finalize=False) self.assert_qmp(result, 'return', {}) - self.complete_and_wait('mirror-job') + self.vm.run_job('mirror-job', use_log=False, auto_finalize=False, + pre_finalize=self.openBacking, auto_dismiss=True) def testFull(self): self.runMirror('full') @@ -221,6 +235,38 @@ class TestBlockdevMirrorForcedBacking(MirrorBaseClass): target_blockdev_backing = { 'driver': 'null-co' } target_real_backing = 'null-co://' +# Attach the backing chain only during completion, with blockdev-reopen +class TestBlockdevMirrorReopen(MirrorBaseClass): + cmd = 'blockdev-mirror' + existing = True + target_backing = 'null-co://' + target_open_with_backing = False + + def openBacking(self): + if not self.target_open_with_backing: + result = self.vm.qmp('blockdev-add', node_name="backing", + driver="null-co") + self.assert_qmp(result, 'return', {}) + result = self.vm.qmp('x-blockdev-reopen', node_name="target", + driver=iotests.imgfmt, file="target-file", + backing="backing") + self.assert_qmp(result, 'return', {}) + +# Attach the backing chain only during completion, with blockdev-snapshot +class TestBlockdevMirrorSnapshot(MirrorBaseClass): + cmd = 'blockdev-mirror' + existing = True + target_backing = 'null-co://' + target_open_with_backing = False + + def openBacking(self): + if not self.target_open_with_backing: + result = self.vm.qmp('blockdev-add', node_name="backing", + driver="null-co") + self.assert_qmp(result, 'return', {}) + result = self.vm.qmp('blockdev-snapshot', node="backing", + overlay="target") + self.assert_qmp(result, 'return', {}) class TestCommit(BaseClass): existing = False diff --git a/tests/qemu-iotests/155.out b/tests/qemu-iotests/155.out index 4176bb940290..4fd1c2dcd218 100644 --- a/tests/qemu-iotests/155.out +++ b/tests/qemu-iotests/155.out @@ -1,5 +1,5 @@ -................... +......................... ---------------------------------------------------------------------- -Ran 19 tests +Ran 25 tests OK From 30dd65f307b647eef8156c4a33bd007823ef85cb Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 10 Mar 2020 12:38:29 +0100 Subject: [PATCH 06/13] block: Fix cross-AioContext blockdev-snapshot external_snapshot_prepare() tries to move the overlay to the AioContext of the backing file (the snapshotted node). However, it's possible that this doesn't work, but the backing file can instead be moved to the overlay's AioContext (e.g. opening the backing chain for a mirror target). bdrv_append() already indirectly uses bdrv_attach_node(), which takes care to move nodes to make sure they use the same AioContext and which tries both directions. So the problem has a simple fix: Just delete the unnecessary extra bdrv_try_set_aio_context() call in external_snapshot_prepare() and instead assert in bdrv_append() that both nodes were indeed moved to the same AioContext. Signed-off-by: Kevin Wolf Message-Id: <20200310113831.27293-6-kwolf@redhat.com> Tested-by: Peter Krempa Signed-off-by: Kevin Wolf --- block.c | 1 + blockdev.c | 16 ---------------- 2 files changed, 1 insertion(+), 16 deletions(-) diff --git a/block.c b/block.c index 79a5a2770f71..8fc7b56937d5 100644 --- a/block.c +++ b/block.c @@ -4365,6 +4365,7 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, bdrv_ref(from); assert(qemu_get_current_aio_context() == qemu_get_aio_context()); + assert(bdrv_get_aio_context(from) == bdrv_get_aio_context(to)); bdrv_drained_begin(from); /* Put all parents into @list and calculate their cumulative permissions */ diff --git a/blockdev.c b/blockdev.c index ea89896f2712..fa8630cb412d 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1470,9 +1470,7 @@ static void external_snapshot_prepare(BlkActionState *common, DO_UPCAST(ExternalSnapshotState, common, common); TransactionAction *action = common->action; AioContext *aio_context; - AioContext *old_context; uint64_t perm, shared; - int ret; /* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar * purpose but a different set of parameters */ @@ -1608,20 +1606,6 @@ static void external_snapshot_prepare(BlkActionState *common, goto out; } - /* Honor bdrv_try_set_aio_context() context acquisition requirements. */ - old_context = bdrv_get_aio_context(state->new_bs); - aio_context_release(aio_context); - aio_context_acquire(old_context); - - ret = bdrv_try_set_aio_context(state->new_bs, aio_context, errp); - - aio_context_release(old_context); - aio_context_acquire(aio_context); - - if (ret < 0) { - goto out; - } - /* 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. */ From 6a5f6403a11307794ec79d277a065c137cfc12b2 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 10 Mar 2020 12:38:30 +0100 Subject: [PATCH 07/13] iotests: Add iothread cases to 155 This patch adds test cases for attaching the backing chain to a mirror job target right before finalising the job, where the image is in a non-mainloop AioContext (i.e. the backing chain needs to be moved to the AioContext of the mirror target). This requires switching the test case from virtio-blk to virtio-scsi because virtio-blk only actually starts using the iothreads when the guest driver initialises the device (which never happens in a test case without a guest OS). virtio-scsi always keeps its block nodes in the AioContext of the the requested iothread without guest interaction. Signed-off-by: Kevin Wolf Message-Id: <20200310113831.27293-7-kwolf@redhat.com> Reviewed-by: Peter Krempa Signed-off-by: Kevin Wolf --- tests/qemu-iotests/155 | 32 +++++++++++++++++++++++--------- tests/qemu-iotests/155.out | 4 ++-- 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/tests/qemu-iotests/155 b/tests/qemu-iotests/155 index 74ddefc849f5..571bce9de460 100755 --- a/tests/qemu-iotests/155 +++ b/tests/qemu-iotests/155 @@ -49,11 +49,14 @@ target_img = os.path.join(iotests.test_dir, 'target.' + iotests.imgfmt) # chain opened right away. If False, blockdev-add # opens it without a backing file and job completion # is supposed to open the backing chain. +# use_iothread: If True, an iothread is configured for the virtio-blk device +# that uses the image being mirrored class BaseClass(iotests.QMPTestCase): target_blockdev_backing = None target_real_backing = None target_open_with_backing = True + use_iothread = False def setUp(self): qemu_img('create', '-f', iotests.imgfmt, back0_img, '1440K') @@ -69,7 +72,16 @@ class BaseClass(iotests.QMPTestCase): 'file': {'driver': 'file', 'filename': source_img}} self.vm.add_blockdev(self.vm.qmp_to_opts(blockdev)) - self.vm.add_device('virtio-blk,id=qdev0,drive=source') + + if self.use_iothread: + self.vm.add_object('iothread,id=iothread0') + iothread = ",iothread=iothread0" + else: + iothread = "" + + self.vm.add_device('virtio-scsi%s' % iothread) + self.vm.add_device('scsi-hd,id=qdev0,drive=source') + self.vm.launch() self.assertIntactSourceBackingChain() @@ -182,24 +194,21 @@ class MirrorBaseClass(BaseClass): def testFull(self): self.runMirror('full') - node = self.findBlockNode('target', - '/machine/peripheral/qdev0/virtio-backend') + node = self.findBlockNode('target', 'qdev0') self.assertCorrectBackingImage(node, None) self.assertIntactSourceBackingChain() def testTop(self): self.runMirror('top') - node = self.findBlockNode('target', - '/machine/peripheral/qdev0/virtio-backend') + node = self.findBlockNode('target', 'qdev0') self.assertCorrectBackingImage(node, back2_img) self.assertIntactSourceBackingChain() def testNone(self): self.runMirror('none') - node = self.findBlockNode('target', - '/machine/peripheral/qdev0/virtio-backend') + node = self.findBlockNode('target', 'qdev0') self.assertCorrectBackingImage(node, source_img) self.assertIntactSourceBackingChain() @@ -252,6 +261,9 @@ class TestBlockdevMirrorReopen(MirrorBaseClass): backing="backing") self.assert_qmp(result, 'return', {}) +class TestBlockdevMirrorReopenIothread(TestBlockdevMirrorReopen): + use_iothread = True + # Attach the backing chain only during completion, with blockdev-snapshot class TestBlockdevMirrorSnapshot(MirrorBaseClass): cmd = 'blockdev-mirror' @@ -268,6 +280,9 @@ class TestBlockdevMirrorSnapshot(MirrorBaseClass): overlay="target") self.assert_qmp(result, 'return', {}) +class TestBlockdevMirrorSnapshotIothread(TestBlockdevMirrorSnapshot): + use_iothread = True + class TestCommit(BaseClass): existing = False @@ -283,8 +298,7 @@ class TestCommit(BaseClass): self.vm.event_wait('BLOCK_JOB_COMPLETED') - node = self.findBlockNode(None, - '/machine/peripheral/qdev0/virtio-backend') + node = self.findBlockNode(None, 'qdev0') self.assert_qmp(node, 'image' + '/backing-image' * 0 + '/filename', back1_img) self.assert_qmp(node, 'image' + '/backing-image' * 1 + '/filename', diff --git a/tests/qemu-iotests/155.out b/tests/qemu-iotests/155.out index 4fd1c2dcd218..ed714d5263bc 100644 --- a/tests/qemu-iotests/155.out +++ b/tests/qemu-iotests/155.out @@ -1,5 +1,5 @@ -......................... +............................... ---------------------------------------------------------------------- -Ran 25 tests +Ran 31 tests OK From c6bdc312f30d5c7326aa2fdca3e0f98c15eb541a Mon Sep 17 00:00:00 2001 From: Peter Krempa Date: Tue, 10 Mar 2020 12:38:31 +0100 Subject: [PATCH 08/13] qapi: Add '@allow-write-only-overlay' feature for 'blockdev-snapshot' Anounce that 'blockdev-snapshot' command's permissions allow changing of the backing file if the 'consistent_read' permission is not required. This is useful for libvirt to allow late opening of the backing chain during a blockdev-mirror. Signed-off-by: Peter Krempa Signed-off-by: Kevin Wolf Message-Id: <20200310113831.27293-8-kwolf@redhat.com> Signed-off-by: Kevin Wolf --- qapi/block-core.json | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/qapi/block-core.json b/qapi/block-core.json index 9758fc48d230..91586fb1fb89 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1472,6 +1472,12 @@ # # For the arguments, see the documentation of BlockdevSnapshot. # +# Features: +# @allow-write-only-overlay: If present, the check whether this operation is safe +# was relaxed so that it can be used to change +# backing file of a destination of a blockdev-mirror. +# (since 5.0) +# # Since: 2.5 # # Example: @@ -1492,7 +1498,8 @@ # ## { 'command': 'blockdev-snapshot', - 'data': 'BlockdevSnapshot' } + 'data': 'BlockdevSnapshot', + 'features': [ 'allow-write-only-overlay' ] } ## # @change-backing-file: From 6e1da96b946a1a38d3517564af4e2f572c3ccf4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Fri, 6 Mar 2020 17:57:51 +0100 Subject: [PATCH 09/13] tests/qemu-iotests: Fix socket_scm_helper build path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The socket_scm_helper path got corrupted during the mechanical refactor moving the qtests files into their own sub-directory. Fixes: 1e8a1fae7 ("test: Move qtests to a separate directory") Signed-off-by: Philippe Mathieu-Daudé Message-Id: <20200306165751.18986-1-philmd@redhat.com> Reviewed-by: Laurent Vivier Signed-off-by: Kevin Wolf --- tests/Makefile.include | 1 + tests/qtest/Makefile.include | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Makefile.include b/tests/Makefile.include index edcbd475aa70..67e8fcdddac5 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -589,6 +589,7 @@ include $(SRC_PATH)/tests/qtest/Makefile.include tests/test-qga$(EXESUF): qemu-ga$(EXESUF) tests/test-qga$(EXESUF): tests/test-qga.o $(qtest-obj-y) tests/vhost-user-bridge$(EXESUF): tests/vhost-user-bridge.o $(test-util-obj-y) libvhost-user.a +tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_helper.o SPEED = quick diff --git a/tests/qtest/Makefile.include b/tests/qtest/Makefile.include index 383b0ab21710..76672990a740 100644 --- a/tests/qtest/Makefile.include +++ b/tests/qtest/Makefile.include @@ -287,7 +287,6 @@ tests/qtest/usb-hcd-ehci-test$(EXESUF): tests/qtest/usb-hcd-ehci-test.o $(libqos tests/qtest/usb-hcd-xhci-test$(EXESUF): tests/qtest/usb-hcd-xhci-test.o $(libqos-usb-obj-y) tests/qtest/cpu-plug-test$(EXESUF): tests/qtest/cpu-plug-test.o tests/qtest/migration-test$(EXESUF): tests/qtest/migration-test.o tests/qtest/migration-helpers.o -tests/qtest/qemu-iotests/qtest/socket_scm_helper$(EXESUF): tests/qtest/qemu-iotests/qtest/socket_scm_helper.o tests/qtest/test-netfilter$(EXESUF): tests/qtest/test-netfilter.o $(qtest-obj-y) tests/qtest/test-filter-mirror$(EXESUF): tests/qtest/test-filter-mirror.o $(qtest-obj-y) tests/qtest/test-filter-redirector$(EXESUF): tests/qtest/test-filter-redirector.o $(qtest-obj-y) From 9bffae14df879255329473a7bd578643af2d4c9c Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Thu, 30 Jan 2020 18:39:04 -0300 Subject: [PATCH 10/13] block: introducing 'bdrv_co_delete_file' interface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding to Block Drivers the capability of being able to clean up its created files can be useful in certain situations. For the LUKS driver, for instance, a failure in one of its authentication steps can leave files in the host that weren't there before. This patch adds the 'bdrv_co_delete_file' interface to block drivers and add it to the 'file' driver in file-posix.c. The implementation is given by 'raw_co_delete_file'. Suggested-by: Daniel P. Berrangé Signed-off-by: Daniel Henrique Barboza Message-Id: <20200130213907.2830642-2-danielhb413@gmail.com> Signed-off-by: Kevin Wolf --- block/file-posix.c | 23 +++++++++++++++++++++++ include/block/block_int.h | 4 ++++ 2 files changed, 27 insertions(+) diff --git a/block/file-posix.c b/block/file-posix.c index 0f77447a25df..9bc3838b2a4f 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -2445,6 +2445,28 @@ static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts, return raw_co_create(&options, errp); } +static int coroutine_fn raw_co_delete_file(BlockDriverState *bs, + Error **errp) +{ + struct stat st; + int ret; + + if (!(stat(bs->filename, &st) == 0) || !S_ISREG(st.st_mode)) { + error_setg_errno(errp, ENOENT, "%s is not a regular file", + bs->filename); + return -ENOENT; + } + + ret = unlink(bs->filename); + if (ret < 0) { + ret = -errno; + error_setg_errno(errp, -ret, "Error when deleting file %s", + bs->filename); + } + + return ret; +} + /* * Find allocation range in @bs around offset @start. * May change underlying file descriptor's file offset. @@ -3075,6 +3097,7 @@ BlockDriver bdrv_file = { .bdrv_co_block_status = raw_co_block_status, .bdrv_co_invalidate_cache = raw_co_invalidate_cache, .bdrv_co_pwrite_zeroes = raw_co_pwrite_zeroes, + .bdrv_co_delete_file = raw_co_delete_file, .bdrv_co_preadv = raw_co_preadv, .bdrv_co_pwritev = raw_co_pwritev, diff --git a/include/block/block_int.h b/include/block/block_int.h index d8d13700a966..ae9c4da4d083 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -314,6 +314,10 @@ struct BlockDriver { */ int coroutine_fn (*bdrv_co_flush)(BlockDriverState *bs); + /* Delete a created file. */ + int coroutine_fn (*bdrv_co_delete_file)(BlockDriverState *bs, + Error **errp); + /* * Flushes all data that was already written to the OS all the way down to * the disk (for example file-posix.c calls fsync()). From e1d7f8bb1ec0c6911dcea81641ce6139dbded02d Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Thu, 30 Jan 2020 18:39:05 -0300 Subject: [PATCH 11/13] block.c: adding bdrv_co_delete_file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Using the new 'bdrv_co_delete_file' interface, a pure co_routine function 'bdrv_co_delete_file' inside block.c can can be used in a way similar of the existing bdrv_create_file to to clean up a created file. We're creating a pure co_routine because the only caller of 'bdrv_co_delete_file' will be already in co_routine context, thus there is no need to add all the machinery to check for qemu_in_coroutine() and create a separated co_routine to do the job. Suggested-by: Daniel P. Berrangé Signed-off-by: Daniel Henrique Barboza Message-Id: <20200130213907.2830642-3-danielhb413@gmail.com> Signed-off-by: Kevin Wolf --- block.c | 26 ++++++++++++++++++++++++++ include/block/block.h | 1 + 2 files changed, 27 insertions(+) diff --git a/block.c b/block.c index 8fc7b56937d5..a2542c977b91 100644 --- a/block.c +++ b/block.c @@ -668,6 +668,32 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp) } } +int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, Error **errp) +{ + Error *local_err = NULL; + int ret; + + assert(bs != NULL); + + if (!bs->drv) { + error_setg(errp, "Block node '%s' is not opened", bs->filename); + return -ENOMEDIUM; + } + + if (!bs->drv->bdrv_co_delete_file) { + error_setg(errp, "Driver '%s' does not support image deletion", + bs->drv->format_name); + return -ENOTSUP; + } + + ret = bs->drv->bdrv_co_delete_file(bs, &local_err); + if (ret < 0) { + error_propagate(errp, local_err); + } + + return ret; +} + /** * Try to get @bs's logical and physical block size. * On success, store them in @bsz struct and return 0. diff --git a/include/block/block.h b/include/block/block.h index cd6b5b95aad2..e569a4d747a0 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -363,6 +363,7 @@ bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base, int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base, Error **errp); void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base); +int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, Error **errp); typedef struct BdrvCheckResult { From 1bba30da24e1124ceeb0693c81382a0d77e20ca5 Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Thu, 30 Jan 2020 18:39:06 -0300 Subject: [PATCH 12/13] crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails When using a non-UTF8 secret to create a volume using qemu-img, the following error happens: $ qemu-img create -f luks --object secret,id=vol_1_encrypt0,file=vol_resize_pool.vol_1.secret.qzVQrI -o key-secret=vol_1_encrypt0 /var/tmp/pool_target/vol_1 10240K Formatting '/var/tmp/pool_target/vol_1', fmt=luks size=10485760 key-secret=vol_1_encrypt0 qemu-img: /var/tmp/pool_target/vol_1: Data from secret vol_1_encrypt0 is not valid UTF-8 However, the created file '/var/tmp/pool_target/vol_1' is left behind in the file system after the failure. This behavior can be observed when creating the volume using Libvirt, via 'virsh vol-create', and then getting "volume target path already exist" errors when trying to re-create the volume. The volume file is created inside block_crypto_co_create_opts_luks(), in block/crypto.c. If the bdrv_create_file() call is successful but any succeeding step fails*, the existing 'fail' label does not take into account the created file, leaving it behind. This patch changes block_crypto_co_create_opts_luks() to delete 'filename' in case of failure. A failure in this point means that the volume is now truncated/corrupted, so even if 'filename' was an existing volume before calling qemu-img, it is now unusable. Deleting the file it is not much worse than leaving it in the filesystem in this scenario, and we don't have to deal with checking the file pre-existence in the code. * in our case, block_crypto_co_create_generic calls qcrypto_block_create, which calls qcrypto_block_luks_create, and this function fails when calling qcrypto_secret_lookup_as_utf8. Reported-by: Srikanth Aithal Suggested-by: Kevin Wolf Signed-off-by: Daniel Henrique Barboza Message-Id: <20200130213907.2830642-4-danielhb413@gmail.com> Signed-off-by: Kevin Wolf --- block/crypto.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/block/crypto.c b/block/crypto.c index 24823835c1c2..00e8ec537db1 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -30,6 +30,7 @@ #include "qapi/error.h" #include "qemu/module.h" #include "qemu/option.h" +#include "qemu/cutils.h" #include "crypto.h" typedef struct BlockCrypto BlockCrypto; @@ -596,6 +597,23 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename, ret = 0; fail: + /* + * If an error occurred, delete 'filename'. Even if the file existed + * beforehand, it has been truncated and corrupted in the process. + */ + if (ret && bs) { + Error *local_delete_err = NULL; + int r_del = bdrv_co_delete_file(bs, &local_delete_err); + /* + * ENOTSUP will happen if the block driver doesn't support + * the 'bdrv_co_delete_file' interface. This is a predictable + * scenario and shouldn't be reported back to the user. + */ + if ((r_del < 0) && (r_del != -ENOTSUP)) { + error_report_err(local_delete_err); + } + } + bdrv_unref(bs); qapi_free_QCryptoBlockCreateOptions(create_opts); qobject_unref(cryptoopts); From 8bb3b023f2055054ee119cb45b42d2b14be7fc8a Mon Sep 17 00:00:00 2001 From: Daniel Henrique Barboza Date: Thu, 30 Jan 2020 18:39:07 -0300 Subject: [PATCH 13/13] qemu-iotests: adding LUKS cleanup for non-UTF8 secret error This patch adds a new test file to exercise the case where qemu-img fails to complete for the LUKS format when a non-UTF8 secret is used. Signed-off-by: Daniel Henrique Barboza Message-Id: <20200130213907.2830642-5-danielhb413@gmail.com> Signed-off-by: Kevin Wolf --- tests/qemu-iotests/282 | 67 ++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/282.out | 11 +++++++ tests/qemu-iotests/group | 1 + 3 files changed, 79 insertions(+) create mode 100755 tests/qemu-iotests/282 create mode 100644 tests/qemu-iotests/282.out diff --git a/tests/qemu-iotests/282 b/tests/qemu-iotests/282 new file mode 100755 index 000000000000..081eb1208021 --- /dev/null +++ b/tests/qemu-iotests/282 @@ -0,0 +1,67 @@ +#!/usr/bin/env bash +# +# Test qemu-img file cleanup for LUKS when using a non-UTF8 secret +# +# Copyright (C) 2020, IBM Corporation. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +# + +seq=`basename $0` +echo "QA output created by $seq" + +status=1 # failure is the default! +TEST_IMAGE_FILE='vol.img' + +_cleanup() +{ + _cleanup_test_img + rm non_utf8_secret + rm -f $TEST_IMAGE_FILE +} +trap "_cleanup; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter + +_supported_fmt luks +_supported_proto generic +_unsupported_proto vxhs + +echo "== Create non-UTF8 secret ==" +echo -n -e '\x3a\x3c\x3b\xff' > non_utf8_secret +SECRET="secret,id=sec0,file=non_utf8_secret" + +echo "== Throws an error because of invalid UTF-8 secret ==" +$QEMU_IMG create -f $IMGFMT --object $SECRET -o "key-secret=sec0" $TEST_IMAGE_FILE 4M + +echo "== Image file should not exist after the error ==" +if test -f "$TEST_IMAGE_FILE"; then + exit 1 +fi + +echo "== Create a stub image file and run qemu-img again ==" +touch $TEST_IMAGE_FILE +$QEMU_IMG create -f $IMGFMT --object $SECRET -o "key-secret=sec0" $TEST_IMAGE_FILE 4M + +echo "== Pre-existing image file should also be deleted after the error ==" +if test -f "$TEST_IMAGE_FILE"; then + exit 1 +fi + +# success, all done +echo "*** done" +rm -f $seq.full +status=0 diff --git a/tests/qemu-iotests/282.out b/tests/qemu-iotests/282.out new file mode 100644 index 000000000000..5d079dabce1d --- /dev/null +++ b/tests/qemu-iotests/282.out @@ -0,0 +1,11 @@ +QA output created by 282 +== Create non-UTF8 secret == +== Throws an error because of invalid UTF-8 secret == +qemu-img: vol.img: Data from secret sec0 is not valid UTF-8 +Formatting 'vol.img', fmt=luks size=4194304 key-secret=sec0 +== Image file should not exist after the error == +== Create a stub image file and run qemu-img again == +qemu-img: vol.img: Data from secret sec0 is not valid UTF-8 +Formatting 'vol.img', fmt=luks size=4194304 key-secret=sec0 +== Pre-existing image file should also be deleted after the error == + *** done diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index 031766769571..3c1329b081e4 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -290,6 +290,7 @@ 279 rw backing quick 280 rw migration quick 281 rw quick +282 rw img quick 283 auto quick 284 rw 286 rw quick