Re: [PATCH v12 13/14] stream: skip filters when writing backing file name to QCOW2 header

From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	qemu-block@nongnu.org
Cc: qemu-devel@nongnu.org, fam@euphon.net, stefanha@redhat.com,
	kwolf@redhat.com, mreitz@redhat.com, armbru@redhat.com,
	jsnow@redhat.com, eblake@redhat.com, den@openvz.org
Subject: Re: [PATCH v12 13/14] stream: skip filters when writing backing file name to QCOW2 header
Date: Tue, 27 Oct 2020 19:42:19 +0300	[thread overview]
Message-ID: <e751c20d-7cf0-6e31-80cd-1f6a827a5243@virtuozzo.com> (raw)
In-Reply-To: <5703e680-a731-2aec-33da-26c431f4b89b@virtuozzo.com>

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 now
>>> 3. 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?

> 
> 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.
>>
>> Andrey
>>
>>>>                    job_flags, has_speed ? speed : 0, on_error,
>>>>                    filter_node_name, &local_err);
>>>>       if (local_err) {
>>>>
>>>
>>>
> 
> 