Re: [PATCH v10 8/9] block: remove unused backing-file name parameter
On 07.10.2020 13:21, Vladimir Sementsov-Ogievskiy wrote:
29.09.2020 15:38, Andrey Shinkevich wrote:
The block stream QMP parameter backing-file is in use no more. It
designates a backing file name to set in QCOW2 image header after the
block stream job finished. The base file name is used instead.
Signed-off-by: Andrey Shinkevich
We can't just remove it without a deprecation period of three releases.
It has not been in use for a long. It's time.
So actually, in a previous patch, we should implement new behavior for
automatic backing-file detection if this parameter is unspecified. Amd
keep old behavior for backing-file-name if it is given.
Hmm. Or, probably, we can use direct base for base-filename? And in cases
when we should skip filters (for example of parallel jobs) user should
specify backing-file explicitly?
The backing_file_str is always specified if the base is specified and is
always equal to the base->filename. So, the user's backing file name is
always NULL for the stream job. Furthermore, it is not checked for being
the correct backing node and can lead to a wrong record in the QCOW2 header.
Andrey
---
block/monitor/block-hmp-cmds.c | 2 +-
block/stream.c | 6 +-
blockdev.c | 17 +
include/block/block_int.h | 2 +-
qapi/block-core.json | 17 +
5 files changed, 5 insertions(+), 39 deletions(-)
diff --git a/block/monitor/block-hmp-cmds.c
b/block/monitor/block-hmp-cmds.c
index 4e66775..5f19499 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -506,7 +506,7 @@ void hmp_block_stream(Monitor *mon, const QDict
*qdict)
int64_t speed = qdict_get_try_int(qdict, "speed", 0);
qmp_block_stream(true, device, device, base != NULL, base,
false, NULL,
- false, NULL, qdict_haskey(qdict, "speed"),
speed, true,
+ qdict_haskey(qdict, "speed"), speed, true,
BLOCKDEV_ON_ERROR_REPORT, false, NULL, false,
false, false,
false, );
diff --git a/block/stream.c b/block/stream.c
index b0719e9..fe2663f 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -34,7 +34,6 @@ typedef struct StreamBlockJob {
BlockDriverState *base_overlay; /* COW overlay (stream from
this) */
BlockDriverState *above_base; /* Node directly above the base */
BlockdevOnError on_error;
- char *backing_file_str;
bool bs_read_only;
bool chain_frozen;
} StreamBlockJob;
@@ -103,8 +102,6 @@ static void stream_clean(Job *job)
blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, _abort);
bdrv_reopen_set_read_only(bs, true, NULL);
}
-
- g_free(s->backing_file_str);
}
static int coroutine_fn stream_run(Job *job, Error **errp)
@@ -220,7 +217,7 @@ static const BlockJobDriver stream_job_driver = {
};
void stream_start(const char *job_id, BlockDriverState *bs,
- BlockDriverState *base, const char *backing_file_str,
+ BlockDriverState *base,
int creation_flags, int64_t speed,
BlockdevOnError on_error,
const char *filter_node_name,
@@ -295,7 +292,6 @@ void stream_start(const char *job_id,
BlockDriverState *bs,
s->base_overlay = base_overlay;
s->above_base = above_base;
- s->backing_file_str = g_strdup(backing_file_str);
s->bs_read_only = bs_read_only;
s->chain_frozen = true;
diff --git a/blockdev.c b/blockdev.c
index d719c47..b223601 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2486,7 +2486,6 @@ out:
void qmp_block_stream(bool has_job_id, const char *job_id, const
char *device,
bool has_base, const char *base,
bool has_base_node, const char *base_node,
- bool has_backing_file, const char *backing_file,
bool has_speed, int64_t speed,
bool has_on_error, BlockdevOnError on_error,
bool has_filter_node_name, const char
*filter_node_name,
@@ -2498,7 +2497,6 @@ void qmp_block_stream(bool has_job_id, const
char *job_id, const char *device,
BlockDriverState *base_bs = NULL;
AioContext *aio_context;
Error *local_err = NULL;
- const char *base_name = NULL;
int job_flags = JOB_DEFAULT;
if (!has_on_error) {
@@ -2526,7 +2524,6 @@ void qmp_block_stream(bool has_job_id, const
char *job_id, const char *device,
goto out;
}
assert(bdrv_get_aio_context(base_bs) == aio_context);
- base_name = base;
}
if (has_base_node) {
@@ -2541,7 +2538,6 @@ void qmp_block_stream(bool has_job_id, const
char *job_id, const char *device,
}
assert(bdrv_get_aio_context(base_bs) == aio_context);
bdrv_refresh_filename(base_bs);
- base_name = base_bs->filename;
}
/* Check for op blockers
Re: [PATCH v10 8/9] block: remove unused backing-file name parameter
29.09.2020 15:38, Andrey Shinkevich wrote:
The block stream QMP parameter backing-file is in use no more. It
designates a backing file name to set in QCOW2 image header after the
block stream job finished. The base file name is used instead.
Signed-off-by: Andrey Shinkevich
We can't just remove it without a deprecation period of three releases.
So actually, in a previous patch, we should implement new behavior for
automatic backing-file detection if this parameter is unspecified. Amd
keep old behavior for backing-file-name if it is given.
Hmm. Or, probably, we can use direct base for base-filename? And in cases
when we should skip filters (for example of parallel jobs) user should
specify backing-file explicitly?
---
block/monitor/block-hmp-cmds.c | 2 +-
block/stream.c | 6 +-
blockdev.c | 17 +
include/block/block_int.h | 2 +-
qapi/block-core.json | 17 +
5 files changed, 5 insertions(+), 39 deletions(-)
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 4e66775..5f19499 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -506,7 +506,7 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
int64_t speed = qdict_get_try_int(qdict, "speed", 0);
qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
- false, NULL, qdict_haskey(qdict, "speed"), speed, true,
+ qdict_haskey(qdict, "speed"), speed, true,
BLOCKDEV_ON_ERROR_REPORT, false, NULL, false, false,
false,
false, );
diff --git a/block/stream.c b/block/stream.c
index b0719e9..fe2663f 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -34,7 +34,6 @@ typedef struct StreamBlockJob {
BlockDriverState *base_overlay; /* COW overlay (stream from this) */
BlockDriverState *above_base; /* Node directly above the base */
BlockdevOnError on_error;
-char *backing_file_str;
bool bs_read_only;
bool chain_frozen;
} StreamBlockJob;
@@ -103,8 +102,6 @@ static void stream_clean(Job *job)
blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, _abort);
bdrv_reopen_set_read_only(bs, true, NULL);
}
-
-g_free(s->backing_file_str);
}
static int coroutine_fn stream_run(Job *job, Error **errp)
@@ -220,7 +217,7 @@ static const BlockJobDriver stream_job_driver = {
};
void stream_start(const char *job_id, BlockDriverState *bs,
- BlockDriverState *base, const char *backing_file_str,
+ BlockDriverState *base,
int creation_flags, int64_t speed,
BlockdevOnError on_error,
const char *filter_node_name,
@@ -295,7 +292,6 @@ void stream_start(const char *job_id, BlockDriverState *bs,
s->base_overlay = base_overlay;
s->above_base = above_base;
-s->backing_file_str = g_strdup(backing_file_str);
s->bs_read_only = bs_read_only;
s->chain_frozen = true;
diff --git a/blockdev.c b/blockdev.c
index d719c47..b223601 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2486,7 +2486,6 @@ out:
void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
bool has_base, const char *base,
bool has_base_node, const char *base_node,
- bool has_backing_file, const char *backing_file,
bool has_speed, int64_t speed,
bool has_on_error, BlockdevOnError on_error,
bool has_filter_node_name, const char *filter_node_name,
@@ -2498,7 +2497,6 @@ void qmp_block_stream(bool has_job_id, const char
*job_id, const char *device,
BlockDriverState *base_bs = NULL;
AioContext *aio_context;
Error *local_err = NULL;
-const char *base_name = NULL;
int job_flags = JOB_DEFAULT;
if (!has_on_error) {
@@ -2526,7 +2524,6 @@ void qmp_block_stream(bool has_job_id, const char
*job_id, const char *device,
goto out;
}
assert(bdrv_get_aio_context(base_bs) == aio_context);
-base_name = base;
}
if (has_base_node) {
@@ -2541,7 +2538,6 @@ void qmp_block_stream(bool has_job_id, const char
*job_id, const char *device,
}
assert(bdrv_get_aio_context(base_bs) == aio_context);
bdrv_refresh_filename(base_bs);
-base_name = base_bs->filename;
}
/* Check for op blockers in the whole chain between bs and base */
@@ -2553,17 +2549,6 @@ void qmp_block_stream(bool has_job_id, const char
*job_id, const char *device,
}
}
-/* if we are streaming the entire chain, the result will have no backing
- * file, and specifying one is therefore an error */
-if (base_bs == NULL && has_backing_file) {
-error_setg(errp, "backing file
[PATCH v10 8/9] block: remove unused backing-file name parameter
The block stream QMP parameter backing-file is in use no more. It
designates a backing file name to set in QCOW2 image header after the
block stream job finished. The base file name is used instead.
Signed-off-by: Andrey Shinkevich
---
block/monitor/block-hmp-cmds.c | 2 +-
block/stream.c | 6 +-
blockdev.c | 17 +
include/block/block_int.h | 2 +-
qapi/block-core.json | 17 +
5 files changed, 5 insertions(+), 39 deletions(-)
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 4e66775..5f19499 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -506,7 +506,7 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
int64_t speed = qdict_get_try_int(qdict, "speed", 0);
qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
- false, NULL, qdict_haskey(qdict, "speed"), speed, true,
+ qdict_haskey(qdict, "speed"), speed, true,
BLOCKDEV_ON_ERROR_REPORT, false, NULL, false, false,
false,
false, );
diff --git a/block/stream.c b/block/stream.c
index b0719e9..fe2663f 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -34,7 +34,6 @@ typedef struct StreamBlockJob {
BlockDriverState *base_overlay; /* COW overlay (stream from this) */
BlockDriverState *above_base; /* Node directly above the base */
BlockdevOnError on_error;
-char *backing_file_str;
bool bs_read_only;
bool chain_frozen;
} StreamBlockJob;
@@ -103,8 +102,6 @@ static void stream_clean(Job *job)
blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, _abort);
bdrv_reopen_set_read_only(bs, true, NULL);
}
-
-g_free(s->backing_file_str);
}
static int coroutine_fn stream_run(Job *job, Error **errp)
@@ -220,7 +217,7 @@ static const BlockJobDriver stream_job_driver = {
};
void stream_start(const char *job_id, BlockDriverState *bs,
- BlockDriverState *base, const char *backing_file_str,
+ BlockDriverState *base,
int creation_flags, int64_t speed,
BlockdevOnError on_error,
const char *filter_node_name,
@@ -295,7 +292,6 @@ void stream_start(const char *job_id, BlockDriverState *bs,
s->base_overlay = base_overlay;
s->above_base = above_base;
-s->backing_file_str = g_strdup(backing_file_str);
s->bs_read_only = bs_read_only;
s->chain_frozen = true;
diff --git a/blockdev.c b/blockdev.c
index d719c47..b223601 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2486,7 +2486,6 @@ out:
void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
bool has_base, const char *base,
bool has_base_node, const char *base_node,
- bool has_backing_file, const char *backing_file,
bool has_speed, int64_t speed,
bool has_on_error, BlockdevOnError on_error,
bool has_filter_node_name, const char *filter_node_name,
@@ -2498,7 +2497,6 @@ void qmp_block_stream(bool has_job_id, const char
*job_id, const char *device,
BlockDriverState *base_bs = NULL;
AioContext *aio_context;
Error *local_err = NULL;
-const char *base_name = NULL;
int job_flags = JOB_DEFAULT;
if (!has_on_error) {
@@ -2526,7 +2524,6 @@ void qmp_block_stream(bool has_job_id, const char
*job_id, const char *device,
goto out;
}
assert(bdrv_get_aio_context(base_bs) == aio_context);
-base_name = base;
}
if (has_base_node) {
@@ -2541,7 +2538,6 @@ void qmp_block_stream(bool has_job_id, const char
*job_id, const char *device,
}
assert(bdrv_get_aio_context(base_bs) == aio_context);
bdrv_refresh_filename(base_bs);
-base_name = base_bs->filename;
}
/* Check for op blockers in the whole chain between bs and base */
@@ -2553,17 +2549,6 @@ void qmp_block_stream(bool has_job_id, const char
*job_id, const char *device,
}
}
-/* if we are streaming the entire chain, the result will have no backing
- * file, and specifying one is therefore an error */
-if (base_bs == NULL && has_backing_file) {
-error_setg(errp, "backing file specified, but streaming the "
- "entire chain");
-goto out;
-}
-
-/* backing_file string overrides base bs filename */
-base_name = has_backing_file ? backing_file : base_name;
-
if (has_auto_finalize && !auto_finalize) {
job_flags |= JOB_MANUAL_FINALIZE;
}
@@ -2571,7 +2556,7 @@ void qmp_block_stream(bool has_job_id, const char
*job_id, const char *device,
job_flags |= JOB_MANUAL_DISMISS;
}
-stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
+stream_start(has_job_id ?
