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

[Kevin Wolf]

Am 01.07.2015 um 16:21 hat Alberto Garcia geschrieben:
> 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>

I think I have a fix for this as part of a larger series I've been
working on before I left for holidays. I intended to send it out before
that, but I couldn't get it finished in time.

http://repo.or.cz/w/qemu/kevin.git/shortlog/refs/heads/bdrv_swap
Commit cde08581 'block: Fix backing file child when modifying graph'

It seems cleaner to me than this patch, though I haven't tried yet
to split the series so that it could be applied to 2.4.

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

This looks wrong to me. Attaching a BDS as a backing file doesn't mean
that the (potentially explicitly set) options and flags for that BDS
should be changed. It's perfectly fine for children to not inherit
options from their parent.

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

In theory, it could be valid to attach the same BDS for two different
roles of the same parent.

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

For the same reason, BlockDriverState *child_bs is a bad interface. My
patches use BdrvChild *child instead.

Kevin