Re: [PULL 14/53] block: apply COR-filter to block-stream jobs

[Max Reitz <mreitz@redhat.com>]

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.shinkevich@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.shinkevich@virtuozzo.com>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Message-Id: <20201216061703.70908-14-vsementsov@virtuozzo.com>
>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>> Signed-off-by: Max Reitz <mreitz@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.
> 
> I see, CID already marked false-positive?

Yes, I did that.

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