| Message ID | 1601383109-110988-9-git-send-email-andrey.shinkevich@virtuozzo.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Apply COR-filter to the block-stream permanently | expand |
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 <andrey.shinkevich@virtuozzo.com> 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, &error); > > 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, &error_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 ? job_id : NULL, bs, base_bs, > job_flags, has_speed ? speed : 0, on_error, > filter_node_name, &local_err); > if (local_err) { > diff --git a/include/block/block_int.h b/include/block/block_int.h > index f782737..bbe2ee6 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -1147,7 +1147,7 @@ int is_windows_drive(const char *filename); > * BlockDriverState. > */ > 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, > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 32fb097..6358606 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -2512,21 +2512,6 @@ > # @base-node: the node name of the backing file. > # It cannot be set if @base is also set. (Since 2.8) > # > -# @backing-file: The backing file string to write into the top > -# image. This filename is not validated. > -# > -# If a pathname string is such that it cannot be > -# resolved by QEMU, that means that subsequent QMP or > -# HMP commands must use node-names for the image in > -# question, as filename lookup methods will fail. > -# > -# If not specified, QEMU will automatically determine > -# the backing file string to use, or error out if there > -# is no obvious choice. Care should be taken when > -# specifying the string, to specify a valid filename or > -# protocol. > -# (Since 2.1) > -# > # @speed: the maximum speed, in bytes per second > # > # @on-error: the action to take on an error (default report). > @@ -2566,7 +2551,7 @@ > ## > { 'command': 'block-stream', > 'data': { '*job-id': 'str', 'device': 'str', '*base': 'str', > - '*base-node': 'str', '*backing-file': 'str', '*speed': 'int', > + '*base-node': 'str', '*speed': 'int', > '*on-error': 'BlockdevOnError', > '*filter-node-name': 'str', > '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } } > -- Best regards, Vladimir
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 <andrey.shinkevich@virtuozzo.com> > > 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, &error); >> 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, &error_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 ? job_id : NULL, bs, base_bs, >> job_flags, has_speed ? speed : 0, on_error, >> filter_node_name, &local_err); >> if (local_err) { >> diff --git a/include/block/block_int.h b/include/block/block_int.h >> index f782737..bbe2ee6 100644 >> --- a/include/block/block_int.h >> +++ b/include/block/block_int.h >> @@ -1147,7 +1147,7 @@ int is_windows_drive(const char *filename); >> * BlockDriverState. >> */ >> 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, >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index 32fb097..6358606 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -2512,21 +2512,6 @@ >> # @base-node: the node name of the backing file. >> # It cannot be set if @base is also set. (Since 2.8) >> # >> -# @backing-file: The backing file string to write into the top >> -# image. This filename is not validated. >> -# >> -# If a pathname string is such that it cannot be >> -# resolved by QEMU, that means that subsequent QMP or >> -# HMP commands must use node-names for the image in >> -# question, as filename lookup methods will fail. >> -# >> -# If not specified, QEMU will automatically determine >> -# the backing file string to use, or error out if there >> -# is no obvious choice. Care should be taken when >> -# specifying the string, to specify a valid filename or >> -# protocol. >> -# (Since 2.1) >> -# >> # @speed: the maximum speed, in bytes per second >> # >> # @on-error: the action to take on an error (default report). >> @@ -2566,7 +2551,7 @@ >> ## >> { 'command': 'block-stream', >> 'data': { '*job-id': 'str', 'device': 'str', '*base': 'str', >> - '*base-node': 'str', '*backing-file': 'str', '*speed': >> 'int', >> + '*base-node': 'str', '*speed': 'int', >> '*on-error': 'BlockdevOnError', >> '*filter-node-name': 'str', >> '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } } >> > >
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, &error); 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, &error_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 ? job_id : NULL, bs, base_bs, job_flags, has_speed ? speed : 0, on_error, filter_node_name, &local_err); if (local_err) { diff --git a/include/block/block_int.h b/include/block/block_int.h index f782737..bbe2ee6 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -1147,7 +1147,7 @@ int is_windows_drive(const char *filename); * BlockDriverState. */ 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, diff --git a/qapi/block-core.json b/qapi/block-core.json index 32fb097..6358606 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -2512,21 +2512,6 @@ # @base-node: the node name of the backing file. # It cannot be set if @base is also set. (Since 2.8) # -# @backing-file: The backing file string to write into the top -# image. This filename is not validated. -# -# If a pathname string is such that it cannot be -# resolved by QEMU, that means that subsequent QMP or -# HMP commands must use node-names for the image in -# question, as filename lookup methods will fail. -# -# If not specified, QEMU will automatically determine -# the backing file string to use, or error out if there -# is no obvious choice. Care should be taken when -# specifying the string, to specify a valid filename or -# protocol. -# (Since 2.1) -# # @speed: the maximum speed, in bytes per second # # @on-error: the action to take on an error (default report). @@ -2566,7 +2551,7 @@ ## { 'command': 'block-stream', 'data': { '*job-id': 'str', 'device': 'str', '*base': 'str', - '*base-node': 'str', '*backing-file': 'str', '*speed': 'int', + '*base-node': 'str', '*speed': 'int', '*on-error': 'BlockdevOnError', '*filter-node-name': 'str', '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
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 <andrey.shinkevich@virtuozzo.com> --- 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(-)