Re: [Qemu-devel] [PATCH 1/1] block: update BlockDriverState's children in bdrv_set_backing_hd()

[Max Reitz]

On 01.07.2015 16:21, Alberto Garcia wrote:
> When a backing image is opened using bdrv_open_inherit(), it is added
> to the parent image's list of children. However there's no way to
> remove it from there.
>
> In particular, changing a BlockDriverState's backing image does not
> add the new one to the list nor removes the old one. If the latter is
> closed then the pointer in the list becomes invalid. This can be
> reproduced easily using the block-stream command.
>
> Signed-off-by: Alberto Garcia <address@hidden>
> Cc: Kevin Wolf <address@hidden>
> ---
>   block.c | 40 ++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/block.c b/block.c
> index 7e130cc..eaf3ad0 100644
> --- a/block.c
> +++ b/block.c
> @@ -88,6 +88,13 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const 
> char *filename,
>                                const BdrvChildRole *child_role,
>                                BlockDriver *drv, Error **errp);
>
> +static void bdrv_attach_child(BlockDriverState *parent_bs,
> +                              BlockDriverState *child_bs,
> +                              const BdrvChildRole *child_role);
> +
> +static void bdrv_detach_child(BlockDriverState *parent_bs,
> +                              BlockDriverState *child_bs);
> +
>   static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
>   /* If non-zero, use only whitelisted block drivers */
>   static int use_bdrv_whitelist;
> @@ -1108,6 +1115,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, bs->backing_hd);
>       } else if (backing_hd) {
>           error_setg(&bs->backing_blocker,
>                      "node is used as backing hd of '%s'",
> @@ -1120,6 +1128,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
> BlockDriverState *backing_hd)
>           bs->backing_blocker = NULL;
>           goto out;
>       }
> +
> +    bdrv_attach_child(bs, backing_hd, &child_backing);
> +    backing_hd->inherits_from = bs;
> +    backing_hd->open_flags = child_backing.inherit_flags(bs->open_flags);

Do we really want this, unconditionally? ... After looking through the 
code, I can't find a place where we wouldn't. It just seems strange to 
have it here.

> +
>       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),
> @@ -1332,7 +1345,16 @@ static void bdrv_attach_child(BlockDriverState 
> *parent_bs,
>                                 BlockDriverState *child_bs,
>                                 const BdrvChildRole *child_role)
>   {
> -    BdrvChild *child = g_new(BdrvChild, 1);
> +    BdrvChild *child;
> +
> +    /* Don't attach the child if it's already attached */
> +    QLIST_FOREACH(child, &parent_bs->children, next) {
> +        if (child->bs == child_bs) {
> +            return;
> +        }
> +    }

Hm, it may have been attached with a different role, though... I guess 
that would be a bug, however. But if it's the same role, trying to 
re-attach it seems wrong, too. So where could this happen?

Max

> +
> +    child = g_new(BdrvChild, 1);
>       *child = (BdrvChild) {
>           .bs     = child_bs,
>           .role   = child_role,
> @@ -1341,6 +1363,21 @@ static void bdrv_attach_child(BlockDriverState 
> *parent_bs,
>       QLIST_INSERT_HEAD(&parent_bs->children, child, next);
>   }
>
> +static void bdrv_detach_child(BlockDriverState *parent_bs,
> +                              BlockDriverState *child_bs)
> +{
> +    BdrvChild *child, *next_child;
> +    QLIST_FOREACH_SAFE(child, &parent_bs->children, next, next_child) {
> +        if (child->bs == child_bs) {
> +            if (child->bs->inherits_from == parent_bs) {
> +                child->bs->inherits_from = NULL;
> +            }
> +            QLIST_REMOVE(child, next);
> +            g_free(child);
> +        }
> +    }
> +}
> +
>   /*
>    * Opens a disk image (raw, qcow2, vmdk, ...)
>    *
> @@ -2116,7 +2153,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)