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 <be...@igalia.com> > Cc: Kevin Wolf <kw...@redhat.com>
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
