Re: [Qemu-devel] [PATCH for-4.0 2/3] block: freeze the backing chain earlier in stream_start()

[Vladimir Sementsov-Ogievskiy]

26.03.2019 20:07, Alberto Garcia wrote:
> Commit 6585493369819a48d34a86d57ec6b97cb5cd9bc0 added code to freeze
> the backing chain from 'top' to 'base' for the duration of the
> block-stream job.
> 
> The problem is that the freezing happens too late in stream_start():
> during the bdrv_reopen_set_read_only() call earlier in that function
> another job can jump in and remove the base image. If that happens we
> have an invalid chain and QEMU crashes.
> 
> This patch puts the bdrv_freeze_backing_chain() call at the beginning
> of the function.
> 
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>   block/stream.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index 6253c86fae..6502468f88 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -238,11 +238,16 @@ void stream_start(const char *job_id, BlockDriverState 
> *bs,
>       BlockDriverState *iter;
>       bool bs_read_only;
>   
> +    if (bdrv_freeze_backing_chain(bs, 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) {
> -            return;
> +            bs_read_only = false;
> +            goto fail;
>           }
>       }
>   
> @@ -269,11 +274,6 @@ void stream_start(const char *job_id, BlockDriverState 
> *bs,
>                              &error_abort);
>       }
>   
> -    if (bdrv_freeze_backing_chain(bs, base, errp) < 0) {
> -        job_early_fail(&s->common.job);
> -        goto fail;
> -    }
> -
>       s->base = base;
>       s->backing_file_str = g_strdup(backing_file_str);
>       s->bs_read_only = bs_read_only;
> @@ -285,6 +285,7 @@ void stream_start(const char *job_id, BlockDriverState 
> *bs,
>       return;
>   
>   fail:
> +    bdrv_unfreeze_backing_chain(bs, base);
>       if (bs_read_only) {
>           bdrv_reopen_set_read_only(bs, true, NULL);
>       }
> 

A bit nicer, if rollback in reverse order, unfreeze at last.

Anyway,
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

-- 
Best regards,
Vladimir