On 1/29/21 9:23 AM, Max Reitz wrote: > On 29.01.21 06:26, Vladimir Sementsov-Ogievskiy wrote: >> 28.01.2021 21:38, Philippe Mathieu-Daudé wrote: >>> Hi Andrey, >>> >>> On 1/26/21 3:19 PM, Max Reitz wrote: >>>> From: Andrey Shinkevich <andrey.shinkev...@virtuozzo.com> >>>> >>>> This patch completes the series with the COR-filter applied to >>>> block-stream operations. >>>> >>>> Adding the filter makes it possible in future implement discarding >>>> copied regions in backing files during the block-stream job, to reduce >>>> the disk overuse (we need control on permissions). >>>> >>>> Also, the filter now is smart enough to do copy-on-read with specified >>>> base, so we have benefit on guest reads even when doing block-stream of >>>> the part of the backing chain. >>>> >>>> Several iotests are slightly modified due to filter insertion. >>>> >>>> Signed-off-by: Andrey Shinkevich <andrey.shinkev...@virtuozzo.com> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> >>>> Message-Id: <20201216061703.70908-14-vsement...@virtuozzo.com> >>>> Reviewed-by: Max Reitz <mre...@redhat.com> >>>> Signed-off-by: Max Reitz <mre...@redhat.com> >>>> --- >>>> block/stream.c | 105 >>>> ++++++++++++++++++++++--------------- >>>> tests/qemu-iotests/030 | 8 +-- >>>> tests/qemu-iotests/141.out | 2 +- >>>> tests/qemu-iotests/245 | 20 ++++--- >>>> 4 files changed, 80 insertions(+), 55 deletions(-) >>>> >>>> diff --git a/block/stream.c b/block/stream.c >>> ... >>>> @@ -228,7 +211,9 @@ void stream_start(const char *job_id, >>>> BlockDriverState *bs, >>>> bool bs_read_only; >>>> int basic_flags = BLK_PERM_CONSISTENT_READ | >>>> BLK_PERM_WRITE_UNCHANGED; >>>> BlockDriverState *base_overlay; >>>> + BlockDriverState *cor_filter_bs = NULL; >>>> BlockDriverState *above_base; >>>> + QDict *opts; >>>> assert(!(base && bottom)); >>>> assert(!(backing_file_str && bottom)); >>>> @@ -266,30 +251,62 @@ void stream_start(const char *job_id, >>>> BlockDriverState *bs, >>>> } >>>> } >>>> - if (bdrv_freeze_backing_chain(bs, above_base, errp) < 0) { >>>> - return; >>>> - } >>>> - >>>> /* Make sure that the image is opened in read-write mode */ >>>> bs_read_only = bdrv_is_read_only(bs); >>>> if (bs_read_only) { >>>> - if (bdrv_reopen_set_read_only(bs, false, errp) != 0) { >>>> - bs_read_only = false; >>>> - goto fail; >>>> + int ret; >>>> + /* Hold the chain during reopen */ >>>> + if (bdrv_freeze_backing_chain(bs, above_base, errp) < 0) { >>>> + return; >>>> + } >>>> + >>>> + ret = bdrv_reopen_set_read_only(bs, false, errp); >>>> + >>>> + /* failure, or cor-filter will hold the chain */ >>>> + bdrv_unfreeze_backing_chain(bs, above_base); >>>> + >>>> + if (ret < 0) { >>>> + return; >>>> } >>>> } >>>> - /* Prevent concurrent jobs trying to modify the graph structure >>>> here, we >>>> - * already have our own plans. Also don't allow resize as the >>>> image size is >>>> - * queried only at the job start and then cached. */ >>>> - s = block_job_create(job_id, &stream_job_driver, NULL, bs, >>>> - basic_flags | BLK_PERM_GRAPH_MOD, >>>> + opts = qdict_new(); >>> >>> Coverity reported (CID 1445793) that this resource could be leaked... >>> >>>> + >>>> + qdict_put_str(opts, "driver", "copy-on-read"); >>>> + qdict_put_str(opts, "file", bdrv_get_node_name(bs)); >>>> + /* Pass the base_overlay node name as 'bottom' to COR driver */ >>>> + qdict_put_str(opts, "bottom", base_overlay->node_name); >>>> + if (filter_node_name) { >>>> + qdict_put_str(opts, "node-name", filter_node_name); >>>> + } >>>> + >>>> + cor_filter_bs = bdrv_insert_node(bs, opts, BDRV_O_RDWR, errp); >>>> + if (!cor_filter_bs) { >>> >>> ... probably here. >>> >>> Should we call g_free(opts) here? >> >> >> Actually, not.. >> >> bdrv_insert_node() calls bdrv_open() which eats options even on failure.
Ah OK. >> >> I see, CID already marked false-positive? > > Yes, I did that. Thanks Max! > > This isn’t the first time Coverity has reported a failed bdrv_open() > call would leak the options QDict. Perhaps someone™ should look into > why it likes to thinks that, but so far I haven’t been sufficiently > bothered by it. > > Max >