Re: [PATCH v15 09/13] stream: rework backing-file changing

[Max Reitz <mreitz@redhat.com>]

On 16.12.20 07:16, Vladimir Sementsov-Ogievskiy wrote:
> From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> 
> Stream in stream_prepare calls bdrv_change_backing_file() to change
> backing-file in the metadata of bs.
> 
> It may use either backing-file parameter given by user or just take
> filename of base on job start.
> 
> Backing file format is determined by base on job finish.
> 
> There are some problems with this design, we solve only two by this
> patch:
> 
> 1. Consider scenario with backing-file unset. Current concept of stream
> supports changing of the base during the job (we don't freeze link to
> the base). So, we should not save base filename at job start,
> 
>    - let's determine name of the base on job finish.
> 
> 2. Using direct base to determine filename and format is not very good:
> base node may be a filter, so its filename may be JSON, and format_name
> is not good for storing into qcow2 metadata as backing file format.
> 
>    - let's use unfiltered_base
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>    [vsementsov: change commit subject, change logic in stream_prepare]
> ---
>   block/stream.c | 9 +++++----
>   blockdev.c     | 8 +-------
>   2 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index 6e281c71ac..6a525a5edf 100644
> --- a/block/stream.c
> +++ b/block/stream.c

[...]

> @@ -73,10 +74,10 @@ static int stream_prepare(Job *job)
>   
>       if (bdrv_cow_child(unfiltered_bs)) {
>           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 (unfiltered_base) {
> +            base_id = s->backing_file_str ?: unfiltered_base->filename;
> +            if (unfiltered_base->drv) {
> +                base_fmt = unfiltered_base->drv->format_name;
>               }
>           }
>           bdrv_set_backing_hd(unfiltered_bs, base, &local_err);

I think I preferred the v14 behavior of not setting a backing file 
format if backing_file_str is nowhere to be found in the current backing 
chain.  (I just noticed, I had a typo in my reply to v14, though; the 
“continuing on with setting a backing_fmt” should have read “continuing 
on *without* setting a backing_fmt”...)

Anyway, this is still an improvement on the pre-patch behavior, so:

Reviewed-by: Max Reitz <mreitz@redhat.com>

(And as we discussed, the best would be for the user to specify a 
backing format through a yet-to-be-added option.)