* [PULL 01/24] Prevent compiler warning on block.c
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
@ 2021-06-30 16:01 ` Kevin Wolf
2021-06-30 16:01 ` [PULL 02/24] block: Move read-only check during truncation earlier Kevin Wolf
` (23 subsequent siblings)
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:01 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Miroslav Rezanina <mrezanin@redhat.com>
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 <mrezanin@redhat.com>
Message-Id: <1162368493.17178530.1620201543649.JavaMail.zimbra@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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) {
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 02/24] block: Move read-only check during truncation earlier
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
2021-06-30 16:01 ` [PULL 01/24] Prevent compiler warning on block.c Kevin Wolf
@ 2021-06-30 16:01 ` Kevin Wolf
2021-06-30 16:01 ` [PULL 03/24] block: BDRV_O_NO_IO for backing file on creation Kevin Wolf
` (22 subsequent siblings)
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:01 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Eric Blake <eblake@redhat.com>
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 <eblake@redhat.com>
Message-Id: <20210609163034.997943-1-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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) {
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 03/24] block: BDRV_O_NO_IO for backing file on creation
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
2021-06-30 16:01 ` [PULL 01/24] Prevent compiler warning on block.c Kevin Wolf
2021-06-30 16:01 ` [PULL 02/24] block: Move read-only check during truncation earlier Kevin Wolf
@ 2021-06-30 16:01 ` Kevin Wolf
2021-06-30 16:01 ` [PULL 04/24] block: rename bdrv_replace_child to bdrv_replace_child_tran Kevin Wolf
` (21 subsequent siblings)
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:01 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Max Reitz <mreitz@redhat.com>
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 <mreitz@redhat.com>
Message-Id: <20210622140030.212487-1-mreitz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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 =="
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 04/24] block: rename bdrv_replace_child to bdrv_replace_child_tran
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
` (2 preceding siblings ...)
2021-06-30 16:01 ` [PULL 03/24] block: BDRV_O_NO_IO for backing file on creation Kevin Wolf
@ 2021-06-30 16:01 ` Kevin Wolf
2021-06-30 16:01 ` [PULL 05/24] block: comment graph-modifying function not updating permissions Kevin Wolf
` (20 subsequent siblings)
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:01 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
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 <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-Id: <20210610112618.127378-2-vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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;
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 05/24] block: comment graph-modifying function not updating permissions
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
` (3 preceding siblings ...)
2021-06-30 16:01 ` [PULL 04/24] block: rename bdrv_replace_child to bdrv_replace_child_tran Kevin Wolf
@ 2021-06-30 16:01 ` Kevin Wolf
2021-06-30 16:01 ` [PULL 06/24] block: introduce bdrv_remove_file_or_backing_child() Kevin Wolf
` (19 subsequent siblings)
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:01 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-Id: <20210610112618.127378-3-vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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)
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 06/24] block: introduce bdrv_remove_file_or_backing_child()
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
` (4 preceding siblings ...)
2021-06-30 16:01 ` [PULL 05/24] block: comment graph-modifying function not updating permissions Kevin Wolf
@ 2021-06-30 16:01 ` Kevin Wolf
2021-06-30 16:01 ` [PULL 07/24] block: introduce bdrv_set_file_or_backing_noperm() Kevin Wolf
` (18 subsequent siblings)
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:01 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To be used for reopen in future commit.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20210610120537.196183-2-vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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,
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 07/24] block: introduce bdrv_set_file_or_backing_noperm()
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
` (5 preceding siblings ...)
2021-06-30 16:01 ` [PULL 06/24] block: introduce bdrv_remove_file_or_backing_child() Kevin Wolf
@ 2021-06-30 16:01 ` Kevin Wolf
2021-06-30 16:01 ` [PULL 08/24] block: bdrv_reopen_parse_backing(): don't check aio context Kevin Wolf
` (17 subsequent siblings)
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:01 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
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 <vsementsov@virtuozzo.com>
Message-Id: <20210610120537.196183-3-vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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)
{
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 08/24] block: bdrv_reopen_parse_backing(): don't check aio context
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
` (6 preceding siblings ...)
2021-06-30 16:01 ` [PULL 07/24] block: introduce bdrv_set_file_or_backing_noperm() Kevin Wolf
@ 2021-06-30 16:01 ` Kevin Wolf
2021-06-30 16:01 ` [PULL 09/24] block: bdrv_reopen_parse_backing(): don't check frozen child Kevin Wolf
` (16 subsequent siblings)
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:01 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
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 <vsementsov@virtuozzo.com>
Message-Id: <20210610120537.196183-4-vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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)
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 09/24] block: bdrv_reopen_parse_backing(): don't check frozen child
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
` (7 preceding siblings ...)
2021-06-30 16:01 ` [PULL 08/24] block: bdrv_reopen_parse_backing(): don't check aio context Kevin Wolf
@ 2021-06-30 16:01 ` Kevin Wolf
2021-06-30 16:01 ` [PULL 10/24] block: bdrv_reopen_parse_backing(): simplify handling implicit filters Kevin Wolf
` (15 subsequent siblings)
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:01 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
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 <vsementsov@virtuozzo.com>
Message-Id: <20210610120537.196183-5-vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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')
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 10/24] block: bdrv_reopen_parse_backing(): simplify handling implicit filters
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
` (8 preceding siblings ...)
2021-06-30 16:01 ` [PULL 09/24] block: bdrv_reopen_parse_backing(): don't check frozen child Kevin Wolf
@ 2021-06-30 16:01 ` Kevin Wolf
2021-06-30 16:01 ` [PULL 11/24] block: move supports_backing check to bdrv_set_file_or_backing_noperm() Kevin Wolf
` (14 subsequent siblings)
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:01 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
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 <vsementsov@virtuozzo.com>
Message-Id: <20210610120537.196183-6-vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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);
}
/*
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 11/24] block: move supports_backing check to bdrv_set_file_or_backing_noperm()
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
` (9 preceding siblings ...)
2021-06-30 16:01 ` [PULL 10/24] block: bdrv_reopen_parse_backing(): simplify handling implicit filters Kevin Wolf
@ 2021-06-30 16:01 ` Kevin Wolf
2021-06-30 16:01 ` [PULL 12/24] block: BDRVReopenState: drop replace_backing_bs field Kevin Wolf
` (13 subsequent siblings)
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:01 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
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 <vsementsov@virtuozzo.com>
Message-Id: <20210610120537.196183-7-vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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,
};
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 12/24] block: BDRVReopenState: drop replace_backing_bs field
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
` (10 preceding siblings ...)
2021-06-30 16:01 ` [PULL 11/24] block: move supports_backing check to bdrv_set_file_or_backing_noperm() Kevin Wolf
@ 2021-06-30 16:01 ` Kevin Wolf
2021-06-30 16:01 ` [PULL 13/24] block: Allow changing bs->file on reopen Kevin Wolf
` (12 subsequent siblings)
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:01 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
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 <vsementsov@virtuozzo.com>
Message-Id: <20210610120537.196183-8-vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block.h | 1 -
block.c | 10 ++++------
2 files changed, 4 insertions(+), 7 deletions(-)
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;
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);
}
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 13/24] block: Allow changing bs->file on reopen
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
` (11 preceding siblings ...)
2021-06-30 16:01 ` [PULL 12/24] block: BDRVReopenState: drop replace_backing_bs field Kevin Wolf
@ 2021-06-30 16:01 ` Kevin Wolf
2021-06-30 16:01 ` [PULL 14/24] iotests: Test replacing files with x-blockdev-reopen Kevin Wolf
` (11 subsequent siblings)
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:01 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Alberto Garcia <berto@igalia.com>
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 <berto@igalia.com>
[vsementsov: bdrv_reopen_parse_file_or_backing() is modified a lot]
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20210610120537.196183-9-vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block.h | 1 +
block.c | 78 +++++++++++++++++++++++++++++-------------
tests/qemu-iotests/245 | 23 +++++++------
3 files changed, 67 insertions(+), 35 deletions(-)
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/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/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)
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 14/24] iotests: Test replacing files with x-blockdev-reopen
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
` (12 preceding siblings ...)
2021-06-30 16:01 ` [PULL 13/24] block: Allow changing bs->file on reopen Kevin Wolf
@ 2021-06-30 16:01 ` Kevin Wolf
2021-06-30 16:01 ` [PULL 15/24] introduce QEMU_AUTO_VFREE Kevin Wolf
` (10 subsequent siblings)
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:01 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Alberto Garcia <berto@igalia.com>
This patch adds new tests in which we use x-blockdev-reopen to change
bs->file
Signed-off-by: Alberto Garcia <berto@igalia.com>
Message-Id: <20210610120537.196183-10-vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 15/24] introduce QEMU_AUTO_VFREE
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
` (13 preceding siblings ...)
2021-06-30 16:01 ` [PULL 14/24] iotests: Test replacing files with x-blockdev-reopen Kevin Wolf
@ 2021-06-30 16:01 ` Kevin Wolf
2021-06-30 16:01 ` [PULL 16/24] block/commit: use QEMU_AUTO_VFREE Kevin Wolf
` (9 subsequent siblings)
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:01 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Introduce a convenient macro, that works for qemu_memalign() like
g_autofree works with g_malloc.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20210628121133.193984-2-vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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().
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 16/24] block/commit: use QEMU_AUTO_VFREE
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
` (14 preceding siblings ...)
2021-06-30 16:01 ` [PULL 15/24] introduce QEMU_AUTO_VFREE Kevin Wolf
@ 2021-06-30 16:01 ` Kevin Wolf
2021-06-30 16:01 ` [PULL 17/24] block/ssh: add support for sha256 host key fingerprints Kevin Wolf
` (8 subsequent siblings)
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:01 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20210628121133.193984-3-vsementsov@virtuozzo.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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);
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 17/24] block/ssh: add support for sha256 host key fingerprints
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
` (15 preceding siblings ...)
2021-06-30 16:01 ` [PULL 16/24] block/commit: use QEMU_AUTO_VFREE Kevin Wolf
@ 2021-06-30 16:01 ` Kevin Wolf
2021-06-30 16:02 ` [PULL 18/24] vhost: Add Error parameter to vhost_dev_init() Kevin Wolf
` (7 subsequent siblings)
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:01 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
From: Daniel P. Berrangé <berrange@redhat.com>
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é <berrange@redhat.com>
Message-Id: <20210622115156.138458-1-berrange@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Acked-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qapi/block-core.json | 3 ++-
block/ssh.c | 3 +++
tests/qemu-iotests/207 | 54 ++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/207.out | 25 ++++++++++++++++++
4 files changed, 84 insertions(+), 1 deletion(-)
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/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/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}}}
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 18/24] vhost: Add Error parameter to vhost_dev_init()
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
` (16 preceding siblings ...)
2021-06-30 16:01 ` [PULL 17/24] block/ssh: add support for sha256 host key fingerprints Kevin Wolf
@ 2021-06-30 16:02 ` Kevin Wolf
2021-06-30 16:02 ` [PULL 19/24] vhost: Distinguish errors in vhost_backend_init() Kevin Wolf
` (6 subsequent siblings)
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:02 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
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 <kwolf@redhat.com>
Message-Id: <20210609154658.350308-2-kwolf@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/hw/virtio/vhost.h | 2 +-
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 ++++++++++------
11 files changed, 29 insertions(+), 25 deletions(-)
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);
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;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 19/24] vhost: Distinguish errors in vhost_backend_init()
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
` (17 preceding siblings ...)
2021-06-30 16:02 ` [PULL 18/24] vhost: Add Error parameter to vhost_dev_init() Kevin Wolf
@ 2021-06-30 16:02 ` Kevin Wolf
2021-06-30 16:02 ` [PULL 20/24] vhost: Return 0/-errno in vhost_dev_init() Kevin Wolf
` (5 subsequent siblings)
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:02 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
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 <kwolf@redhat.com>
Message-Id: <20210609154658.350308-3-kwolf@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/hw/virtio/vhost-backend.h | 3 ++-
hw/virtio/vhost-backend.c | 2 +-
hw/virtio/vhost-user.c | 41 ++++++++++++++++---------------
hw/virtio/vhost-vdpa.c | 2 +-
hw/virtio/vhost.c | 13 +++++-----
5 files changed, 32 insertions(+), 29 deletions(-)
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);
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;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 20/24] vhost: Return 0/-errno in vhost_dev_init()
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
` (18 preceding siblings ...)
2021-06-30 16:02 ` [PULL 19/24] vhost: Distinguish errors in vhost_backend_init() Kevin Wolf
@ 2021-06-30 16:02 ` Kevin Wolf
2021-06-30 16:02 ` [PULL 21/24] vhost-user-blk: Add Error parameter to vhost_user_blk_start() Kevin Wolf
` (4 subsequent siblings)
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:02 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
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 <kwolf@redhat.com>
Message-Id: <20210609154658.350308-4-kwolf@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 21/24] vhost-user-blk: Add Error parameter to vhost_user_blk_start()
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
` (19 preceding siblings ...)
2021-06-30 16:02 ` [PULL 20/24] vhost: Return 0/-errno in vhost_dev_init() Kevin Wolf
@ 2021-06-30 16:02 ` Kevin Wolf
2021-06-30 16:02 ` [PULL 22/24] vhost: Distinguish errors in vhost_dev_get_config() Kevin Wolf
` (3 subsequent siblings)
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:02 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
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 <kwolf@redhat.com>
Message-Id: <20210609154658.350308-5-kwolf@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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;
}
}
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 22/24] vhost: Distinguish errors in vhost_dev_get_config()
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
` (20 preceding siblings ...)
2021-06-30 16:02 ` [PULL 21/24] vhost-user-blk: Add Error parameter to vhost_user_blk_start() Kevin Wolf
@ 2021-06-30 16:02 ` Kevin Wolf
2021-06-30 16:02 ` [PULL 23/24] vhost-user-blk: Factor out vhost_user_blk_realize_connect() Kevin Wolf
` (2 subsequent siblings)
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:02 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
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 <kwolf@redhat.com>
Message-Id: <20210609154658.350308-6-kwolf@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/hw/virtio/vhost-backend.h | 2 +-
include/hw/virtio/vhost.h | 4 ++--
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 +++++++++++---
10 files changed, 46 insertions(+), 32 deletions(-)
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
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,
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 23/24] vhost-user-blk: Factor out vhost_user_blk_realize_connect()
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
` (21 preceding siblings ...)
2021-06-30 16:02 ` [PULL 22/24] vhost: Distinguish errors in vhost_dev_get_config() Kevin Wolf
@ 2021-06-30 16:02 ` Kevin Wolf
2021-06-30 16:02 ` [PULL 24/24] vhost-user-blk: Implement reconnection during realize Kevin Wolf
2021-07-02 13:52 ` [PULL 00/24] Block layer patches Peter Maydell
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:02 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
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 <kwolf@redhat.com>
Message-Id: <20210609154658.350308-7-kwolf@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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;
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PULL 24/24] vhost-user-blk: Implement reconnection during realize
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
` (22 preceding siblings ...)
2021-06-30 16:02 ` [PULL 23/24] vhost-user-blk: Factor out vhost_user_blk_realize_connect() Kevin Wolf
@ 2021-06-30 16:02 ` Kevin Wolf
2021-07-02 13:52 ` [PULL 00/24] Block layer patches Peter Maydell
24 siblings, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2021-06-30 16:02 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, peter.maydell, qemu-devel
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 <kwolf@redhat.com>
Message-Id: <20210609154658.350308-8-kwolf@redhat.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
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;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PULL 00/24] Block layer patches
2021-06-30 16:01 [PULL 00/24] Block layer patches Kevin Wolf
` (23 preceding siblings ...)
2021-06-30 16:02 ` [PULL 24/24] vhost-user-blk: Implement reconnection during realize Kevin Wolf
@ 2021-07-02 13:52 ` Peter Maydell
24 siblings, 0 replies; 26+ messages in thread
From: Peter Maydell @ 2021-07-02 13:52 UTC (permalink / raw)
To: Kevin Wolf; +Cc: QEMU Developers, Qemu-block
On Wed, 30 Jun 2021 at 17:02, Kevin Wolf <kwolf@redhat.com> wrote:
>
> The following changes since commit 13d5f87cc3b94bfccc501142df4a7b12fee3a6e7:
>
> Merge remote-tracking branch 'remotes/rth-gitlab/tags/pull-axp-20210628' into staging (2021-06-29 10:02:42 +0100)
>
> are available in the Git repository at:
>
> git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to a527e312b59ac382cb84af4b91f517a846f50705:
>
> vhost-user-blk: Implement reconnection during realize (2021-06-30 13:21:22 +0200)
>
> ----------------------------------------------------------------
> Block layer patches
>
> - Supporting changing 'file' in x-blockdev-reopen
> - ssh: add support for sha256 host key fingerprints
> - vhost-user-blk: Implement reconnection during realize
> - introduce QEMU_AUTO_VFREE
> - Don't require password of encrypted backing file for image creation
> - Code cleanups
Applied, thanks.
Please update the changelog at https://wiki.qemu.org/ChangeLog/6.1
for any user-visible changes.
-- PMM
^ permalink raw reply [flat|nested] 26+ messages in thread