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

[Vladimir Sementsov-Ogievskiy]

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..
2. we can do probing now
3. we can at least check, if backing_file_str == 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
> @@ -2508,7 +2508,6 @@ void qmp_block_stream(bool has_job_id, const char 
> *job_id, const char *device,
>       BlockDriverState *base_bs = NULL;
>       AioContext *aio_context;
>       Error *local_err = NULL;
> -    const char *base_name = NULL;
>       int job_flags = JOB_DEFAULT;
>   
>       if (!has_on_error) {
> @@ -2536,7 +2535,6 @@ void qmp_block_stream(bool has_job_id, const char 
> *job_id, const char *device,
>               goto out;
>           }
>           assert(bdrv_get_aio_context(base_bs) == aio_context);
> -        base_name = base;
>       }
>   
>       if (has_base_node) {
> @@ -2551,7 +2549,6 @@ void qmp_block_stream(bool has_job_id, const char 
> *job_id, const char *device,
>           }
>           assert(bdrv_get_aio_context(base_bs) == aio_context);
>           bdrv_refresh_filename(base_bs);
> -        base_name = base_bs->filename;
>       }
>   
>       /* Check for op blockers in the whole chain between bs and base */
> @@ -2571,9 +2568,6 @@ void qmp_block_stream(bool has_job_id, const char 
> *job_id, const char *device,
>           goto out;
>       }
>   
> -    /* backing_file string overrides base bs filename */
> -    base_name = has_backing_file ? backing_file : base_name;
> -
>       if (has_auto_finalize && !auto_finalize) {
>           job_flags |= JOB_MANUAL_FINALIZE;
>       }
> @@ -2581,7 +2575,8 @@ void qmp_block_stream(bool has_job_id, const char 
> *job_id, const char *device,
>           job_flags |= JOB_MANUAL_DISMISS;
>       }
>   
> -    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.

>                    job_flags, has_speed ? speed : 0, on_error,
>                    filter_node_name, &local_err);
>       if (local_err) {


--
Best regards,
Vladimir