|
From: | Vladimir Sementsov-Ogievskiy |
Subject: | Re: [PATCH v12 13/14] stream: skip filters when writing backing file name to QCOW2 header |
Date: | Tue, 27 Oct 2020 19:44:12 +0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.4.0 |
27.10.2020 19:42, Andrey Shinkevich wrote:
On 27.10.2020 19:21, Vladimir Sementsov-Ogievskiy wrote:27.10.2020 19:01, Andrey Shinkevich wrote:On 27.10.2020 18:09, Vladimir Sementsov-Ogievskiy wrote:22.10.2020 21:13, Andrey Shinkevich wrote:Avoid writing a filter JSON file name and a filter format name to QCOW2 image when the backing file is changed after the block stream job. A user is still able to assign the 'backing-file' parameter for a block-stream job keeping in mind the possible issue mentioned above. If the user does not specify the 'backing-file' parameter, QEMU will assign it automatically. Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> --- block/stream.c | 15 +++++++++++++-- blockdev.c | 9 ++------- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/block/stream.c b/block/stream.c index e0540ee..1ba74ab 100644 --- a/block/stream.c +++ b/block/stream.c @@ -65,6 +65,7 @@ static int stream_prepare(Job *job) BlockDriverState *bs = blk_bs(bjob->blk); BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs); BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base); + BlockDriverState *base_unfiltered = NULL; Error *local_err = NULL; int ret = 0; @@ -75,8 +76,18 @@ static int stream_prepare(Job *job) const char *base_id = NULL, *base_fmt = NULL; if (base) { base_id = s->backing_file_str; - if (base->drv) { - base_fmt = base->drv->format_name; + if (base_id) { + if (base->drv) { + base_fmt = base->drv->format_name;hmm. this doesn't make real sense: so, we assume that user specified backing_file_str, which may not relate to base, but we use base->drv->format_name? But it may be name of the filter driver, which would be wrong.. Any ideas? 1. we can use base_fmt=NULL, to provoke probing on next open of the qcow2 file..I would choose this item #1 but have to check the probing code logic... Particularly, I do not remember now if the probing is able to recognize a protocol. The logic for the format_name in the QEMU existent code (I has kept it here in the patch) is a slippery way for an imprudent user. That's why I staked on the backing_file_str deprication in the previous version.2. we can do probing now3. we can at least check, if backing_file_str ==Not bad for the sanity check but we will search a node by the file name again - not good ((Not search, but only check one very likely option.Yes, just strcmp(). And why a user may not merely specify a desired backing file as the base?
*shrung*
Additionally to 1. or 3. (or combined), or even keeping things as is (i.e. wrong, but it is preexisting), we can: - add backing-format argument to qapi as pair for backing-file - deprecate using backing-file without backing-format. Then, after deprecation period we'll have correct code. This may be done in separate.base_unfiltered->filename, in this case we can use base_unfiltered->drv->format_name+ } + } else { + base_unfiltered = bdrv_skip_filters(base); + if (base_unfiltered) { + base_id = base_unfiltered->filename; + if (base_unfiltered->drv) { + base_fmt = base_unfiltered->drv->format_name; + } + } } } bdrv_set_backing_hd(unfiltered_bs, base, &local_err); diff --git a/blockdev.c b/blockdev.c index c917625..0e9c783 100644 --- a/blockdev.c +++ b/blockdev.c[...]- stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name, + stream_start(has_job_id ? job_id : NULL, bs, base_bs, + has_backing_file ? backing_file : NULL,backing_file should be NULL if has_backing_file is false, so you can use just backing_file instead of ternary operator.Yes, if reliable. I has kept the conformation with the ternary operator at the first parameter above. Andreyjob_flags, has_speed ? speed : 0, on_error, filter_node_name, &local_err); if (local_err) {
-- Best regards, Vladimir
[Prev in Thread] | Current Thread | [Next in Thread] |