From 2d369d6e6e890a0204183e853604f8077329c4bc Mon Sep 17 00:00:00 2001 From: Miroslav Rezanina Date: Wed, 5 May 2021 03:59:03 -0400 Subject: [PATCH 01/24] Prevent compiler warning on block.c Commit 3108a15cf (block: introduce bdrv_drop_filter()) introduced uninitialized variable to_cow_parent in bdrv_replace_node_common function that is used only when detach_subchain is true. It is used in two places. First if block properly initialize the variable and second block use it. However, compiler may treat these two blocks as two independent cases so it thinks first block can fail test and second one pass (although both use same condition). This cause warning that variable can be uninitialized in second block. The warning was observed with GCC 8.4.1 and 11.0.1. To prevent this warning, initialize the variable with NULL. Signed-off-by: Miroslav Rezanina Message-Id: <1162368493.17178530.1620201543649.JavaMail.zimbra@redhat.com> Signed-off-by: Kevin Wolf --- block.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block.c b/block.c index 1d37f133a8..3e277855e7 100644 --- a/block.c +++ b/block.c @@ -4866,7 +4866,7 @@ static int bdrv_replace_node_common(BlockDriverState *from, Transaction *tran = tran_new(); g_autoptr(GHashTable) found = NULL; g_autoptr(GSList) refresh_list = NULL; - BlockDriverState *to_cow_parent; + BlockDriverState *to_cow_parent = NULL; int ret; if (detach_subchain) { From 97efa8698e554769fc23e8120fe9c56ab45cddc5 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 9 Jun 2021 11:30:34 -0500 Subject: [PATCH 02/24] block: Move read-only check during truncation earlier No need to start a tracked request that will always fail. The choice to check read-only after bdrv_inc_in_flight() predates 1bc5f09f2e (block: Use tracked request for truncate), but waiting for serializing requests can make the effect more noticeable. Signed-off-by: Eric Blake Message-Id: <20210609163034.997943-1-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Kevin Wolf --- block/io.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/block/io.c b/block/io.c index dd93364258..cf177a9d2d 100644 --- a/block/io.c +++ b/block/io.c @@ -3392,6 +3392,11 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact, return old_size; } + if (bdrv_is_read_only(bs)) { + error_setg(errp, "Image is read-only"); + return -EACCES; + } + if (offset > old_size) { new_bytes = offset - old_size; } else { @@ -3408,11 +3413,6 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact, if (new_bytes) { bdrv_make_request_serialising(&req, 1); } - if (bdrv_is_read_only(bs)) { - error_setg(errp, "Image is read-only"); - ret = -EACCES; - goto out; - } ret = bdrv_co_write_req_prepare(child, offset - new_bytes, new_bytes, &req, 0); if (ret < 0) { From d5b23994586934f18853684307bc01965bcdad4b Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Tue, 22 Jun 2021 16:00:30 +0200 Subject: [PATCH 03/24] block: BDRV_O_NO_IO for backing file on creation When creating an image file with a backing file, we generally try to open the backing file (unless -u was specified), mostly to verify that it is there, but also to get the file size if none was specified for the new image. For neither of these things do we need data I/O, and so we can pass BDRV_O_NO_IO when opening the backing file. This allows us to open even encrypted backing images without requiring the user to provide a secret. This makes the -u switch in iotests 189 and 198 unnecessary (and the $size parameter), so drop it, because this way we get regression tests for this patch here. Fixes: https://gitlab.com/qemu-project/qemu/-/issues/441 Signed-off-by: Max Reitz Message-Id: <20210622140030.212487-1-mreitz@redhat.com> Signed-off-by: Kevin Wolf --- block.c | 6 +++++- tests/qemu-iotests/189 | 2 +- tests/qemu-iotests/198 | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/block.c b/block.c index 3e277855e7..a2a4a0dd80 100644 --- a/block.c +++ b/block.c @@ -6553,9 +6553,13 @@ void bdrv_img_create(const char *filename, const char *fmt, } assert(full_backing); - /* backing files always opened read-only */ + /* + * No need to do I/O here, which allows us to open encrypted + * backing images without needing the secret + */ back_flags = flags; back_flags &= ~(BDRV_O_RDWR | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING); + back_flags |= BDRV_O_NO_IO; backing_options = qdict_new(); if (backing_fmt) { diff --git a/tests/qemu-iotests/189 b/tests/qemu-iotests/189 index 4e463385b2..801494c6b9 100755 --- a/tests/qemu-iotests/189 +++ b/tests/qemu-iotests/189 @@ -67,7 +67,7 @@ echo "== verify pattern ==" $QEMU_IO --object $SECRET0 -c "read -P 0xa 0 $size" --image-opts $IMGSPECBASE | _filter_qemu_io | _filter_testdir echo "== create overlay ==" -_make_test_img --object $SECRET1 -o "encrypt.format=luks,encrypt.key-secret=sec1,encrypt.iter-time=10" -u -b "$TEST_IMG_BASE" -F $IMGFMT $size +_make_test_img --object $SECRET1 -o "encrypt.format=luks,encrypt.key-secret=sec1,encrypt.iter-time=10" -b "$TEST_IMG_BASE" -F $IMGFMT echo echo "== writing part of a cluster ==" diff --git a/tests/qemu-iotests/198 b/tests/qemu-iotests/198 index b333a8f281..1c93dea1f7 100755 --- a/tests/qemu-iotests/198 +++ b/tests/qemu-iotests/198 @@ -64,7 +64,7 @@ echo "== writing whole image base ==" $QEMU_IO --object $SECRET0 -c "write -P 0xa 0 $size" --image-opts $IMGSPECBASE | _filter_qemu_io | _filter_testdir echo "== create overlay ==" -_make_test_img --object $SECRET1 -o "encrypt.format=luks,encrypt.key-secret=sec1,encrypt.iter-time=10" -u -b "$TEST_IMG_BASE" -F $IMGFMT $size +_make_test_img --object $SECRET1 -o "encrypt.format=luks,encrypt.key-secret=sec1,encrypt.iter-time=10" -b "$TEST_IMG_BASE" -F $IMGFMT echo echo "== writing whole image layer ==" From 4bf021dbd5d4da22058d425dceae93ce11ff3700 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Thu, 10 Jun 2021 14:25:44 +0300 Subject: [PATCH 04/24] block: rename bdrv_replace_child to bdrv_replace_child_tran We have bdrv_replace_child() wrapper on bdrv_replace_child_noperm(). But bdrv_replace_child() doesn't update permissions. It's rather strange, as normally it's expected that foo() should call foo_noperm() and update permissions. Let's rename and add comment. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Max Reitz Message-Id: <20210610112618.127378-2-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf --- block.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/block.c b/block.c index a2a4a0dd80..e49ff8c078 100644 --- a/block.c +++ b/block.c @@ -2249,12 +2249,14 @@ static TransactionActionDrv bdrv_replace_child_drv = { }; /* - * bdrv_replace_child + * bdrv_replace_child_tran * * Note: real unref of old_bs is done only on commit. + * + * The function doesn't update permissions, caller is responsible for this. */ -static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs, - Transaction *tran) +static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState *new_bs, + Transaction *tran) { BdrvReplaceChildState *s = g_new(BdrvReplaceChildState, 1); *s = (BdrvReplaceChildState) { @@ -4766,7 +4768,7 @@ static void bdrv_remove_filter_or_cow_child_abort(void *opaque) } /* - * We don't have to restore child->bs here to undo bdrv_replace_child() + * We don't have to restore child->bs here to undo bdrv_replace_child_tran() * because that function is transactionable and it registered own completion * entries in @tran, so .abort() for bdrv_replace_child_safe() will be * called automatically. @@ -4802,7 +4804,7 @@ static void bdrv_remove_filter_or_cow_child(BlockDriverState *bs, } if (child->bs) { - bdrv_replace_child(child, NULL, tran); + bdrv_replace_child_tran(child, NULL, tran); } s = g_new(BdrvRemoveFilterOrCowChild, 1); @@ -4842,7 +4844,7 @@ static int bdrv_replace_node_noperm(BlockDriverState *from, c->name, from->node_name); return -EPERM; } - bdrv_replace_child(c, to, tran); + bdrv_replace_child_tran(c, to, tran); } return 0; From 7ec390d587c32fa37c80099bd668ededd02309e0 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Thu, 10 Jun 2021 14:25:45 +0300 Subject: [PATCH 05/24] block: comment graph-modifying function not updating permissions Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Max Reitz Message-Id: <20210610112618.127378-3-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf --- block.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/block.c b/block.c index e49ff8c078..9518f0e087 100644 --- a/block.c +++ b/block.c @@ -2770,6 +2770,8 @@ static TransactionActionDrv bdrv_attach_child_common_drv = { * @child is saved to a new entry of @tran, so that *@child could be reverted to * NULL on abort(). So referenced variable must live at least until transaction * end. + * + * Function doesn't update permissions, caller is responsible for this. */ static int bdrv_attach_child_common(BlockDriverState *child_bs, const char *child_name, @@ -2848,6 +2850,8 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs, /* * Variable referenced by @child must live at least until transaction end. * (see bdrv_attach_child_common() doc for details) + * + * Function doesn't update permissions, caller is responsible for this. */ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs, BlockDriverState *child_bs, @@ -3115,6 +3119,8 @@ static BdrvChildRole bdrv_backing_role(BlockDriverState *bs) /* * Sets the bs->backing link of a BDS. A new reference is created; callers * which don't need their own reference any more must call bdrv_unref(). + * + * Function doesn't update permissions, caller is responsible for this. */ static int bdrv_set_backing_noperm(BlockDriverState *bs, BlockDriverState *backing_hd, @@ -4792,6 +4798,8 @@ static TransactionActionDrv bdrv_remove_filter_or_cow_child_drv = { * A function to remove backing-chain child of @bs if exists: cow child for * format nodes (always .backing) and filter child for filters (may be .file or * .backing) + * + * Function doesn't update permissions, caller is responsible for this. */ static void bdrv_remove_filter_or_cow_child(BlockDriverState *bs, Transaction *tran) From 5b9950193bca788e4f01b98002f326cdd6a21e6c Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Thu, 10 Jun 2021 15:05:29 +0300 Subject: [PATCH 06/24] block: introduce bdrv_remove_file_or_backing_child() To be used for reopen in future commit. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210610120537.196183-2-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf --- block.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/block.c b/block.c index 9518f0e087..7eceef0afa 100644 --- a/block.c +++ b/block.c @@ -4795,17 +4795,16 @@ static TransactionActionDrv bdrv_remove_filter_or_cow_child_drv = { }; /* - * A function to remove backing-chain child of @bs if exists: cow child for - * format nodes (always .backing) and filter child for filters (may be .file or - * .backing) - * + * A function to remove backing or file child of @bs. * Function doesn't update permissions, caller is responsible for this. */ -static void bdrv_remove_filter_or_cow_child(BlockDriverState *bs, - Transaction *tran) +static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, + BdrvChild *child, + Transaction *tran) { BdrvRemoveFilterOrCowChild *s; - BdrvChild *child = bdrv_filter_or_cow_child(bs); + + assert(child == bs->backing || child == bs->file); if (!child) { return; @@ -4830,6 +4829,17 @@ static void bdrv_remove_filter_or_cow_child(BlockDriverState *bs, } } +/* + * A function to remove backing-chain child of @bs if exists: cow child for + * format nodes (always .backing) and filter child for filters (may be .file or + * .backing) + */ +static void bdrv_remove_filter_or_cow_child(BlockDriverState *bs, + Transaction *tran) +{ + bdrv_remove_file_or_backing_child(bs, bdrv_filter_or_cow_child(bs), tran); +} + static int bdrv_replace_node_noperm(BlockDriverState *from, BlockDriverState *to, bool auto_skip, Transaction *tran, From e9238278c2272fd3c6a1d339303e3ee2a2e2fbd3 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Thu, 10 Jun 2021 15:05:30 +0300 Subject: [PATCH 07/24] block: introduce bdrv_set_file_or_backing_noperm() To be used for reopen in future commit. Notes: - It seems OK to update inherits_from if new bs is recursively inherits from parent bs. Let's just not check for backing_chain_contains, to support file child of non-filters. - Simply check child->frozen instead of bdrv_is_backing_chain_frozen(), as we really interested only in this one child. - Role determination of new child is a bit more complex: it remains the same for backing child, it's obvious for filter driver. But for non-filter file child let's for now restrict to only replacing existing child (and keeping its role). Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210610120537.196183-3-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf --- block.c | 83 +++++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 63 insertions(+), 20 deletions(-) diff --git a/block.c b/block.c index 7eceef0afa..14711ce7ab 100644 --- a/block.c +++ b/block.c @@ -84,6 +84,9 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, static void bdrv_replace_child_noperm(BdrvChild *child, BlockDriverState *new_bs); +static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, + BdrvChild *child, + Transaction *tran); static void bdrv_remove_filter_or_cow_child(BlockDriverState *bs, Transaction *tran); @@ -3117,56 +3120,96 @@ static BdrvChildRole bdrv_backing_role(BlockDriverState *bs) } /* - * Sets the bs->backing link of a BDS. A new reference is created; callers - * which don't need their own reference any more must call bdrv_unref(). + * Sets the bs->backing or bs->file link of a BDS. A new reference is created; + * callers which don't need their own reference any more must call bdrv_unref(). * * Function doesn't update permissions, caller is responsible for this. */ -static int bdrv_set_backing_noperm(BlockDriverState *bs, - BlockDriverState *backing_hd, - Transaction *tran, Error **errp) +static int bdrv_set_file_or_backing_noperm(BlockDriverState *parent_bs, + BlockDriverState *child_bs, + bool is_backing, + Transaction *tran, Error **errp) { int ret = 0; - bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) && - bdrv_inherits_from_recursive(backing_hd, bs); + bool update_inherits_from = + bdrv_inherits_from_recursive(child_bs, parent_bs); + BdrvChild *child = is_backing ? parent_bs->backing : parent_bs->file; + BdrvChildRole role; - if (bdrv_is_backing_chain_frozen(bs, child_bs(bs->backing), errp)) { + if (!parent_bs->drv) { + /* + * Node without drv is an object without a class :/. TODO: finally fix + * qcow2 driver to never clear bs->drv and implement format corruption + * handling in other way. + */ + error_setg(errp, "Node corrupted"); + return -EINVAL; + } + + if (child && child->frozen) { + error_setg(errp, "Cannot change frozen '%s' link from '%s' to '%s'", + child->name, parent_bs->node_name, child->bs->node_name); return -EPERM; } - if (bs->backing) { - /* Cannot be frozen, we checked that above */ - bdrv_unset_inherits_from(bs, bs->backing, tran); - bdrv_remove_filter_or_cow_child(bs, tran); + if (parent_bs->drv->is_filter) { + role = BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY; + } else if (is_backing) { + role = BDRV_CHILD_COW; + } else { + /* + * We only can use same role as it is in existing child. We don't have + * infrastructure to determine role of file child in generic way + */ + if (!child) { + error_setg(errp, "Cannot set file child to format node without " + "file child"); + return -EINVAL; + } + role = child->role; } - if (!backing_hd) { + if (child) { + bdrv_unset_inherits_from(parent_bs, child, tran); + bdrv_remove_file_or_backing_child(parent_bs, child, tran); + } + + if (!child_bs) { goto out; } - ret = bdrv_attach_child_noperm(bs, backing_hd, "backing", - &child_of_bds, bdrv_backing_role(bs), - &bs->backing, tran, errp); + ret = bdrv_attach_child_noperm(parent_bs, child_bs, + is_backing ? "backing" : "file", + &child_of_bds, role, + is_backing ? &parent_bs->backing : + &parent_bs->file, + tran, errp); if (ret < 0) { return ret; } /* - * If backing_hd was already part of bs's backing chain, and - * inherits_from pointed recursively to bs then let's update it to + * If inherits_from pointed recursively to bs then let's update it to * point directly to bs (else it will become NULL). */ if (update_inherits_from) { - bdrv_set_inherits_from(backing_hd, bs, tran); + bdrv_set_inherits_from(child_bs, parent_bs, tran); } out: - bdrv_refresh_limits(bs, tran, NULL); + bdrv_refresh_limits(parent_bs, tran, NULL); return 0; } +static int bdrv_set_backing_noperm(BlockDriverState *bs, + BlockDriverState *backing_hd, + Transaction *tran, Error **errp) +{ + return bdrv_set_file_or_backing_noperm(bs, backing_hd, true, tran, errp); +} + int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd, Error **errp) { From fd26b8a089962fa7ac1cbd44e138fbef4b484271 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Thu, 10 Jun 2021 15:05:31 +0300 Subject: [PATCH 08/24] block: bdrv_reopen_parse_backing(): don't check aio context We don't need this check: bdrv_set_backing_noperm() will do it anyway (actually in bdrv_attach_child_common()). Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210610120537.196183-4-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf --- block.c | 33 --------------------------------- 1 file changed, 33 deletions(-) diff --git a/block.c b/block.c index 14711ce7ab..69dc9d188d 100644 --- a/block.c +++ b/block.c @@ -4215,29 +4215,6 @@ int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only, return ret; } -static bool bdrv_reopen_can_attach(BlockDriverState *parent, - BdrvChild *child, - BlockDriverState *new_child, - Error **errp) -{ - AioContext *parent_ctx = bdrv_get_aio_context(parent); - AioContext *child_ctx = bdrv_get_aio_context(new_child); - GSList *ignore; - bool ret; - - ignore = g_slist_prepend(NULL, child); - ret = bdrv_can_set_aio_context(new_child, parent_ctx, &ignore, NULL); - g_slist_free(ignore); - if (ret) { - return ret; - } - - ignore = g_slist_prepend(NULL, child); - ret = bdrv_can_set_aio_context(parent, child_ctx, &ignore, errp); - g_slist_free(ignore); - return ret; -} - /* * Take a BDRVReopenState and check if the value of 'backing' in the * reopen_state->options QDict is valid or not. @@ -4289,16 +4266,6 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, g_assert_not_reached(); } - /* - * Check AioContext compatibility so that the bdrv_set_backing_hd() call in - * bdrv_reopen_commit() won't fail. - */ - if (new_backing_bs) { - if (!bdrv_reopen_can_attach(bs, bs->backing, new_backing_bs, errp)) { - return -EINVAL; - } - } - /* * Ensure that @bs can really handle backing files, because we are * about to give it one (or swap the existing one) From bfae052a57d20af91b2fd9fb92469be6283f4820 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Thu, 10 Jun 2021 15:05:32 +0300 Subject: [PATCH 09/24] block: bdrv_reopen_parse_backing(): don't check frozen child bdrv_set_backing_noperm() takes care of it (actual check is in bdrv_set_file_or_backing_noperm()), so we don't need to check it here. While being here, improve error message a bit. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210610120537.196183-5-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf --- block.c | 14 +------------- tests/qemu-iotests/245 | 8 ++++---- 2 files changed, 5 insertions(+), 17 deletions(-) diff --git a/block.c b/block.c index 69dc9d188d..6472866283 100644 --- a/block.c +++ b/block.c @@ -4308,19 +4308,7 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, "an implicit backing file", bs->node_name); return -EPERM; } - /* - * Check if the backing link that we want to replace is frozen. - * Note that - * bdrv_filter_or_cow_child(overlay_bs) == overlay_bs->backing, - * because we know that overlay_bs == bs, and that @bs - * either is a filter that uses ->backing or a COW format BDS - * with bs->drv->supports_backing == true. - */ - if (bdrv_is_backing_chain_frozen(overlay_bs, - child_bs(overlay_bs->backing), errp)) - { - return -EPERM; - } + reopen_state->replace_backing_bs = true; reopen_state->old_backing_bs = bs->backing ? bs->backing->bs : NULL; ret = bdrv_set_backing_noperm(bs, new_backing_bs, set_backings_tran, diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245 index fc5297e268..c7d671366a 100755 --- a/tests/qemu-iotests/245 +++ b/tests/qemu-iotests/245 @@ -878,7 +878,7 @@ class TestBlockdevReopen(iotests.QMPTestCase): # We can't remove hd1 while the stream job is ongoing opts['backing'] = None - self.reopen(opts, {}, "Cannot change 'backing' link from 'hd0' to 'hd1'") + self.reopen(opts, {}, "Cannot change frozen 'backing' link from 'hd0' to 'hd1'") self.vm.run_job('stream0', auto_finalize = False, auto_dismiss = True) @@ -910,7 +910,7 @@ class TestBlockdevReopen(iotests.QMPTestCase): # We can't remove hd2 while the stream job is ongoing opts['backing']['backing'] = None self.reopen(opts['backing'], {'read-only': False}, - "Cannot change 'backing' link from 'hd1' to 'hd2'") + "Cannot change frozen 'backing' link from 'hd1' to 'hd2'") # We can detach hd1 from hd0 because it doesn't affect the stream job opts['backing'] = None @@ -933,11 +933,11 @@ class TestBlockdevReopen(iotests.QMPTestCase): # We can't remove hd2 while the commit job is ongoing opts['backing']['backing'] = None - self.reopen(opts, {}, "Cannot change 'backing' link from 'hd1' to 'hd2'") + self.reopen(opts, {}, "Cannot change frozen 'backing' link from 'hd1' to 'hd2'") # We can't remove hd1 while the commit job is ongoing opts['backing'] = None - self.reopen(opts, {}, "Cannot change 'backing' link from 'hd0' to 'hd1'") + self.reopen(opts, {}, "Cannot change frozen 'backing' link from 'hd0' to 'hd1'") event = self.vm.event_wait(name='BLOCK_JOB_READY') self.assert_qmp(event, 'data/device', 'commit0') From cbfdb98ce21532e6705fdbf324583f99602e183a Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Thu, 10 Jun 2021 15:05:33 +0300 Subject: [PATCH 10/24] block: bdrv_reopen_parse_backing(): simplify handling implicit filters The logic around finding overlay here is not obvious. Actually it does two simple things: 1. If new bs is already in backing chain, split from parent bs by several implicit filters we are done, do nothing. 2. Otherwise, don't try to replace implicit filter. Let's rewrite this in more obvious way. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210610120537.196183-6-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf --- block.c | 53 ++++++++++++++++------------------------------------- 1 file changed, 16 insertions(+), 37 deletions(-) diff --git a/block.c b/block.c index 6472866283..d047cae43c 100644 --- a/block.c +++ b/block.c @@ -4237,7 +4237,7 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, Error **errp) { BlockDriverState *bs = reopen_state->bs; - BlockDriverState *overlay_bs, *below_bs, *new_backing_bs; + BlockDriverState *new_backing_bs; QObject *value; const char *str; @@ -4266,6 +4266,18 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, g_assert_not_reached(); } + if (bs->backing) { + if (bdrv_skip_implicit_filters(bs->backing->bs) == new_backing_bs) { + return 0; + } + + if (bs->backing->bs->implicit) { + error_setg(errp, "Cannot change backing link if '%s' has " + "an implicit backing file", bs->node_name); + return -EPERM; + } + } + /* * Ensure that @bs can really handle backing files, because we are * about to give it one (or swap the existing one) @@ -4283,42 +4295,9 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, return -EINVAL; } - /* - * Find the "actual" backing file by skipping all links that point - * to an implicit node, if any (e.g. a commit filter node). - * We cannot use any of the bdrv_skip_*() functions here because - * those return the first explicit node, while we are looking for - * its overlay here. - */ - overlay_bs = bs; - for (below_bs = bdrv_filter_or_cow_bs(overlay_bs); - below_bs && below_bs->implicit; - below_bs = bdrv_filter_or_cow_bs(overlay_bs)) - { - overlay_bs = below_bs; - } - - /* If we want to replace the backing file we need some extra checks */ - if (new_backing_bs != bdrv_filter_or_cow_bs(overlay_bs)) { - int ret; - - /* Check for implicit nodes between bs and its backing file */ - if (bs != overlay_bs) { - error_setg(errp, "Cannot change backing link if '%s' has " - "an implicit backing file", bs->node_name); - return -EPERM; - } - - reopen_state->replace_backing_bs = true; - reopen_state->old_backing_bs = bs->backing ? bs->backing->bs : NULL; - ret = bdrv_set_backing_noperm(bs, new_backing_bs, set_backings_tran, - errp); - if (ret < 0) { - return ret; - } - } - - return 0; + reopen_state->replace_backing_bs = true; + reopen_state->old_backing_bs = bs->backing ? bs->backing->bs : NULL; + return bdrv_set_backing_noperm(bs, new_backing_bs, set_backings_tran, errp); } /* From 25f78d9e2de528473d52acfcf7acdfb64e3453d4 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Thu, 10 Jun 2021 15:05:34 +0300 Subject: [PATCH 11/24] block: move supports_backing check to bdrv_set_file_or_backing_noperm() Move supports_backing check of bdrv_reopen_parse_backing to called (through bdrv_set_backing_noperm()) bdrv_set_file_or_backing_noperm() function. The check applies to general case, so it's appropriate for bdrv_set_file_or_backing_noperm(). We have to declare backing support for two test drivers, otherwise new check fails. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210610120537.196183-7-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf --- block.c | 29 +++++++++++++++-------------- tests/unit/test-bdrv-drain.c | 1 + tests/unit/test-bdrv-graph-mod.c | 1 + 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/block.c b/block.c index d047cae43c..0a73335380 100644 --- a/block.c +++ b/block.c @@ -3152,6 +3152,14 @@ static int bdrv_set_file_or_backing_noperm(BlockDriverState *parent_bs, return -EPERM; } + if (is_backing && !parent_bs->drv->is_filter && + !parent_bs->drv->supports_backing) + { + error_setg(errp, "Driver '%s' of node '%s' does not support backing " + "files", parent_bs->drv->format_name, parent_bs->node_name); + return -EINVAL; + } + if (parent_bs->drv->is_filter) { role = BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY; } else if (is_backing) { @@ -4278,20 +4286,13 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, } } - /* - * Ensure that @bs can really handle backing files, because we are - * about to give it one (or swap the existing one) - */ - if (bs->drv->is_filter) { - /* Filters always have a file or a backing child */ - if (!bs->backing) { - error_setg(errp, "'%s' is a %s filter node that does not support a " - "backing child", bs->node_name, bs->drv->format_name); - return -EINVAL; - } - } else if (!bs->drv->supports_backing) { - error_setg(errp, "Driver '%s' of node '%s' does not support backing " - "files", bs->drv->format_name, bs->node_name); + if (bs->drv->is_filter && !bs->backing) { + /* + * Filters always have a file or a backing child, so we are trying to + * change wrong child + */ + error_setg(errp, "'%s' is a %s filter node that does not support a " + "backing child", bs->node_name, bs->drv->format_name); return -EINVAL; } diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c index 892f7f47d8..ce071b5fc5 100644 --- a/tests/unit/test-bdrv-drain.c +++ b/tests/unit/test-bdrv-drain.c @@ -95,6 +95,7 @@ static int bdrv_test_change_backing_file(BlockDriverState *bs, static BlockDriver bdrv_test = { .format_name = "test", .instance_size = sizeof(BDRVTestState), + .supports_backing = true, .bdrv_close = bdrv_test_close, .bdrv_co_preadv = bdrv_test_co_preadv, diff --git a/tests/unit/test-bdrv-graph-mod.c b/tests/unit/test-bdrv-graph-mod.c index 88f25c0cdb..a6e3bb79be 100644 --- a/tests/unit/test-bdrv-graph-mod.c +++ b/tests/unit/test-bdrv-graph-mod.c @@ -41,6 +41,7 @@ static void no_perm_default_perms(BlockDriverState *bs, BdrvChild *c, static BlockDriver bdrv_no_perm = { .format_name = "no-perm", + .supports_backing = true, .bdrv_child_perm = no_perm_default_perms, }; From 3d0e8743f0fca85e2d9b98924dcedaa5ab79db4d Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Thu, 10 Jun 2021 15:05:35 +0300 Subject: [PATCH 12/24] block: BDRVReopenState: drop replace_backing_bs field It's used only in bdrv_reopen_commit(). "backing" is covered by the loop through all children except for case when we removed backing child during reopen. Make it more obvious and drop extra boolean field: qdict_del will not fail if there is no such entry. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210610120537.196183-8-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf --- block.c | 10 ++++------ include/block/block.h | 1 - 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/block.c b/block.c index 0a73335380..ca11078cd2 100644 --- a/block.c +++ b/block.c @@ -4296,7 +4296,6 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, return -EINVAL; } - reopen_state->replace_backing_bs = true; reopen_state->old_backing_bs = bs->backing ? bs->backing->bs : NULL; return bdrv_set_backing_noperm(bs, new_backing_bs, set_backings_tran, errp); } @@ -4550,17 +4549,16 @@ static void bdrv_reopen_commit(BDRVReopenState *reopen_state) bs->open_flags = reopen_state->flags; bs->detect_zeroes = reopen_state->detect_zeroes; - if (reopen_state->replace_backing_bs) { - qdict_del(bs->explicit_options, "backing"); - qdict_del(bs->options, "backing"); - } - /* Remove child references from bs->options and bs->explicit_options. * Child options were already removed in bdrv_reopen_queue_child() */ QLIST_FOREACH(child, &bs->children, next) { qdict_del(bs->explicit_options, child->name); qdict_del(bs->options, child->name); } + /* backing is probably removed, so it's not handled by previous loop */ + qdict_del(bs->explicit_options, "backing"); + qdict_del(bs->options, "backing"); + bdrv_refresh_limits(bs, NULL, NULL); } diff --git a/include/block/block.h b/include/block/block.h index 8e707a83b7..d28022e761 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -208,7 +208,6 @@ typedef struct BDRVReopenState { int flags; BlockdevDetectZeroesOptions detect_zeroes; bool backing_missing; - bool replace_backing_bs; /* new_backing_bs is ignored if this is false */ BlockDriverState *old_backing_bs; /* keep pointer for permissions update */ QDict *options; QDict *explicit_options; From ecd30d2d978493f2280798f4c48f674d6290efa4 Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Thu, 10 Jun 2021 15:05:36 +0300 Subject: [PATCH 13/24] block: Allow changing bs->file on reopen When the x-blockdev-reopen was added it allowed reconfiguring the graph by replacing backing files, but changing the 'file' option was forbidden. Because of this restriction some operations are not possible, notably inserting and removing block filters. This patch adds support for replacing the 'file' option. This is similar to replacing the backing file and the user is likewise responsible for the correctness of the resulting graph, otherwise this can lead to data corruption. Signed-off-by: Alberto Garcia [vsementsov: bdrv_reopen_parse_file_or_backing() is modified a lot] Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210610120537.196183-9-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf --- block.c | 78 +++++++++++++++++++++++++++++------------- include/block/block.h | 1 + tests/qemu-iotests/245 | 23 +++++++------ 3 files changed, 67 insertions(+), 35 deletions(-) diff --git a/block.c b/block.c index ca11078cd2..acd35cb0cb 100644 --- a/block.c +++ b/block.c @@ -92,7 +92,7 @@ static void bdrv_remove_filter_or_cow_child(BlockDriverState *bs, static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, - Transaction *set_backings_tran, Error **errp); + Transaction *change_child_tran, Error **errp); static void bdrv_reopen_commit(BDRVReopenState *reopen_state); static void bdrv_reopen_abort(BDRVReopenState *reopen_state); @@ -4148,6 +4148,10 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp) refresh_list = bdrv_topological_dfs(refresh_list, found, state->old_backing_bs); } + if (state->old_file_bs) { + refresh_list = bdrv_topological_dfs(refresh_list, found, + state->old_file_bs); + } } /* @@ -4240,64 +4244,81 @@ int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only, * * Return 0 on success, otherwise return < 0 and set @errp. */ -static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, - Transaction *set_backings_tran, - Error **errp) +static int bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state, + bool is_backing, Transaction *tran, + Error **errp) { BlockDriverState *bs = reopen_state->bs; - BlockDriverState *new_backing_bs; + BlockDriverState *new_child_bs; + BlockDriverState *old_child_bs = is_backing ? child_bs(bs->backing) : + child_bs(bs->file); + const char *child_name = is_backing ? "backing" : "file"; QObject *value; const char *str; - value = qdict_get(reopen_state->options, "backing"); + value = qdict_get(reopen_state->options, child_name); if (value == NULL) { return 0; } switch (qobject_type(value)) { case QTYPE_QNULL: - new_backing_bs = NULL; + assert(is_backing); /* The 'file' option does not allow a null value */ + new_child_bs = NULL; break; case QTYPE_QSTRING: str = qstring_get_str(qobject_to(QString, value)); - new_backing_bs = bdrv_lookup_bs(NULL, str, errp); - if (new_backing_bs == NULL) { + new_child_bs = bdrv_lookup_bs(NULL, str, errp); + if (new_child_bs == NULL) { return -EINVAL; - } else if (bdrv_recurse_has_child(new_backing_bs, bs)) { - error_setg(errp, "Making '%s' a backing file of '%s' " - "would create a cycle", str, bs->node_name); + } else if (bdrv_recurse_has_child(new_child_bs, bs)) { + error_setg(errp, "Making '%s' a %s child of '%s' would create a " + "cycle", str, child_name, bs->node_name); return -EINVAL; } break; default: - /* 'backing' does not allow any other data type */ + /* + * The options QDict has been flattened, so 'backing' and 'file' + * do not allow any other data type here. + */ g_assert_not_reached(); } - if (bs->backing) { - if (bdrv_skip_implicit_filters(bs->backing->bs) == new_backing_bs) { + if (old_child_bs == new_child_bs) { + return 0; + } + + if (old_child_bs) { + if (bdrv_skip_implicit_filters(old_child_bs) == new_child_bs) { return 0; } - if (bs->backing->bs->implicit) { - error_setg(errp, "Cannot change backing link if '%s' has " - "an implicit backing file", bs->node_name); + if (old_child_bs->implicit) { + error_setg(errp, "Cannot replace implicit %s child of %s", + child_name, bs->node_name); return -EPERM; } } - if (bs->drv->is_filter && !bs->backing) { + if (bs->drv->is_filter && !old_child_bs) { /* * Filters always have a file or a backing child, so we are trying to * change wrong child */ error_setg(errp, "'%s' is a %s filter node that does not support a " - "backing child", bs->node_name, bs->drv->format_name); + "%s child", bs->node_name, bs->drv->format_name, child_name); return -EINVAL; } - reopen_state->old_backing_bs = bs->backing ? bs->backing->bs : NULL; - return bdrv_set_backing_noperm(bs, new_backing_bs, set_backings_tran, errp); + if (is_backing) { + reopen_state->old_backing_bs = old_child_bs; + } else { + reopen_state->old_file_bs = old_child_bs; + } + + return bdrv_set_file_or_backing_noperm(bs, new_child_bs, is_backing, + tran, errp); } /* @@ -4319,7 +4340,7 @@ static int bdrv_reopen_parse_backing(BDRVReopenState *reopen_state, */ static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, - Transaction *set_backings_tran, Error **errp) + Transaction *change_child_tran, Error **errp) { int ret = -1; int old_flags; @@ -4439,12 +4460,21 @@ static int bdrv_reopen_prepare(BDRVReopenState *reopen_state, * either a reference to an existing node (using its node name) * or NULL to simply detach the current backing file. */ - ret = bdrv_reopen_parse_backing(reopen_state, set_backings_tran, errp); + ret = bdrv_reopen_parse_file_or_backing(reopen_state, true, + change_child_tran, errp); if (ret < 0) { goto error; } qdict_del(reopen_state->options, "backing"); + /* Allow changing the 'file' option. In this case NULL is not allowed */ + ret = bdrv_reopen_parse_file_or_backing(reopen_state, false, + change_child_tran, errp); + if (ret < 0) { + goto error; + } + qdict_del(reopen_state->options, "file"); + /* Options that are not handled are only okay if they are unchanged * compared to the old state. It is expected that some options are only * used for the initial open, but not reopen (e.g. filename) */ diff --git a/include/block/block.h b/include/block/block.h index d28022e761..7ec77ecb1a 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -209,6 +209,7 @@ typedef struct BDRVReopenState { BlockdevDetectZeroesOptions detect_zeroes; bool backing_missing; BlockDriverState *old_backing_bs; /* keep pointer for permissions update */ + BlockDriverState *old_file_bs; /* keep pointer for permissions update */ QDict *options; QDict *explicit_options; void *opaque; diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245 index c7d671366a..d955e0dfd3 100755 --- a/tests/qemu-iotests/245 +++ b/tests/qemu-iotests/245 @@ -146,8 +146,8 @@ class TestBlockdevReopen(iotests.QMPTestCase): self.reopen(opts, {'driver': 'raw'}, "Cannot change the option 'driver'") self.reopen(opts, {'driver': ''}, "Invalid parameter ''") self.reopen(opts, {'driver': None}, "Invalid parameter type for 'driver', expected: string") - self.reopen(opts, {'file': 'not-found'}, "Cannot change the option 'file'") - self.reopen(opts, {'file': ''}, "Cannot change the option 'file'") + self.reopen(opts, {'file': 'not-found'}, "Cannot find device='' nor node-name='not-found'") + self.reopen(opts, {'file': ''}, "Cannot find device='' nor node-name=''") self.reopen(opts, {'file': None}, "Invalid parameter type for 'file', expected: BlockdevRef") self.reopen(opts, {'file.node-name': 'newname'}, "Cannot change the option 'node-name'") self.reopen(opts, {'file.driver': 'host_device'}, "Cannot change the option 'driver'") @@ -443,7 +443,7 @@ class TestBlockdevReopen(iotests.QMPTestCase): # Illegal operation: hd2 is a child of hd1 self.reopen(opts[2], {'backing': 'hd1'}, - "Making 'hd1' a backing file of 'hd2' would create a cycle") + "Making 'hd1' a backing child of 'hd2' would create a cycle") # hd2 <- hd0, hd2 <- hd1 self.reopen(opts[0], {'backing': 'hd2'}) @@ -454,8 +454,9 @@ class TestBlockdevReopen(iotests.QMPTestCase): # More illegal operations self.reopen(opts[2], {'backing': 'hd1'}, - "Making 'hd1' a backing file of 'hd2' would create a cycle") - self.reopen(opts[2], {'file': 'hd0-file'}, "Cannot change the option 'file'") + "Making 'hd1' a backing child of 'hd2' would create a cycle") + self.reopen(opts[2], {'file': 'hd0-file'}, + "Permission conflict on node 'hd0-file': permissions 'write, resize' are both required by node 'hd0' (uses node 'hd0-file' as 'file' child) and unshared by node 'hd2' (uses node 'hd0-file' as 'file' child).") result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'hd2') self.assert_qmp(result, 'error/class', 'GenericError') @@ -497,18 +498,18 @@ class TestBlockdevReopen(iotests.QMPTestCase): # Illegal: hd2 is backed by hd1 self.reopen(opts[1], {'backing': 'hd2'}, - "Making 'hd2' a backing file of 'hd1' would create a cycle") + "Making 'hd2' a backing child of 'hd1' would create a cycle") # hd1 <- hd0 <- hd2 self.reopen(opts[2], {'backing': 'hd0'}) # Illegal: hd2 is backed by hd0, which is backed by hd1 self.reopen(opts[1], {'backing': 'hd2'}, - "Making 'hd2' a backing file of 'hd1' would create a cycle") + "Making 'hd2' a backing child of 'hd1' would create a cycle") # Illegal: hd1 cannot point to itself self.reopen(opts[1], {'backing': 'hd1'}, - "Making 'hd1' a backing file of 'hd1' would create a cycle") + "Making 'hd1' a backing child of 'hd1' would create a cycle") # Remove all backing files self.reopen(opts[0]) @@ -530,7 +531,7 @@ class TestBlockdevReopen(iotests.QMPTestCase): # Illegal: hd0 is a child of the blkverify node self.reopen(opts[0], {'backing': 'bv'}, - "Making 'bv' a backing file of 'hd0' would create a cycle") + "Making 'bv' a backing child of 'hd0' would create a cycle") # Delete the blkverify node result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'bv') @@ -563,7 +564,7 @@ class TestBlockdevReopen(iotests.QMPTestCase): # You can't make quorum0 a backing file of hd0: # hd0 is already a child of quorum0. self.reopen(hd_opts(0), {'backing': 'quorum0'}, - "Making 'quorum0' a backing file of 'hd0' would create a cycle") + "Making 'quorum0' a backing child of 'hd0' would create a cycle") # Delete quorum0 result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'quorum0') @@ -969,7 +970,7 @@ class TestBlockdevReopen(iotests.QMPTestCase): # We can't remove hd1 while the commit job is ongoing opts['backing'] = None - self.reopen(opts, {}, "Cannot change backing link if 'hd0' has an implicit backing file") + self.reopen(opts, {}, "Cannot replace implicit backing child of hd0") # hd2 <- hd0 self.vm.run_job('commit0', auto_finalize = False, auto_dismiss = True) From 4c5393f169ca8587cc766928293bbcbbbdff78a4 Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Thu, 10 Jun 2021 15:05:37 +0300 Subject: [PATCH 14/24] iotests: Test replacing files with x-blockdev-reopen This patch adds new tests in which we use x-blockdev-reopen to change bs->file Signed-off-by: Alberto Garcia Message-Id: <20210610120537.196183-10-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf --- tests/qemu-iotests/245 | 109 ++++++++++++++++++++++++++++++++++++- tests/qemu-iotests/245.out | 11 +++- 2 files changed, 117 insertions(+), 3 deletions(-) diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245 index d955e0dfd3..0295129cbb 100755 --- a/tests/qemu-iotests/245 +++ b/tests/qemu-iotests/245 @@ -79,7 +79,7 @@ class TestBlockdevReopen(iotests.QMPTestCase): for line in log.split("\n"): if line.startswith("Pattern verification failed"): raise Exception("%s (command #%d)" % (line, found)) - if re.match("read .*/.* bytes at offset", line): + if re.match("(read|wrote) .*/.* bytes at offset", line): found += 1 self.assertEqual(found, self.total_io_cmds, "Expected output of %d qemu-io commands, found %d" % @@ -537,6 +537,113 @@ class TestBlockdevReopen(iotests.QMPTestCase): result = self.vm.qmp('blockdev-del', conv_keys = True, node_name = 'bv') self.assert_qmp(result, 'return', {}) + # Replace the protocol layer ('file' parameter) of a disk image + def test_replace_file(self): + # Create two small raw images and add them to a running VM + qemu_img('create', '-f', 'raw', hd_path[0], '10k') + qemu_img('create', '-f', 'raw', hd_path[1], '10k') + + hd0_opts = {'driver': 'file', 'node-name': 'hd0-file', 'filename': hd_path[0] } + hd1_opts = {'driver': 'file', 'node-name': 'hd1-file', 'filename': hd_path[1] } + + result = self.vm.qmp('blockdev-add', conv_keys = False, **hd0_opts) + self.assert_qmp(result, 'return', {}) + result = self.vm.qmp('blockdev-add', conv_keys = False, **hd1_opts) + self.assert_qmp(result, 'return', {}) + + # Add a raw format layer that uses hd0-file as its protocol layer + opts = {'driver': 'raw', 'node-name': 'hd', 'file': 'hd0-file'} + + result = self.vm.qmp('blockdev-add', conv_keys = False, **opts) + self.assert_qmp(result, 'return', {}) + + # Fill the image with data + self.run_qemu_io("hd", "read -P 0 0 10k") + self.run_qemu_io("hd", "write -P 0xa0 0 10k") + + # Replace hd0-file with hd1-file and fill it with (different) data + self.reopen(opts, {'file': 'hd1-file'}) + self.run_qemu_io("hd", "read -P 0 0 10k") + self.run_qemu_io("hd", "write -P 0xa1 0 10k") + + # Use hd0-file again and check that it contains the expected data + self.reopen(opts, {'file': 'hd0-file'}) + self.run_qemu_io("hd", "read -P 0xa0 0 10k") + + # And finally do the same with hd1-file + self.reopen(opts, {'file': 'hd1-file'}) + self.run_qemu_io("hd", "read -P 0xa1 0 10k") + + # Insert (and remove) a throttle filter + def test_insert_throttle_filter(self): + # Add an image to the VM + hd0_opts = hd_opts(0) + result = self.vm.qmp('blockdev-add', conv_keys = False, **hd0_opts) + self.assert_qmp(result, 'return', {}) + + # Create a throttle-group object + opts = { 'qom-type': 'throttle-group', 'id': 'group0', + 'limits': { 'iops-total': 1000 } } + result = self.vm.qmp('object-add', conv_keys = False, **opts) + self.assert_qmp(result, 'return', {}) + + # Add a throttle filter with the group that we just created. + # The filter is not used by anyone yet + opts = { 'driver': 'throttle', 'node-name': 'throttle0', + 'throttle-group': 'group0', 'file': 'hd0-file' } + result = self.vm.qmp('blockdev-add', conv_keys = False, **opts) + self.assert_qmp(result, 'return', {}) + + # Insert the throttle filter between hd0 and hd0-file + self.reopen(hd0_opts, {'file': 'throttle0'}) + + # Remove the throttle filter from hd0 + self.reopen(hd0_opts, {'file': 'hd0-file'}) + + # Insert (and remove) a compress filter + def test_insert_compress_filter(self): + # Add an image to the VM: hd (raw) -> hd0 (qcow2) -> hd0-file (file) + opts = {'driver': 'raw', 'node-name': 'hd', 'file': hd_opts(0)} + result = self.vm.qmp('blockdev-add', conv_keys = False, **opts) + self.assert_qmp(result, 'return', {}) + + # Add a 'compress' filter + filter_opts = {'driver': 'compress', + 'node-name': 'compress0', + 'file': 'hd0'} + result = self.vm.qmp('blockdev-add', conv_keys = False, **filter_opts) + self.assert_qmp(result, 'return', {}) + + # Unmap the beginning of the image (we cannot write compressed + # data to an allocated cluster) + self.run_qemu_io("hd", "write -z -u 0 128k") + + # Write data to the first cluster + self.run_qemu_io("hd", "write -P 0xa0 0 64k") + + # Insert the filter then write to the second cluster + # hd -> compress0 -> hd0 -> hd0-file + self.reopen(opts, {'file': 'compress0'}) + self.run_qemu_io("hd", "write -P 0xa1 64k 64k") + + # Remove the filter then write to the third cluster + # hd -> hd0 -> hd0-file + self.reopen(opts, {'file': 'hd0'}) + self.run_qemu_io("hd", "write -P 0xa2 128k 64k") + + # Verify the data that we just wrote + self.run_qemu_io("hd", "read -P 0xa0 0 64k") + self.run_qemu_io("hd", "read -P 0xa1 64k 64k") + self.run_qemu_io("hd", "read -P 0xa2 128k 64k") + + self.vm.shutdown() + + # Check the first byte of the first three L2 entries and verify that + # the second one is compressed (0x40) while the others are not (0x80) + iotests.qemu_io_log('-f', 'raw', '-c', 'read -P 0x80 0x40000 1', + '-c', 'read -P 0x40 0x40008 1', + '-c', 'read -P 0x80 0x40010 1', hd_path[0]) + # Misc reopen tests with different block drivers @iotests.skip_if_unsupported(['quorum', 'throttle']) def test_misc_drivers(self): diff --git a/tests/qemu-iotests/245.out b/tests/qemu-iotests/245.out index 99c12f4f98..daf1e51922 100644 --- a/tests/qemu-iotests/245.out +++ b/tests/qemu-iotests/245.out @@ -10,8 +10,15 @@ {"return": {}} {"data": {"id": "stream0", "type": "stream"}, "event": "BLOCK_JOB_PENDING", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} {"data": {"device": "stream0", "len": 3145728, "offset": 3145728, "speed": 0, "type": "stream"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} -............... +....read 1/1 bytes at offset 262144 +1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 1/1 bytes at offset 262152 +1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 1/1 bytes at offset 262160 +1 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +.............. ---------------------------------------------------------------------- -Ran 21 tests +Ran 24 tests OK From 4d324c0bf65c615ffbe95b35497353996b97753b Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Mon, 28 Jun 2021 15:11:32 +0300 Subject: [PATCH 15/24] introduce QEMU_AUTO_VFREE Introduce a convenient macro, that works for qemu_memalign() like g_autofree works with g_malloc. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210628121133.193984-2-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf --- include/qemu/osdep.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h index c3656b755a..c91a78b5e6 100644 --- a/include/qemu/osdep.h +++ b/include/qemu/osdep.h @@ -386,6 +386,21 @@ void *qemu_anon_ram_alloc(size_t size, uint64_t *align, bool shared, void qemu_vfree(void *ptr); void qemu_anon_ram_free(void *ptr, size_t size); +/* + * It's an analog of GLIB's g_autoptr_cleanup_generic_gfree(), used to define + * g_autofree macro. + */ +static inline void qemu_cleanup_generic_vfree(void *p) +{ + void **pp = (void **)p; + qemu_vfree(*pp); +} + +/* + * Analog of g_autofree, but qemu_vfree is called on cleanup instead of g_free. + */ +#define QEMU_AUTO_VFREE __attribute__((cleanup(qemu_cleanup_generic_vfree))) + /* * Abstraction of PROT_ and MAP_ flags as passed to mmap(), for example, * consumed by qemu_ram_mmap(). From 7170170866f74f3ee9e3a143c959c97c2bc5897e Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Mon, 28 Jun 2021 15:11:33 +0300 Subject: [PATCH 16/24] block/commit: use QEMU_AUTO_VFREE Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210628121133.193984-3-vsementsov@virtuozzo.com> Signed-off-by: Kevin Wolf --- block/commit.c | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/block/commit.c b/block/commit.c index b7f0c7c061..42792b4556 100644 --- a/block/commit.c +++ b/block/commit.c @@ -119,24 +119,24 @@ static int coroutine_fn commit_run(Job *job, Error **errp) uint64_t delay_ns = 0; int ret = 0; int64_t n = 0; /* bytes */ - void *buf = NULL; + QEMU_AUTO_VFREE void *buf = NULL; int64_t len, base_len; - ret = len = blk_getlength(s->top); + len = blk_getlength(s->top); if (len < 0) { - goto out; + return len; } job_progress_set_remaining(&s->common.job, len); - ret = base_len = blk_getlength(s->base); + base_len = blk_getlength(s->base); if (base_len < 0) { - goto out; + return base_len; } if (base_len < len) { ret = blk_truncate(s->base, len, false, PREALLOC_MODE_OFF, 0, NULL); if (ret) { - goto out; + return ret; } } @@ -174,7 +174,7 @@ static int coroutine_fn commit_run(Job *job, Error **errp) block_job_error_action(&s->common, s->on_error, error_in_source, -ret); if (action == BLOCK_ERROR_ACTION_REPORT) { - goto out; + return ret; } else { n = 0; continue; @@ -190,12 +190,7 @@ static int coroutine_fn commit_run(Job *job, Error **errp) } } - ret = 0; - -out: - qemu_vfree(buf); - - return ret; + return 0; } static const BlockJobDriver commit_job_driver = { @@ -435,7 +430,7 @@ int bdrv_commit(BlockDriverState *bs) int ro; int64_t n; int ret = 0; - uint8_t *buf = NULL; + QEMU_AUTO_VFREE uint8_t *buf = NULL; Error *local_err = NULL; if (!drv) @@ -556,8 +551,6 @@ int bdrv_commit(BlockDriverState *bs) ret = 0; ro_cleanup: - qemu_vfree(buf); - blk_unref(backing); if (bdrv_cow_bs(bs) != backing_file_bs) { bdrv_set_backing_hd(bs, backing_file_bs, &error_abort); From bf783261f0aee6e81af3916bff7606d71ccdc153 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Tue, 22 Jun 2021 12:51:56 +0100 Subject: [PATCH 17/24] block/ssh: add support for sha256 host key fingerprints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently the SSH block driver supports MD5 and SHA1 for host key fingerprints. This is a cryptographically sensitive operation and so these hash algorithms are inadequate by modern standards. This adds support for SHA256 which has been supported in libssh since the 0.8.1 release. Signed-off-by: Daniel P. Berrangé Message-Id: <20210622115156.138458-1-berrange@redhat.com> Reviewed-by: Philippe Mathieu-Daudé Acked-by: Richard W.M. Jones Signed-off-by: Kevin Wolf --- block/ssh.c | 3 +++ qapi/block-core.json | 3 ++- tests/qemu-iotests/207 | 54 ++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/207.out | 25 ++++++++++++++++++ 4 files changed, 84 insertions(+), 1 deletion(-) diff --git a/block/ssh.c b/block/ssh.c index b51a031620..d008caf059 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -442,6 +442,9 @@ static int check_host_key(BDRVSSHState *s, SshHostKeyCheck *hkc, Error **errp) } else if (hkc->u.hash.type == SSH_HOST_KEY_CHECK_HASH_TYPE_SHA1) { return check_host_key_hash(s, hkc->u.hash.hash, SSH_PUBLICKEY_HASH_SHA1, errp); + } else if (hkc->u.hash.type == SSH_HOST_KEY_CHECK_HASH_TYPE_SHA256) { + return check_host_key_hash(s, hkc->u.hash.hash, + SSH_PUBLICKEY_HASH_SHA256, errp); } g_assert_not_reached(); break; diff --git a/qapi/block-core.json b/qapi/block-core.json index a54f37dbef..3114ba69bb 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3190,11 +3190,12 @@ # # @md5: The given hash is an md5 hash # @sha1: The given hash is an sha1 hash +# @sha256: The given hash is an sha256 hash # # Since: 2.12 ## { 'enum': 'SshHostKeyCheckHashType', - 'data': [ 'md5', 'sha1' ] } + 'data': [ 'md5', 'sha1', 'sha256' ] } ## # @SshHostKeyHash: diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207 index f9f3fd7131..0f5c4bc8a0 100755 --- a/tests/qemu-iotests/207 +++ b/tests/qemu-iotests/207 @@ -73,6 +73,9 @@ with iotests.FilePath('t.img') as disk_path, \ iotests.log("=== Test host-key-check options ===") iotests.log("") + iotests.log("--- no host key checking --") + iotests.log("") + vm.launch() blockdev_create(vm, { 'driver': 'ssh', 'location': { @@ -90,6 +93,9 @@ with iotests.FilePath('t.img') as disk_path, \ iotests.img_info_log(remote_path) + iotests.log("--- known_hosts key checking --") + iotests.log("") + vm.launch() blockdev_create(vm, { 'driver': 'ssh', 'location': { @@ -115,6 +121,7 @@ with iotests.FilePath('t.img') as disk_path, \ # Mappings of base64 representations to digests md5_keys = {} sha1_keys = {} + sha256_keys = {} for key in keys: md5_keys[key] = subprocess.check_output( @@ -125,6 +132,10 @@ with iotests.FilePath('t.img') as disk_path, \ 'echo %s | base64 -d | sha1sum -b | cut -d" " -f1' % key, shell=True).rstrip().decode('ascii') + sha256_keys[key] = subprocess.check_output( + 'echo %s | base64 -d | sha256sum -b | cut -d" " -f1' % key, + shell=True).rstrip().decode('ascii') + vm.launch() # Find correct key first @@ -150,6 +161,9 @@ with iotests.FilePath('t.img') as disk_path, \ vm.shutdown() iotests.notrun('Did not find a key that fits 127.0.0.1') + iotests.log("--- explicit md5 key checking --") + iotests.log("") + blockdev_create(vm, { 'driver': 'ssh', 'location': { 'path': disk_path, @@ -164,6 +178,7 @@ with iotests.FilePath('t.img') as disk_path, \ } }, 'size': 2097152 }) + blockdev_create(vm, { 'driver': 'ssh', 'location': { 'path': disk_path, @@ -182,6 +197,9 @@ with iotests.FilePath('t.img') as disk_path, \ iotests.img_info_log(remote_path) + iotests.log("--- explicit sha1 key checking --") + iotests.log("") + vm.launch() blockdev_create(vm, { 'driver': 'ssh', 'location': { @@ -215,6 +233,42 @@ with iotests.FilePath('t.img') as disk_path, \ iotests.img_info_log(remote_path) + iotests.log("--- explicit sha256 key checking --") + iotests.log("") + + vm.launch() + blockdev_create(vm, { 'driver': 'ssh', + 'location': { + 'path': disk_path, + 'server': { + 'host': '127.0.0.1', + 'port': '22' + }, + 'host-key-check': { + 'mode': 'hash', + 'type': 'sha256', + 'hash': 'wrong', + } + }, + 'size': 2097152 }) + blockdev_create(vm, { 'driver': 'ssh', + 'location': { + 'path': disk_path, + 'server': { + 'host': '127.0.0.1', + 'port': '22' + }, + 'host-key-check': { + 'mode': 'hash', + 'type': 'sha256', + 'hash': sha256_keys[matching_key], + } + }, + 'size': 4194304 }) + vm.shutdown() + + iotests.img_info_log(remote_path) + # # Invalid path and user # diff --git a/tests/qemu-iotests/207.out b/tests/qemu-iotests/207.out index 1239d9d648..aeb8569d77 100644 --- a/tests/qemu-iotests/207.out +++ b/tests/qemu-iotests/207.out @@ -16,6 +16,8 @@ virtual size: 4 MiB (4194304 bytes) === Test host-key-check options === +--- no host key checking -- + {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "ssh", "location": {"host-key-check": {"mode": "none"}, "path": "TEST_DIR/PID-t.img", "server": {"host": "127.0.0.1", "port": "22"}}, "size": 8388608}}} {"return": {}} {"execute": "job-dismiss", "arguments": {"id": "job0"}} @@ -25,6 +27,8 @@ image: TEST_IMG file format: IMGFMT virtual size: 8 MiB (8388608 bytes) +--- known_hosts key checking -- + {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "ssh", "location": {"host-key-check": {"mode": "known_hosts"}, "path": "TEST_DIR/PID-t.img", "server": {"host": "127.0.0.1", "port": "22"}}, "size": 4194304}}} {"return": {}} {"execute": "job-dismiss", "arguments": {"id": "job0"}} @@ -34,6 +38,8 @@ image: TEST_IMG file format: IMGFMT virtual size: 4 MiB (4194304 bytes) +--- explicit md5 key checking -- + {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "ssh", "location": {"host-key-check": {"hash": "wrong", "mode": "hash", "type": "md5"}, "path": "TEST_DIR/PID-t.img", "server": {"host": "127.0.0.1", "port": "22"}}, "size": 2097152}}} {"return": {}} Job failed: remote host key does not match host_key_check 'wrong' @@ -49,6 +55,8 @@ image: TEST_IMG file format: IMGFMT virtual size: 8 MiB (8388608 bytes) +--- explicit sha1 key checking -- + {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "ssh", "location": {"host-key-check": {"hash": "wrong", "mode": "hash", "type": "sha1"}, "path": "TEST_DIR/PID-t.img", "server": {"host": "127.0.0.1", "port": "22"}}, "size": 2097152}}} {"return": {}} Job failed: remote host key does not match host_key_check 'wrong' @@ -64,6 +72,23 @@ image: TEST_IMG file format: IMGFMT virtual size: 4 MiB (4194304 bytes) +--- explicit sha256 key checking -- + +{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "ssh", "location": {"host-key-check": {"hash": "wrong", "mode": "hash", "type": "sha256"}, "path": "TEST_DIR/PID-t.img", "server": {"host": "127.0.0.1", "port": "22"}}, "size": 2097152}}} +{"return": {}} +Job failed: remote host key does not match host_key_check 'wrong' +{"execute": "job-dismiss", "arguments": {"id": "job0"}} +{"return": {}} + +{"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "ssh", "location": {"host-key-check": {"hash": "HASH", "mode": "hash", "type": "sha256"}, "path": "TEST_DIR/PID-t.img", "server": {"host": "127.0.0.1", "port": "22"}}, "size": 4194304}}} +{"return": {}} +{"execute": "job-dismiss", "arguments": {"id": "job0"}} +{"return": {}} + +image: TEST_IMG +file format: IMGFMT +virtual size: 4 MiB (4194304 bytes) + === Invalid path and user === {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "ssh", "location": {"host-key-check": {"mode": "none"}, "path": "/this/is/not/an/existing/path", "server": {"host": "127.0.0.1", "port": "22"}}, "size": 4194304}}} From a6945f2287aa7f048b263d7187364cbf1dd5d94d Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 9 Jun 2021 08:46:52 -0700 Subject: [PATCH 18/24] vhost: Add Error parameter to vhost_dev_init() This allows callers to return better error messages instead of making one up while the real error ends up on stderr. Most callers can immediately make use of this because they already have an Error parameter themselves. The others just keep printing the error with error_report_err(). Signed-off-by: Kevin Wolf Message-Id: <20210609154658.350308-2-kwolf@redhat.com> Reviewed-by: Stefano Garzarella Reviewed-by: Raphael Norwitz Signed-off-by: Kevin Wolf --- backends/cryptodev-vhost.c | 5 ++++- backends/vhost-user.c | 4 ++-- hw/block/vhost-user-blk.c | 4 ++-- hw/net/vhost_net.c | 6 +++++- hw/scsi/vhost-scsi.c | 4 +--- hw/scsi/vhost-user-scsi.c | 4 +--- hw/virtio/vhost-user-fs.c | 3 +-- hw/virtio/vhost-user-vsock.c | 3 +-- hw/virtio/vhost-vsock.c | 3 +-- hw/virtio/vhost.c | 16 ++++++++++------ include/hw/virtio/vhost.h | 2 +- 11 files changed, 29 insertions(+), 25 deletions(-) diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c index 8231e7f1bc..bc13e466b4 100644 --- a/backends/cryptodev-vhost.c +++ b/backends/cryptodev-vhost.c @@ -52,6 +52,7 @@ cryptodev_vhost_init( { int r; CryptoDevBackendVhost *crypto; + Error *local_err = NULL; crypto = g_new(CryptoDevBackendVhost, 1); crypto->dev.max_queues = 1; @@ -66,8 +67,10 @@ cryptodev_vhost_init( /* vhost-user needs vq_index to initiate a specific queue pair */ crypto->dev.vq_index = crypto->cc->queue_index * crypto->dev.nvqs; - r = vhost_dev_init(&crypto->dev, options->opaque, options->backend_type, 0); + r = vhost_dev_init(&crypto->dev, options->opaque, options->backend_type, 0, + &local_err); if (r < 0) { + error_report_err(local_err); goto fail; } diff --git a/backends/vhost-user.c b/backends/vhost-user.c index b366610e16..10b39992d2 100644 --- a/backends/vhost-user.c +++ b/backends/vhost-user.c @@ -48,9 +48,9 @@ vhost_user_backend_dev_init(VhostUserBackend *b, VirtIODevice *vdev, b->dev.nvqs = nvqs; b->dev.vqs = g_new0(struct vhost_virtqueue, nvqs); - ret = vhost_dev_init(&b->dev, &b->vhost_user, VHOST_BACKEND_TYPE_USER, 0); + ret = vhost_dev_init(&b->dev, &b->vhost_user, VHOST_BACKEND_TYPE_USER, 0, + errp); if (ret < 0) { - error_setg_errno(errp, -ret, "vhost initialization failed"); return -1; } diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index c6210fad0c..0cb56baefb 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -332,9 +332,9 @@ static int vhost_user_blk_connect(DeviceState *dev, Error **errp) vhost_dev_set_config_notifier(&s->dev, &blk_ops); - ret = vhost_dev_init(&s->dev, &s->vhost_user, VHOST_BACKEND_TYPE_USER, 0); + ret = vhost_dev_init(&s->dev, &s->vhost_user, VHOST_BACKEND_TYPE_USER, 0, + errp); if (ret < 0) { - error_setg_errno(errp, -ret, "vhost initialization failed"); return ret; } diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c index 44c1ed92dc..447b119f85 100644 --- a/hw/net/vhost_net.c +++ b/hw/net/vhost_net.c @@ -22,6 +22,7 @@ #include "standard-headers/linux/vhost_types.h" #include "hw/virtio/virtio-net.h" #include "net/vhost_net.h" +#include "qapi/error.h" #include "qemu/error-report.h" #include "qemu/main-loop.h" @@ -157,6 +158,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options) bool backend_kernel = options->backend_type == VHOST_BACKEND_TYPE_KERNEL; struct vhost_net *net = g_new0(struct vhost_net, 1); uint64_t features = 0; + Error *local_err = NULL; if (!options->net_backend) { fprintf(stderr, "vhost-net requires net backend to be setup\n"); @@ -187,8 +189,10 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options) } r = vhost_dev_init(&net->dev, options->opaque, - options->backend_type, options->busyloop_timeout); + options->backend_type, options->busyloop_timeout, + &local_err); if (r < 0) { + error_report_err(local_err); goto fail; } if (backend_kernel) { diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c index 4d70fa036b..8c611bfd2d 100644 --- a/hw/scsi/vhost-scsi.c +++ b/hw/scsi/vhost-scsi.c @@ -219,10 +219,8 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) vsc->dev.backend_features = 0; ret = vhost_dev_init(&vsc->dev, (void *)(uintptr_t)vhostfd, - VHOST_BACKEND_TYPE_KERNEL, 0); + VHOST_BACKEND_TYPE_KERNEL, 0, errp); if (ret < 0) { - error_setg(errp, "vhost-scsi: vhost initialization failed: %s", - strerror(-ret)); goto free_vqs; } diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c index 4666019442..1b2f7eed98 100644 --- a/hw/scsi/vhost-user-scsi.c +++ b/hw/scsi/vhost-user-scsi.c @@ -122,10 +122,8 @@ static void vhost_user_scsi_realize(DeviceState *dev, Error **errp) vqs = vsc->dev.vqs; ret = vhost_dev_init(&vsc->dev, &s->vhost_user, - VHOST_BACKEND_TYPE_USER, 0); + VHOST_BACKEND_TYPE_USER, 0, errp); if (ret < 0) { - error_setg(errp, "vhost-user-scsi: vhost initialization failed: %s", - strerror(-ret)); goto free_vhost; } diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c index 6f7f91533d..c595957983 100644 --- a/hw/virtio/vhost-user-fs.c +++ b/hw/virtio/vhost-user-fs.c @@ -235,9 +235,8 @@ static void vuf_device_realize(DeviceState *dev, Error **errp) fs->vhost_dev.nvqs = 1 + fs->conf.num_request_queues; fs->vhost_dev.vqs = g_new0(struct vhost_virtqueue, fs->vhost_dev.nvqs); ret = vhost_dev_init(&fs->vhost_dev, &fs->vhost_user, - VHOST_BACKEND_TYPE_USER, 0); + VHOST_BACKEND_TYPE_USER, 0, errp); if (ret < 0) { - error_setg_errno(errp, -ret, "vhost_dev_init failed"); goto err_virtio; } diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c index a6f08c26b9..b6a4a25ea1 100644 --- a/hw/virtio/vhost-user-vsock.c +++ b/hw/virtio/vhost-user-vsock.c @@ -108,9 +108,8 @@ static void vuv_device_realize(DeviceState *dev, Error **errp) vhost_dev_set_config_notifier(&vvc->vhost_dev, &vsock_ops); ret = vhost_dev_init(&vvc->vhost_dev, &vsock->vhost_user, - VHOST_BACKEND_TYPE_USER, 0); + VHOST_BACKEND_TYPE_USER, 0, errp); if (ret < 0) { - error_setg_errno(errp, -ret, "vhost_dev_init failed"); goto err_virtio; } diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c index 8ddfb9abfe..777cafe70d 100644 --- a/hw/virtio/vhost-vsock.c +++ b/hw/virtio/vhost-vsock.c @@ -170,9 +170,8 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp) vhost_vsock_common_realize(vdev, "vhost-vsock"); ret = vhost_dev_init(&vvc->vhost_dev, (void *)(uintptr_t)vhostfd, - VHOST_BACKEND_TYPE_KERNEL, 0); + VHOST_BACKEND_TYPE_KERNEL, 0, errp); if (ret < 0) { - error_setg_errno(errp, -ret, "vhost-vsock: vhost_dev_init failed"); goto err_virtio; } diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 7b7bde7657..991c67ddcd 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -1286,7 +1286,8 @@ static void vhost_virtqueue_cleanup(struct vhost_virtqueue *vq) } int vhost_dev_init(struct vhost_dev *hdev, void *opaque, - VhostBackendType backend_type, uint32_t busyloop_timeout) + VhostBackendType backend_type, uint32_t busyloop_timeout, + Error **errp) { uint64_t features; int i, r, n_initialized_vqs = 0; @@ -1300,24 +1301,26 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, r = hdev->vhost_ops->vhost_backend_init(hdev, opaque); if (r < 0) { + error_setg(errp, "vhost_backend_init failed"); goto fail; } r = hdev->vhost_ops->vhost_set_owner(hdev); if (r < 0) { - VHOST_OPS_DEBUG("vhost_set_owner failed"); + error_setg(errp, "vhost_set_owner failed"); goto fail; } r = hdev->vhost_ops->vhost_get_features(hdev, &features); if (r < 0) { - VHOST_OPS_DEBUG("vhost_get_features failed"); + error_setg(errp, "vhost_get_features failed"); goto fail; } for (i = 0; i < hdev->nvqs; ++i, ++n_initialized_vqs) { r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i); if (r < 0) { + error_setg_errno(errp, -r, "Failed to initialize virtqueue %d", i); goto fail; } } @@ -1327,6 +1330,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, r = vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i, busyloop_timeout); if (r < 0) { + error_setg(errp, "Failed to set busyloop timeout"); goto fail_busyloop; } } @@ -1367,7 +1371,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, if (hdev->migration_blocker != NULL) { r = migrate_add_blocker(hdev->migration_blocker, &local_err); if (local_err) { - error_report_err(local_err); + error_propagate(errp, local_err); error_free(hdev->migration_blocker); goto fail_busyloop; } @@ -1384,8 +1388,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, QLIST_INSERT_HEAD(&vhost_devices, hdev, entry); if (used_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) { - error_report("vhost backend memory slots limit is less" - " than current number of present memory slots"); + error_setg(errp, "vhost backend memory slots limit is less" + " than current number of present memory slots"); r = -1; goto fail_busyloop; } diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h index 21a9a52088..2d7aaad67b 100644 --- a/include/hw/virtio/vhost.h +++ b/include/hw/virtio/vhost.h @@ -104,7 +104,7 @@ struct vhost_net { int vhost_dev_init(struct vhost_dev *hdev, void *opaque, VhostBackendType backend_type, - uint32_t busyloop_timeout); + uint32_t busyloop_timeout, Error **errp); void vhost_dev_cleanup(struct vhost_dev *hdev); int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev); void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev); From 28770ff935bce723c5799d292bc788770b69a733 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 9 Jun 2021 08:46:53 -0700 Subject: [PATCH 19/24] vhost: Distinguish errors in vhost_backend_init() Instead of just returning 0/-1 and letting the caller make up a meaningless error message, add an Error parameter to allow reporting the real error and switch to 0/-errno so that different kind of errors can be distinguished in the caller. Specifically, in vhost-user, EPROTO is used for all errors that relate to the connection itself, whereas other error codes are used for errors relating to the content of the connection. This will allow us later to automatically reconnect when the connection goes away, without ending up in an endless loop if it's a permanent error in the configuration. Signed-off-by: Kevin Wolf Message-Id: <20210609154658.350308-3-kwolf@redhat.com> Reviewed-by: Stefano Garzarella Reviewed-by: Raphael Norwitz Signed-off-by: Kevin Wolf --- hw/virtio/vhost-backend.c | 2 +- hw/virtio/vhost-user.c | 41 ++++++++++++++++--------------- hw/virtio/vhost-vdpa.c | 2 +- hw/virtio/vhost.c | 13 +++++----- include/hw/virtio/vhost-backend.h | 3 ++- 5 files changed, 32 insertions(+), 29 deletions(-) diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c index 31b33bde37..f4f71cf58a 100644 --- a/hw/virtio/vhost-backend.c +++ b/hw/virtio/vhost-backend.c @@ -30,7 +30,7 @@ static int vhost_kernel_call(struct vhost_dev *dev, unsigned long int request, return ioctl(fd, request, arg); } -static int vhost_kernel_init(struct vhost_dev *dev, void *opaque) +static int vhost_kernel_init(struct vhost_dev *dev, void *opaque, Error **errp) { assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_KERNEL); diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index ee57abe045..024cb201bb 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -1856,7 +1856,8 @@ static int vhost_user_postcopy_notifier(NotifierWithReturn *notifier, return 0; } -static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) +static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque, + Error **errp) { uint64_t features, protocol_features, ram_slots; struct vhost_user *u; @@ -1871,7 +1872,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) err = vhost_user_get_features(dev, &features); if (err < 0) { - return err; + return -EPROTO; } if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) { @@ -1880,7 +1881,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) err = vhost_user_get_u64(dev, VHOST_USER_GET_PROTOCOL_FEATURES, &protocol_features); if (err < 0) { - return err; + return -EPROTO; } dev->protocol_features = @@ -1891,14 +1892,14 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) dev->protocol_features &= ~(1ULL << VHOST_USER_PROTOCOL_F_CONFIG); } else if (!(protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_CONFIG))) { - error_report("Device expects VHOST_USER_PROTOCOL_F_CONFIG " - "but backend does not support it."); - return -1; + error_setg(errp, "Device expects VHOST_USER_PROTOCOL_F_CONFIG " + "but backend does not support it."); + return -EINVAL; } err = vhost_user_set_protocol_features(dev, dev->protocol_features); if (err < 0) { - return err; + return -EPROTO; } /* query the max queues we support if backend supports Multiple Queue */ @@ -1906,12 +1907,12 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) err = vhost_user_get_u64(dev, VHOST_USER_GET_QUEUE_NUM, &dev->max_queues); if (err < 0) { - return err; + return -EPROTO; } } if (dev->num_queues && dev->max_queues < dev->num_queues) { - error_report("The maximum number of queues supported by the " - "backend is %" PRIu64, dev->max_queues); + error_setg(errp, "The maximum number of queues supported by the " + "backend is %" PRIu64, dev->max_queues); return -EINVAL; } @@ -1920,9 +1921,9 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) VHOST_USER_PROTOCOL_F_SLAVE_REQ) && virtio_has_feature(dev->protocol_features, VHOST_USER_PROTOCOL_F_REPLY_ACK))) { - error_report("IOMMU support requires reply-ack and " - "slave-req protocol features."); - return -1; + error_setg(errp, "IOMMU support requires reply-ack and " + "slave-req protocol features."); + return -EINVAL; } /* get max memory regions if backend supports configurable RAM slots */ @@ -1932,15 +1933,15 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) } else { err = vhost_user_get_max_memslots(dev, &ram_slots); if (err < 0) { - return err; + return -EPROTO; } if (ram_slots < u->user->memory_slots) { - error_report("The backend specified a max ram slots limit " - "of %" PRIu64", when the prior validated limit was %d. " - "This limit should never decrease.", ram_slots, - u->user->memory_slots); - return -1; + error_setg(errp, "The backend specified a max ram slots limit " + "of %" PRIu64", when the prior validated limit was " + "%d. This limit should never decrease.", ram_slots, + u->user->memory_slots); + return -EINVAL; } u->user->memory_slots = MIN(ram_slots, VHOST_USER_MAX_RAM_SLOTS); @@ -1958,7 +1959,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque) if (dev->vq_index == 0) { err = vhost_setup_slave_channel(dev); if (err < 0) { - return err; + return -EPROTO; } } diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index 61ba313331..80827ee040 100644 --- a/hw/virtio/vhost-vdpa.c +++ b/hw/virtio/vhost-vdpa.c @@ -265,7 +265,7 @@ static void vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status) vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &s); } -static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque) +static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp) { struct vhost_vdpa *v; assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA); diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 991c67ddcd..fd13135706 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -1289,9 +1289,9 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, VhostBackendType backend_type, uint32_t busyloop_timeout, Error **errp) { + ERRP_GUARD(); uint64_t features; int i, r, n_initialized_vqs = 0; - Error *local_err = NULL; hdev->vdev = NULL; hdev->migration_blocker = NULL; @@ -1299,9 +1299,11 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, r = vhost_set_backend_type(hdev, backend_type); assert(r >= 0); - r = hdev->vhost_ops->vhost_backend_init(hdev, opaque); + r = hdev->vhost_ops->vhost_backend_init(hdev, opaque, errp); if (r < 0) { - error_setg(errp, "vhost_backend_init failed"); + if (!*errp) { + error_setg_errno(errp, -r, "vhost_backend_init failed"); + } goto fail; } @@ -1369,9 +1371,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, } if (hdev->migration_blocker != NULL) { - r = migrate_add_blocker(hdev->migration_blocker, &local_err); - if (local_err) { - error_propagate(errp, local_err); + r = migrate_add_blocker(hdev->migration_blocker, errp); + if (*errp) { error_free(hdev->migration_blocker); goto fail_busyloop; } diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h index 8a6f8e2a7a..728ebb0ed9 100644 --- a/include/hw/virtio/vhost-backend.h +++ b/include/hw/virtio/vhost-backend.h @@ -37,7 +37,8 @@ struct vhost_scsi_target; struct vhost_iotlb_msg; struct vhost_virtqueue; -typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque); +typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque, + Error **errp); typedef int (*vhost_backend_cleanup)(struct vhost_dev *dev); typedef int (*vhost_backend_memslots_limit)(struct vhost_dev *dev); From f2a6e6c4fa1f0d8ca847eecdb4d955b5770c057f Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 9 Jun 2021 08:46:54 -0700 Subject: [PATCH 20/24] vhost: Return 0/-errno in vhost_dev_init() Instead of just returning 0/-1 and letting the caller make up a meaningless error message, switch to 0/-errno so that different kinds of errors can be distinguished in the caller. This involves changing a few more callbacks in VhostOps to return 0/-errno: .vhost_set_owner(), .vhost_get_features() and .vhost_virtqueue_set_busyloop_timeout(). The implementations of these functions are trivial as they generally just send a message to the backend. Signed-off-by: Kevin Wolf Message-Id: <20210609154658.350308-4-kwolf@redhat.com> Reviewed-by: Stefano Garzarella Reviewed-by: Raphael Norwitz Signed-off-by: Kevin Wolf --- hw/virtio/vhost-backend.c | 4 +++- hw/virtio/vhost-user.c | 10 +++++++--- hw/virtio/vhost-vdpa.c | 4 +++- hw/virtio/vhost.c | 8 ++++---- 4 files changed, 17 insertions(+), 9 deletions(-) diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c index f4f71cf58a..594d770b75 100644 --- a/hw/virtio/vhost-backend.c +++ b/hw/virtio/vhost-backend.c @@ -24,10 +24,12 @@ static int vhost_kernel_call(struct vhost_dev *dev, unsigned long int request, void *arg) { int fd = (uintptr_t) dev->opaque; + int ret; assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_KERNEL); - return ioctl(fd, request, arg); + ret = ioctl(fd, request, arg); + return ret < 0 ? -errno : ret; } static int vhost_kernel_init(struct vhost_dev *dev, void *opaque, Error **errp) diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 024cb201bb..889559d86a 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -1353,7 +1353,11 @@ static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t *u64) static int vhost_user_get_features(struct vhost_dev *dev, uint64_t *features) { - return vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features); + if (vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features) < 0) { + return -EPROTO; + } + + return 0; } static int vhost_user_set_owner(struct vhost_dev *dev) @@ -1364,7 +1368,7 @@ static int vhost_user_set_owner(struct vhost_dev *dev) }; if (vhost_user_write(dev, &msg, NULL, 0) < 0) { - return -1; + return -EPROTO; } return 0; @@ -1872,7 +1876,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque, err = vhost_user_get_features(dev, &features); if (err < 0) { - return -EPROTO; + return err; } if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) { diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index 80827ee040..0f469f1823 100644 --- a/hw/virtio/vhost-vdpa.c +++ b/hw/virtio/vhost-vdpa.c @@ -245,10 +245,12 @@ static int vhost_vdpa_call(struct vhost_dev *dev, unsigned long int request, { struct vhost_vdpa *v = dev->opaque; int fd = v->device_fd; + int ret; assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA); - return ioctl(fd, request, arg); + ret = ioctl(fd, request, arg); + return ret < 0 ? -errno : ret; } static void vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status) diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index fd13135706..c7f9d8bb06 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -1309,13 +1309,13 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, r = hdev->vhost_ops->vhost_set_owner(hdev); if (r < 0) { - error_setg(errp, "vhost_set_owner failed"); + error_setg_errno(errp, -r, "vhost_set_owner failed"); goto fail; } r = hdev->vhost_ops->vhost_get_features(hdev, &features); if (r < 0) { - error_setg(errp, "vhost_get_features failed"); + error_setg_errno(errp, -r, "vhost_get_features failed"); goto fail; } @@ -1332,7 +1332,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, r = vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i, busyloop_timeout); if (r < 0) { - error_setg(errp, "Failed to set busyloop timeout"); + error_setg_errno(errp, -r, "Failed to set busyloop timeout"); goto fail_busyloop; } } @@ -1391,7 +1391,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque, if (used_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) { error_setg(errp, "vhost backend memory slots limit is less" " than current number of present memory slots"); - r = -1; + r = -EINVAL; goto fail_busyloop; } From b8da65689ac18f4d288bdc679fb178031bdd929e Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 9 Jun 2021 08:46:55 -0700 Subject: [PATCH 21/24] vhost-user-blk: Add Error parameter to vhost_user_blk_start() Instead of letting the caller make up a meaningless error message, add an Error parameter to allow reporting the real error. Signed-off-by: Kevin Wolf Message-Id: <20210609154658.350308-5-kwolf@redhat.com> Reviewed-by: Stefano Garzarella Reviewed-by: Raphael Norwitz Signed-off-by: Kevin Wolf --- hw/block/vhost-user-blk.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index 0cb56baefb..e9382e152a 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -113,7 +113,7 @@ const VhostDevConfigOps blk_ops = { .vhost_dev_config_notifier = vhost_user_blk_handle_config_change, }; -static int vhost_user_blk_start(VirtIODevice *vdev) +static int vhost_user_blk_start(VirtIODevice *vdev, Error **errp) { VHostUserBlk *s = VHOST_USER_BLK(vdev); BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev))); @@ -121,19 +121,19 @@ static int vhost_user_blk_start(VirtIODevice *vdev) int i, ret; if (!k->set_guest_notifiers) { - error_report("binding does not support guest notifiers"); + error_setg(errp, "binding does not support guest notifiers"); return -ENOSYS; } ret = vhost_dev_enable_notifiers(&s->dev, vdev); if (ret < 0) { - error_report("Error enabling host notifiers: %d", -ret); + error_setg_errno(errp, -ret, "Error enabling host notifiers"); return ret; } ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, true); if (ret < 0) { - error_report("Error binding guest notifier: %d", -ret); + error_setg_errno(errp, -ret, "Error binding guest notifier"); goto err_host_notifiers; } @@ -141,27 +141,27 @@ static int vhost_user_blk_start(VirtIODevice *vdev) ret = vhost_dev_prepare_inflight(&s->dev, vdev); if (ret < 0) { - error_report("Error set inflight format: %d", -ret); + error_setg_errno(errp, -ret, "Error setting inflight format"); goto err_guest_notifiers; } if (!s->inflight->addr) { ret = vhost_dev_get_inflight(&s->dev, s->queue_size, s->inflight); if (ret < 0) { - error_report("Error get inflight: %d", -ret); + error_setg_errno(errp, -ret, "Error getting inflight"); goto err_guest_notifiers; } } ret = vhost_dev_set_inflight(&s->dev, s->inflight); if (ret < 0) { - error_report("Error set inflight: %d", -ret); + error_setg_errno(errp, -ret, "Error setting inflight"); goto err_guest_notifiers; } ret = vhost_dev_start(&s->dev, vdev); if (ret < 0) { - error_report("Error starting vhost: %d", -ret); + error_setg_errno(errp, -ret, "Error starting vhost"); goto err_guest_notifiers; } s->started_vu = true; @@ -214,6 +214,7 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status) { VHostUserBlk *s = VHOST_USER_BLK(vdev); bool should_start = virtio_device_started(vdev, status); + Error *local_err = NULL; int ret; if (!vdev->vm_running) { @@ -229,10 +230,9 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status) } if (should_start) { - ret = vhost_user_blk_start(vdev); + ret = vhost_user_blk_start(vdev, &local_err); if (ret < 0) { - error_report("vhost-user-blk: vhost start failed: %s", - strerror(-ret)); + error_reportf_err(local_err, "vhost-user-blk: vhost start failed: "); qemu_chr_fe_disconnect(&s->chardev); } } else { @@ -270,6 +270,7 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev, static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq) { VHostUserBlk *s = VHOST_USER_BLK(vdev); + Error *local_err = NULL; int i, ret; if (!vdev->start_on_kick) { @@ -287,10 +288,9 @@ static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq) /* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start * vhost here instead of waiting for .set_status(). */ - ret = vhost_user_blk_start(vdev); + ret = vhost_user_blk_start(vdev, &local_err); if (ret < 0) { - error_report("vhost-user-blk: vhost start failed: %s", - strerror(-ret)); + error_reportf_err(local_err, "vhost-user-blk: vhost start failed: "); qemu_chr_fe_disconnect(&s->chardev); return; } @@ -340,9 +340,8 @@ static int vhost_user_blk_connect(DeviceState *dev, Error **errp) /* restore vhost state */ if (virtio_device_started(vdev, vdev->status)) { - ret = vhost_user_blk_start(vdev); + ret = vhost_user_blk_start(vdev, errp); if (ret < 0) { - error_setg_errno(errp, -ret, "vhost start failed"); return ret; } } From 50de51387f3fda9d3da049d60f8b631164f11f08 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 9 Jun 2021 08:46:56 -0700 Subject: [PATCH 22/24] vhost: Distinguish errors in vhost_dev_get_config() Instead of just returning 0/-1 and letting the caller make up a meaningless error message, add an Error parameter to allow reporting the real error and switch to 0/-errno so that different kind of errors can be distinguished in the caller. config_len in vhost_user_get_config() is defined by the device, so if it's larger than VHOST_USER_MAX_CONFIG_SIZE, this is a programming error. Turn the corresponding check into an assertion. Signed-off-by: Kevin Wolf Message-Id: <20210609154658.350308-6-kwolf@redhat.com> Reviewed-by: Stefano Garzarella Reviewed-by: Raphael Norwitz Signed-off-by: Kevin Wolf --- hw/block/vhost-user-blk.c | 9 +++++---- hw/display/vhost-user-gpu.c | 6 ++++-- hw/input/vhost-user-input.c | 6 ++++-- hw/net/vhost_net.c | 2 +- hw/virtio/vhost-user-vsock.c | 9 +++++---- hw/virtio/vhost-user.c | 24 ++++++++++++------------ hw/virtio/vhost-vdpa.c | 2 +- hw/virtio/vhost.c | 14 +++++++++++--- include/hw/virtio/vhost-backend.h | 2 +- include/hw/virtio/vhost.h | 4 ++-- 10 files changed, 46 insertions(+), 32 deletions(-) diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index e9382e152a..3770f715da 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -91,11 +91,13 @@ static int vhost_user_blk_handle_config_change(struct vhost_dev *dev) int ret; struct virtio_blk_config blkcfg; VHostUserBlk *s = VHOST_USER_BLK(dev->vdev); + Error *local_err = NULL; ret = vhost_dev_get_config(dev, (uint8_t *)&blkcfg, - sizeof(struct virtio_blk_config)); + sizeof(struct virtio_blk_config), + &local_err); if (ret < 0) { - error_report("get config space failed"); + error_report_err(local_err); return -1; } @@ -478,9 +480,8 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) assert(s->connected); ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg, - sizeof(struct virtio_blk_config)); + sizeof(struct virtio_blk_config), errp); if (ret < 0) { - error_setg(errp, "vhost-user-blk: get block config failed"); goto vhost_err; } diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c index 6cdaa1c73b..389199e6ca 100644 --- a/hw/display/vhost-user-gpu.c +++ b/hw/display/vhost-user-gpu.c @@ -415,14 +415,16 @@ vhost_user_gpu_get_config(VirtIODevice *vdev, uint8_t *config_data) VirtIOGPUBase *b = VIRTIO_GPU_BASE(vdev); struct virtio_gpu_config *vgconfig = (struct virtio_gpu_config *)config_data; + Error *local_err = NULL; int ret; memset(config_data, 0, sizeof(struct virtio_gpu_config)); ret = vhost_dev_get_config(&g->vhost->dev, - config_data, sizeof(struct virtio_gpu_config)); + config_data, sizeof(struct virtio_gpu_config), + &local_err); if (ret) { - error_report("vhost-user-gpu: get device config space failed"); + error_report_err(local_err); return; } diff --git a/hw/input/vhost-user-input.c b/hw/input/vhost-user-input.c index 63984a8ba7..273e96a7b1 100644 --- a/hw/input/vhost-user-input.c +++ b/hw/input/vhost-user-input.c @@ -49,13 +49,15 @@ static void vhost_input_get_config(VirtIODevice *vdev, uint8_t *config_data) { VirtIOInput *vinput = VIRTIO_INPUT(vdev); VHostUserInput *vhi = VHOST_USER_INPUT(vdev); + Error *local_err = NULL; int ret; memset(config_data, 0, vinput->cfg_size); - ret = vhost_dev_get_config(&vhi->vhost->dev, config_data, vinput->cfg_size); + ret = vhost_dev_get_config(&vhi->vhost->dev, config_data, vinput->cfg_size, + &local_err); if (ret) { - error_report("vhost-user-input: get device config space failed"); + error_report_err(local_err); return; } } diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c index 447b119f85..10a7780a13 100644 --- a/hw/net/vhost_net.c +++ b/hw/net/vhost_net.c @@ -117,7 +117,7 @@ uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features) int vhost_net_get_config(struct vhost_net *net, uint8_t *config, uint32_t config_len) { - return vhost_dev_get_config(&net->dev, config, config_len); + return vhost_dev_get_config(&net->dev, config, config_len, NULL); } int vhost_net_set_config(struct vhost_net *net, const uint8_t *data, uint32_t offset, uint32_t size, uint32_t flags) diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c index b6a4a25ea1..6095ed7349 100644 --- a/hw/virtio/vhost-user-vsock.c +++ b/hw/virtio/vhost-user-vsock.c @@ -34,10 +34,12 @@ static void vuv_get_config(VirtIODevice *vdev, uint8_t *config) static int vuv_handle_config_change(struct vhost_dev *dev) { VHostUserVSock *vsock = VHOST_USER_VSOCK(dev->vdev); + Error *local_err = NULL; int ret = vhost_dev_get_config(dev, (uint8_t *)&vsock->vsockcfg, - sizeof(struct virtio_vsock_config)); + sizeof(struct virtio_vsock_config), + &local_err); if (ret < 0) { - error_report("get config space failed"); + error_report_err(local_err); return -1; } @@ -114,9 +116,8 @@ static void vuv_device_realize(DeviceState *dev, Error **errp) } ret = vhost_dev_get_config(&vvc->vhost_dev, (uint8_t *)&vsock->vsockcfg, - sizeof(struct virtio_vsock_config)); + sizeof(struct virtio_vsock_config), errp); if (ret < 0) { - error_setg_errno(errp, -ret, "get config space failed"); goto err_vhost_dev; } diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c index 889559d86a..1ac4a2ebec 100644 --- a/hw/virtio/vhost-user.c +++ b/hw/virtio/vhost-user.c @@ -2117,7 +2117,7 @@ static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled) } static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config, - uint32_t config_len) + uint32_t config_len, Error **errp) { VhostUserMsg msg = { .hdr.request = VHOST_USER_GET_CONFIG, @@ -2127,32 +2127,32 @@ static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config, if (!virtio_has_feature(dev->protocol_features, VHOST_USER_PROTOCOL_F_CONFIG)) { - return -1; + error_setg(errp, "VHOST_USER_PROTOCOL_F_CONFIG not supported"); + return -EINVAL; } - if (config_len > VHOST_USER_MAX_CONFIG_SIZE) { - return -1; - } + assert(config_len <= VHOST_USER_MAX_CONFIG_SIZE); msg.payload.config.offset = 0; msg.payload.config.size = config_len; if (vhost_user_write(dev, &msg, NULL, 0) < 0) { - return -1; + return -EPROTO; } if (vhost_user_read(dev, &msg) < 0) { - return -1; + return -EPROTO; } if (msg.hdr.request != VHOST_USER_GET_CONFIG) { - error_report("Received unexpected msg type. Expected %d received %d", - VHOST_USER_GET_CONFIG, msg.hdr.request); - return -1; + error_setg(errp, + "Received unexpected msg type. Expected %d received %d", + VHOST_USER_GET_CONFIG, msg.hdr.request); + return -EINVAL; } if (msg.hdr.size != VHOST_USER_CONFIG_HDR_SIZE + config_len) { - error_report("Received bad msg size."); - return -1; + error_setg(errp, "Received bad msg size."); + return -EINVAL; } memcpy(config, msg.payload.config.region, config_len); diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index 0f469f1823..4fa414feea 100644 --- a/hw/virtio/vhost-vdpa.c +++ b/hw/virtio/vhost-vdpa.c @@ -523,7 +523,7 @@ static int vhost_vdpa_set_config(struct vhost_dev *dev, const uint8_t *data, } static int vhost_vdpa_get_config(struct vhost_dev *dev, uint8_t *config, - uint32_t config_len) + uint32_t config_len, Error **errp) { struct vhost_vdpa_config *v_config; unsigned long config_size = offsetof(struct vhost_vdpa_config, buf); diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index c7f9d8bb06..e8f85a5d2d 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -1562,15 +1562,23 @@ void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits, } int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config, - uint32_t config_len) + uint32_t config_len, Error **errp) { + ERRP_GUARD(); + int ret; + assert(hdev->vhost_ops); if (hdev->vhost_ops->vhost_get_config) { - return hdev->vhost_ops->vhost_get_config(hdev, config, config_len); + ret = hdev->vhost_ops->vhost_get_config(hdev, config, config_len, errp); + if (ret < 0 && !*errp) { + error_setg_errno(errp, -ret, "vhost_get_config failed"); + } + return ret; } - return -1; + error_setg(errp, "vhost_get_config not implemented"); + return -ENOTSUP; } int vhost_dev_set_config(struct vhost_dev *hdev, const uint8_t *data, diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h index 728ebb0ed9..8475c5a29d 100644 --- a/include/hw/virtio/vhost-backend.h +++ b/include/hw/virtio/vhost-backend.h @@ -98,7 +98,7 @@ typedef int (*vhost_set_config_op)(struct vhost_dev *dev, const uint8_t *data, uint32_t offset, uint32_t size, uint32_t flags); typedef int (*vhost_get_config_op)(struct vhost_dev *dev, uint8_t *config, - uint32_t config_len); + uint32_t config_len, Error **errp); typedef int (*vhost_crypto_create_session_op)(struct vhost_dev *dev, void *session_info, diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h index 2d7aaad67b..045d0fd9f2 100644 --- a/include/hw/virtio/vhost.h +++ b/include/hw/virtio/vhost.h @@ -130,8 +130,8 @@ int vhost_net_set_backend(struct vhost_dev *hdev, struct vhost_vring_file *file); int vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t iova, int write); -int vhost_dev_get_config(struct vhost_dev *dev, uint8_t *config, - uint32_t config_len); +int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config, + uint32_t config_len, Error **errp); int vhost_dev_set_config(struct vhost_dev *dev, const uint8_t *data, uint32_t offset, uint32_t size, uint32_t flags); /* notifier callback in case vhost device config space changed From 415fc2940b1536061c904bf192e097c27d3a787b Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 9 Jun 2021 08:46:57 -0700 Subject: [PATCH 23/24] vhost-user-blk: Factor out vhost_user_blk_realize_connect() This function is the part that we will want to retry if the connection is lost during initialisation, so factor it out to keep the following patch simpler. The error path for vhost_dev_get_config() forgot disconnecting the chardev, add this while touching the code. Signed-off-by: Kevin Wolf Message-Id: <20210609154658.350308-7-kwolf@redhat.com> Reviewed-by: Stefano Garzarella Reviewed-by: Raphael Norwitz Signed-off-by: Kevin Wolf --- hw/block/vhost-user-blk.c | 48 ++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index 3770f715da..e49d2e4c83 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -423,6 +423,36 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event) } } +static int vhost_user_blk_realize_connect(VHostUserBlk *s, Error **errp) +{ + DeviceState *dev = &s->parent_obj.parent_obj; + int ret; + + s->connected = false; + + ret = qemu_chr_fe_wait_connected(&s->chardev, errp); + if (ret < 0) { + return ret; + } + + ret = vhost_user_blk_connect(dev, errp); + if (ret < 0) { + qemu_chr_fe_disconnect(&s->chardev); + return ret; + } + assert(s->connected); + + ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg, + sizeof(struct virtio_blk_config), errp); + if (ret < 0) { + qemu_chr_fe_disconnect(&s->chardev); + vhost_dev_cleanup(&s->dev); + return ret; + } + + return 0; +} + static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) { VirtIODevice *vdev = VIRTIO_DEVICE(dev); @@ -467,22 +497,10 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) s->inflight = g_new0(struct vhost_inflight, 1); s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues); - s->connected = false; - if (qemu_chr_fe_wait_connected(&s->chardev, errp) < 0) { - goto virtio_err; - } - - if (vhost_user_blk_connect(dev, errp) < 0) { - qemu_chr_fe_disconnect(&s->chardev); - goto virtio_err; - } - assert(s->connected); - - ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg, - sizeof(struct virtio_blk_config), errp); + ret = vhost_user_blk_realize_connect(s, errp); if (ret < 0) { - goto vhost_err; + goto virtio_err; } /* we're fully initialized, now we can operate, so add the handler */ @@ -491,8 +509,6 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) NULL, true); return; -vhost_err: - vhost_dev_cleanup(&s->dev); virtio_err: g_free(s->vhost_vqs); s->vhost_vqs = NULL; From a527e312b59ac382cb84af4b91f517a846f50705 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 9 Jun 2021 08:46:58 -0700 Subject: [PATCH 24/24] vhost-user-blk: Implement reconnection during realize Commit dabefdd6 removed code that was supposed to try reconnecting during .realize(), but actually just crashed and had several design problems. This adds the feature back without the crash in simple cases while also fixing some design problems: Reconnection is now only tried if there was a problem with the connection and not an error related to the content (which would fail again the same way in the next attempt). Reconnection is limited to three attempts (four with the initial attempt) so that we won't end up in an infinite loop if a problem is permanent. If the backend restarts three times in the very short time window of device initialisation, we have bigger problems and erroring out is the right course of action. In the case that a connection error occurs and we reconnect, the error message is printed using error_report_err(), but otherwise ignored. Signed-off-by: Kevin Wolf Message-Id: <20210609154658.350308-8-kwolf@redhat.com> Reviewed-by: Raphael Norwitz Signed-off-by: Kevin Wolf --- hw/block/vhost-user-blk.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index e49d2e4c83..ba13cb87e5 100644 --- a/hw/block/vhost-user-blk.c +++ b/hw/block/vhost-user-blk.c @@ -31,6 +31,8 @@ #include "sysemu/sysemu.h" #include "sysemu/runstate.h" +#define REALIZE_CONNECTION_RETRIES 3 + static const int user_feature_bits[] = { VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_SEG_MAX, @@ -455,8 +457,10 @@ static int vhost_user_blk_realize_connect(VHostUserBlk *s, Error **errp) static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) { + ERRP_GUARD(); VirtIODevice *vdev = VIRTIO_DEVICE(dev); VHostUserBlk *s = VHOST_USER_BLK(vdev); + int retries; int i, ret; if (!s->chardev.chr) { @@ -498,7 +502,17 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) s->inflight = g_new0(struct vhost_inflight, 1); s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues); - ret = vhost_user_blk_realize_connect(s, errp); + retries = REALIZE_CONNECTION_RETRIES; + assert(!*errp); + do { + if (*errp) { + error_prepend(errp, "Reconnecting after error: "); + error_report_err(*errp); + *errp = NULL; + } + ret = vhost_user_blk_realize_connect(s, errp); + } while (ret == -EPROTO && retries--); + if (ret < 0) { goto virtio_err; }