[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/1] block: update BlockDriverState's children i
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 1/1] block: update BlockDriverState's children in bdrv_set_backing_hd() |
Date: |
Tue, 7 Jul 2015 16:49:54 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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