* [PATCH 0/5] Fix not white-listed copy-before-write
@ 2021-09-20 11:55 Vladimir Sementsov-Ogievskiy
2021-09-20 11:55 ` [PATCH 1/5] block: implement bdrv_new_open_driver_opts() Vladimir Sementsov-Ogievskiy
` (4 more replies)
0 siblings, 5 replies; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-20 11:55 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, hreitz, kwolf, vsementsov
Hi all!
As reported in https://bugzilla.redhat.com/show_bug.cgi?id=2004812
backup don't work when copy-before-write is not white-listed.
Yes, we do need copy-before-write filter for backup to work (like we
always use copy-on-read filter in block-stream).
The problem is that in bdrv_insert_node() (called to insert filters
internally) we use bdrv_open(), which does a lot of things we don't need
for internal node creation and among them check the white-list.
Backup job should of course work when copy-before-write is not
white-listed. As well, block-stream should work with not-white-listed
copy-on-read. White-list is for user, not for internal implementation.
Following Kevin's suggestion fix the problem by implementing a version
of bdrv_new_open_driver() that supports QDict of options, and use it
instead of bdrv_open().
Vladimir Sementsov-Ogievskiy (5):
block: implement bdrv_new_open_driver_opts()
block: bdrv_insert_node(): fix and improve error handling
block: bdrv_insert_node(): doc and style
block: bdrv_insert_node(): don't use bdrv_open()
iotests/image-fleecing: declare requirement of copy-before-write
include/block/block.h | 4 ++
block.c | 77 ++++++++++++++++++++-----
tests/qemu-iotests/tests/image-fleecing | 1 +
3 files changed, 68 insertions(+), 14 deletions(-)
--
2.29.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/5] block: implement bdrv_new_open_driver_opts()
2021-09-20 11:55 [PATCH 0/5] Fix not white-listed copy-before-write Vladimir Sementsov-Ogievskiy
@ 2021-09-20 11:55 ` Vladimir Sementsov-Ogievskiy
2021-09-20 11:55 ` [PATCH 2/5] block: bdrv_insert_node(): fix and improve error handling Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-20 11:55 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, hreitz, kwolf, vsementsov
Add version of bdrv_new_open_driver() that supports QDict options.
We'll use it in further commit.
Simply add one more argument to bdrv_new_open_driver() is worse, as
there are too many invocations of bdrv_new_open_driver() to update
then.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Suggested-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block.h | 4 ++++
block.c | 25 +++++++++++++++++++++----
2 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 740038a892..24b773e69c 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -383,6 +383,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
const char *bdref_key, Error **errp);
BlockDriverState *bdrv_open(const char *filename, const char *reference,
QDict *options, int flags, Error **errp);
+BlockDriverState *bdrv_new_open_driver_opts(BlockDriver *drv,
+ const char *node_name,
+ QDict *options, int flags,
+ Error **errp);
BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
int flags, Error **errp);
BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
diff --git a/block.c b/block.c
index 5ce08a79fd..917fb7faca 100644
--- a/block.c
+++ b/block.c
@@ -1604,16 +1604,26 @@ open_failed:
return ret;
}
-BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
- int flags, Error **errp)
+/*
+ * Create and open a block node.
+ *
+ * @options is a QDict of options to pass to the block drivers, or NULL for an
+ * empty set of options. The reference to the QDict belongs to the block layer
+ * after the call (even on failure), so if the caller intends to reuse the
+ * dictionary, it needs to use qobject_ref() before calling bdrv_open.
+ */
+BlockDriverState *bdrv_new_open_driver_opts(BlockDriver *drv,
+ const char *node_name,
+ QDict *options, int flags,
+ Error **errp)
{
BlockDriverState *bs;
int ret;
bs = bdrv_new();
bs->open_flags = flags;
- bs->explicit_options = qdict_new();
- bs->options = qdict_new();
+ bs->options = options ?: qdict_new();
+ bs->explicit_options = qdict_clone_shallow(bs->options);
bs->opaque = NULL;
update_options_from_flags(bs->options, flags);
@@ -1631,6 +1641,13 @@ BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
return bs;
}
+/* Create and open a block node. */
+BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
+ int flags, Error **errp)
+{
+ return bdrv_new_open_driver_opts(drv, node_name, NULL, flags, errp);
+}
+
QemuOptsList bdrv_runtime_opts = {
.name = "bdrv_common",
.head = QTAILQ_HEAD_INITIALIZER(bdrv_runtime_opts.head),
--
2.29.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/5] block: bdrv_insert_node(): fix and improve error handling
2021-09-20 11:55 [PATCH 0/5] Fix not white-listed copy-before-write Vladimir Sementsov-Ogievskiy
2021-09-20 11:55 ` [PATCH 1/5] block: implement bdrv_new_open_driver_opts() Vladimir Sementsov-Ogievskiy
@ 2021-09-20 11:55 ` Vladimir Sementsov-Ogievskiy
2021-09-20 11:55 ` [PATCH 3/5] block: bdrv_insert_node(): doc and style Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-20 11:55 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, hreitz, kwolf, vsementsov
- use ERRP_GUARD(): function calls error_prepend(), so it must use
ERRP_GUARD(), otherwise error_prepend() would not be called when
passed errp is error_fatal
- drop error propagation, handle return code instead
- for symmetry, do error_prepend() for the second failure
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/block.c b/block.c
index 917fb7faca..5d49188073 100644
--- a/block.c
+++ b/block.c
@@ -5122,8 +5122,9 @@ static void bdrv_delete(BlockDriverState *bs)
BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
int flags, Error **errp)
{
+ ERRP_GUARD();
+ int ret;
BlockDriverState *new_node_bs;
- Error *local_err = NULL;
new_node_bs = bdrv_open(NULL, NULL, node_options, flags, errp);
if (new_node_bs == NULL) {
@@ -5132,12 +5133,12 @@ BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
}
bdrv_drained_begin(bs);
- bdrv_replace_node(bs, new_node_bs, &local_err);
+ ret = bdrv_replace_node(bs, new_node_bs, errp);
bdrv_drained_end(bs);
- if (local_err) {
+ if (ret < 0) {
+ error_prepend(errp, "Could not replace node: ");
bdrv_unref(new_node_bs);
- error_propagate(errp, local_err);
return NULL;
}
--
2.29.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/5] block: bdrv_insert_node(): doc and style
2021-09-20 11:55 [PATCH 0/5] Fix not white-listed copy-before-write Vladimir Sementsov-Ogievskiy
2021-09-20 11:55 ` [PATCH 1/5] block: implement bdrv_new_open_driver_opts() Vladimir Sementsov-Ogievskiy
2021-09-20 11:55 ` [PATCH 2/5] block: bdrv_insert_node(): fix and improve error handling Vladimir Sementsov-Ogievskiy
@ 2021-09-20 11:55 ` Vladimir Sementsov-Ogievskiy
2021-09-20 11:55 ` [PATCH 4/5] block: bdrv_insert_node(): don't use bdrv_open() Vladimir Sementsov-Ogievskiy
2021-09-20 11:55 ` [PATCH 5/5] iotests/image-fleecing: declare requirement of copy-before-write Vladimir Sementsov-Ogievskiy
4 siblings, 0 replies; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-20 11:55 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, hreitz, kwolf, vsementsov
- options & flags is common pair for open-like functions, let's use it
- add a comment that specifies use of @options
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index 5d49188073..3a90407b83 100644
--- a/block.c
+++ b/block.c
@@ -5119,14 +5119,23 @@ static void bdrv_delete(BlockDriverState *bs)
g_free(bs);
}
-BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
+
+/*
+ * Replace @bs by newly created block node.
+ *
+ * @options is a QDict of options to pass to the block drivers, or NULL for an
+ * empty set of options. The reference to the QDict belongs to the block layer
+ * after the call (even on failure), so if the caller intends to reuse the
+ * dictionary, it needs to use qobject_ref() before calling bdrv_open.
+ */
+BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *options,
int flags, Error **errp)
{
ERRP_GUARD();
int ret;
BlockDriverState *new_node_bs;
- new_node_bs = bdrv_open(NULL, NULL, node_options, flags, errp);
+ new_node_bs = bdrv_open(NULL, NULL, options, flags, errp);
if (new_node_bs == NULL) {
error_prepend(errp, "Could not create node: ");
return NULL;
--
2.29.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 4/5] block: bdrv_insert_node(): don't use bdrv_open()
2021-09-20 11:55 [PATCH 0/5] Fix not white-listed copy-before-write Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2021-09-20 11:55 ` [PATCH 3/5] block: bdrv_insert_node(): doc and style Vladimir Sementsov-Ogievskiy
@ 2021-09-20 11:55 ` Vladimir Sementsov-Ogievskiy
2021-09-21 14:33 ` Kevin Wolf
2021-09-20 11:55 ` [PATCH 5/5] iotests/image-fleecing: declare requirement of copy-before-write Vladimir Sementsov-Ogievskiy
4 siblings, 1 reply; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-20 11:55 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, hreitz, kwolf, vsementsov
Use bdrv_new_open_driver_opts() instead of complicated bdrv_open().
Among other extra things bdrv_open() also check for white-listed
formats, which we don't want for internal node creation: currently
backup doesn't work when copy-before-write filter is not white-listed.
As well block-stream doesn't work when copy-on-read is not
white-listed.
Fixes: 751cec7a261adaf1145dc7adf6de7c9c084e5a0b
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2004812
Reported-by: Yanan Fu
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block.c | 32 +++++++++++++++++++++++++++-----
1 file changed, 27 insertions(+), 5 deletions(-)
diff --git a/block.c b/block.c
index 3a90407b83..a174801785 100644
--- a/block.c
+++ b/block.c
@@ -5134,11 +5134,29 @@ BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *options,
ERRP_GUARD();
int ret;
BlockDriverState *new_node_bs;
+ const char *drvname, *node_name;
+ BlockDriver *drv;
+
+ drvname = qdict_get_try_str(options, "driver");
+ if (!drvname) {
+ error_setg(errp, "driver is not specified");
+ goto fail;
+ }
+
+ drv = bdrv_find_format(drvname);
+ if (!drv) {
+ error_setg(errp, "Unknown driver: '%s'", drvname);
+ goto fail;
+ }
- new_node_bs = bdrv_open(NULL, NULL, options, flags, errp);
- if (new_node_bs == NULL) {
+ node_name = qdict_get_try_str(options, "node-name");
+
+ new_node_bs = bdrv_new_open_driver_opts(drv, node_name, options, flags,
+ errp);
+ options = NULL; /* bdrv_new_open_driver() eats options */
+ if (!new_node_bs) {
error_prepend(errp, "Could not create node: ");
- return NULL;
+ goto fail;
}
bdrv_drained_begin(bs);
@@ -5147,11 +5165,15 @@ BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *options,
if (ret < 0) {
error_prepend(errp, "Could not replace node: ");
- bdrv_unref(new_node_bs);
- return NULL;
+ goto fail;
}
return new_node_bs;
+
+fail:
+ qobject_unref(options);
+ bdrv_unref(new_node_bs);
+ return NULL;
}
/*
--
2.29.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 5/5] iotests/image-fleecing: declare requirement of copy-before-write
2021-09-20 11:55 [PATCH 0/5] Fix not white-listed copy-before-write Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2021-09-20 11:55 ` [PATCH 4/5] block: bdrv_insert_node(): don't use bdrv_open() Vladimir Sementsov-Ogievskiy
@ 2021-09-20 11:55 ` Vladimir Sementsov-Ogievskiy
4 siblings, 0 replies; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-20 11:55 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, hreitz, kwolf, vsementsov
Now test fails if copy-before-write is not white-listed.
Let's skip test instead.
Fixes: c0605985696a19ef034fa25d04f53f3b3b383896
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/tests/image-fleecing | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/qemu-iotests/tests/image-fleecing b/tests/qemu-iotests/tests/image-fleecing
index f6318492c6..35164e9036 100755
--- a/tests/qemu-iotests/tests/image-fleecing
+++ b/tests/qemu-iotests/tests/image-fleecing
@@ -28,6 +28,7 @@ from iotests import log, qemu_img, qemu_io, qemu_io_silent
iotests.script_initialize(
supported_fmts=['qcow2', 'qcow', 'qed', 'vmdk', 'vhdx', 'raw'],
supported_platforms=['linux'],
+ required_fmts=['copy-before-write'],
)
patterns = [('0x5d', '0', '64k'),
--
2.29.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 4/5] block: bdrv_insert_node(): don't use bdrv_open()
2021-09-20 11:55 ` [PATCH 4/5] block: bdrv_insert_node(): don't use bdrv_open() Vladimir Sementsov-Ogievskiy
@ 2021-09-21 14:33 ` Kevin Wolf
0 siblings, 0 replies; 7+ messages in thread
From: Kevin Wolf @ 2021-09-21 14:33 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: hreitz, qemu-devel, qemu-block
Am 20.09.2021 um 13:55 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Use bdrv_new_open_driver_opts() instead of complicated bdrv_open().
>
> Among other extra things bdrv_open() also check for white-listed
> formats, which we don't want for internal node creation: currently
> backup doesn't work when copy-before-write filter is not white-listed.
> As well block-stream doesn't work when copy-on-read is not
> white-listed.
>
> Fixes: 751cec7a261adaf1145dc7adf6de7c9c084e5a0b
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2004812
> Reported-by: Yanan Fu
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block.c | 32 +++++++++++++++++++++++++++-----
> 1 file changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/block.c b/block.c
> index 3a90407b83..a174801785 100644
> --- a/block.c
> +++ b/block.c
> @@ -5134,11 +5134,29 @@ BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *options,
> ERRP_GUARD();
> int ret;
> BlockDriverState *new_node_bs;
gcc tells me that this needs to be initialised now because of the
bdrv_unref() aftert the fail: label.
Fixed this up to be new_node_bs = NULL and applied the series to the
block branch, thanks.
Kevin
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-09-21 14:35 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-20 11:55 [PATCH 0/5] Fix not white-listed copy-before-write Vladimir Sementsov-Ogievskiy
2021-09-20 11:55 ` [PATCH 1/5] block: implement bdrv_new_open_driver_opts() Vladimir Sementsov-Ogievskiy
2021-09-20 11:55 ` [PATCH 2/5] block: bdrv_insert_node(): fix and improve error handling Vladimir Sementsov-Ogievskiy
2021-09-20 11:55 ` [PATCH 3/5] block: bdrv_insert_node(): doc and style Vladimir Sementsov-Ogievskiy
2021-09-20 11:55 ` [PATCH 4/5] block: bdrv_insert_node(): don't use bdrv_open() Vladimir Sementsov-Ogievskiy
2021-09-21 14:33 ` Kevin Wolf
2021-09-20 11:55 ` [PATCH 5/5] iotests/image-fleecing: declare requirement of copy-before-write Vladimir Sementsov-Ogievskiy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.