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

[Philippe Mathieu-Daudé]

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.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.

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
>