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

From: Max Reitz <mreitz@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	qemu-block@nongnu.org
Cc: fam@euphon.net, kwolf@redhat.com, qemu-devel@nongnu.org,
	armbru@redhat.com, stefanha@redhat.com,
	andrey.shinkevich@virtuozzo.com, den@openvz.org,
	jsnow@redhat.com
Subject: Re: [PATCH v14 09/13] stream: skip filters when writing backing file name to QCOW2 header
Date: Fri, 11 Dec 2020 16:15:57 +0100	[thread overview]
Message-ID: <a55e0557-2b86-b2ee-b9a9-406da522b53c@redhat.com> (raw)
In-Reply-To: <20201204220758.2879-10-vsementsov@virtuozzo.com>

On 04.12.20 23:07, Vladimir Sementsov-Ogievskiy wrote:
> From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> 
> Avoid writing a filter JSON file name and a filter format name to QCOW2
> image when the backing file is being changed after the block stream
> job. It can occur due to a concurrent commit job on the same backing
> chain.
> 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>
>   [vsementsov: use unfiltered_bs for bdrv_find_backing_image()]
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/stream.c | 21 +++++++++++++++++++--
>   blockdev.c     |  8 +-------
>   2 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index 6e281c71ac..c208393c34 100644
> --- a/block/stream.c
> +++ b/block/stream.c

[...]

> @@ -75,8 +78,22 @@ 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) {
> +                backing_bs = bdrv_find_backing_image(unfiltered_bs, base_id);
> +                if (backing_bs && backing_bs->drv) {
> +                    base_fmt = backing_bs->drv->format_name;
> +                } else {
> +                    error_report("Format not found for backing file %s",
> +                                 s->backing_file_str);

I think it’s actually going to be rather likely that we’re not going to 
find the backing file here.  If the user were to use a filename that 
just appears as-such in the backing chain, they wouldn’t need to specify 
a backing-file parameter at all, because the one figured out 
automatically would be just fine.

But then again, what are we supposed to do then.  We can continue as 
before, which is to just use the base node’s format.  But if the user 
wants to perhaps use a backing file that isn’t even open in qemu (a copy 
of the the base on some different storage), we have no idea what format 
it’s in.

So printing an error here, but continuing on with setting a backing_fmt 
is probably the most reasonable thing to do indeed.

> +                }
> +            } else {
> +                base_unfiltered = bdrv_skip_filters(base);
> +                if (base_unfiltered) {

@base_unfiltered cannot be NULL here (because @base is sure not to be 
NULL).  Of course, double-checking isn’t wrong, it just looks a bit 
weird, because it seems to imply that we might end up with a case where 
base != NULL, but base_id == NULL.  Anyway:

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

> +                    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);