Re: [Qemu-devel] [PATCH for-2.4 5/5] block: Fix backing file child when modifying graph

[Max Reitz <mreitz@redhat.com>]

On 08.07.2015 21:36, Kevin Wolf wrote:
> This patch moves bdrv_attach_child() from the individual places that add
> a backing file to a BDS to bdrv_set_backing_hd(), which is called by all
> of them. It also adds bdrv_detach_child() there.
>
> For normal operation (starting with one backing file chain and not
> changing it until the topmost image is closed) and live snapshots, this
> constitutes no change in behaviour.
>
> For all other cases, this is a fix for the bug that the old backing file
> was still referenced as a child, and the new one wasn't referenced.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block.c                   | 5 +++--
>   include/block/block_int.h | 1 +
>   2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/block.c b/block.c
> index d5c9f03..d088ee0 100644
> --- a/block.c
> +++ b/block.c
> @@ -1141,6 +1141,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
>       if (bs->backing_hd) {
>           assert(bs->backing_blocker);
>           bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
> +        bdrv_detach_child(bs->backing_child);
>       } else if (backing_hd) {
>           error_setg(&bs->backing_blocker,
>                      "node is used as backing hd of '%s'",
> @@ -1151,8 +1152,10 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
>       if (!backing_hd) {
>           error_free(bs->backing_blocker);
>           bs->backing_blocker = NULL;
> +        bs->backing_child = NULL;

I'd prefer this to be directly below the bdrv_detach_child() call, but 
that's just a question of personal preference.

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

>           goto out;
>       }
> +    bs->backing_child = bdrv_attach_child(bs, backing_hd, &child_backing);
>       bs->open_flags &= ~BDRV_O_NO_BACKING;
>       pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename);
>       pstrcpy(bs->backing_format, sizeof(bs->backing_format),
> @@ -1236,7 +1239,6 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
>           goto free_exit;
>       }
>
> -    bdrv_attach_child(bs, backing_hd, &child_backing);
>       bdrv_set_backing_hd(bs, backing_hd);
>
>   free_exit:
> @@ -2171,7 +2173,6 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
>       /* The contents of 'tmp' will become bs_top, as we are
>        * swapping bs_new and bs_top contents. */
>       bdrv_set_backing_hd(bs_top, bs_new);
> -    bdrv_attach_child(bs_top, bs_new, &child_backing);
>   }
>
>   static void bdrv_delete(BlockDriverState *bs)
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 8996baf..2cc973c 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -379,6 +379,7 @@ struct BlockDriverState {
>       char exact_filename[PATH_MAX];
>
>       BlockDriverState *backing_hd;
> +    BdrvChild *backing_child;
>       BlockDriverState *file;
>
>       NotifierList close_notifiers;
>