11.12.2020 19:05, Max Reitz wrote:
On 04.12.20 23:07, Vladimir Sementsov-Ogievskiy wrote:
The code already don't freeze base node and we try to make it prepared
for the situation when base node is changed during the operation. In
other words, block-stream doesn't own base node.
Let's introduce a new interface which should replace the current one,
which will in better relations with the code. Specifying bottom node
instead of base, and requiring it to be non-filter gives us the
following benefits:
- drop difference between above_base and base_overlay, which will be
renamed to just bottom, when old interface dropped
- clean way to work with parallel streams/commits on the same backing
chain, which otherwise become a problem when we introduce a filter
for stream job
- cleaner interface. Nobody will surprised the fact that base node may
disappear during block-stream, when there is no word about "base" in
the interface.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
---
qapi/block-core.json | 8 +++--
include/block/block_int.h | 1 +
block/monitor/block-hmp-cmds.c | 3 +-
block/stream.c | 50 +++++++++++++++++++---------
blockdev.c | 61 ++++++++++++++++++++++++++++------
5 files changed, 94 insertions(+), 29 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 04055ef50c..5d6681a35d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2522,6 +2522,10 @@
# @base-node: the node name of the backing file.
# It cannot be set if @base is also set. (Since 2.8)
#
+# @bottom: the last node in the chain that should be streamed into
+# top. It cannot be set any of @base, @base-node or @backing-file
s/set any/set if any/
But what’s the problem with backing-file? The fact that specifying
backing-file means that stream will look for that filename in the backing chain
when the job is done (so if you use @bottom, we generally don’t want to rely on
the presence of any nodes below it)?
I just wanted to deprecate 'backing-file' together with base and base-node as a
next step. If user wants to set backing file unrelated to current
backing-chain, is it correct at all? It's a direct violation of what's going
on, and I doubt that other parts of Qemu working with backing-file are prepared
for such situation. User can do it by hand later.. Anyway, we'll have three
releases deprecation period for people to come and cry that this is a really
needed option, so we can support it later on demand.
(If so, I would have thought that we actually want the user to specify
backing-file so we don’t have to look down below @bottom to look for a
filename. Perhaps a @backing-fmt parameter would help.)
If we decide that 'backing-file' is really needed, than yes we should require backing-fmt
to be specified together with backing-file when using new "bottom" interface.
[...]
diff --git a/blockdev.c b/blockdev.c
index 70900f4f77..e0e19db88b 100644
--- a/blockdev.c
+++ b/blockdev.c
[...]
@@ -2551,8 +2567,33 @@ void qmp_block_stream(bool has_job_id, const char
*job_id, const char *device,
bdrv_refresh_filename(base_bs);
}
- /* Check for op blockers in the whole chain between bs and base */
- for (iter = bs; iter && iter != base_bs;
+ if (has_bottom) {
+ bottom_bs = bdrv_lookup_bs(NULL, bottom, errp);
+ if (!bottom_bs) {
+ goto out;
+ }
+ if (!bottom_bs->drv) {
+ error_setg(errp, "Node '%s' is not open", bottom);
+ goto out;
+ }
+ if (bottom_bs->drv->is_filter) {
+ error_setg(errp, "Node '%s' is filter, use non-filter node"
+ "as 'bottom'", bottom);
Missing a space between “node” and “as”. (Also, probably two articles, i.e.
“Node '%s' is a filter, use a non-filter node...”.)
The rest looks good to me, but I’m withholding my R-b because I haven’t
understood why using @bottom precludes giving @backing-file.
Max
--
Best regards,
Vladimir
