From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52252) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZAKWE-00082P-K4 for qemu-devel@nongnu.org; Wed, 01 Jul 2015 12:05:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZAKWA-00022j-BL for qemu-devel@nongnu.org; Wed, 01 Jul 2015 12:05:50 -0400 References: <0d86a074935ca6c1a0645e5d4af492c7cac12821.1435758248.git.berto@igalia.com> From: Max Reitz Message-ID: <55940FCC.1010206@redhat.com> Date: Wed, 1 Jul 2015 18:05:32 +0200 MIME-Version: 1.0 In-Reply-To: <0d86a074935ca6c1a0645e5d4af492c7cac12821.1435758248.git.berto@igalia.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/1] block: update BlockDriverState's children in bdrv_set_backing_hd() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia , qemu-devel@nongnu.org Cc: kwolf@redhat.com, qemu-block@nongnu.org, stefanha@redhat.com 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 > Cc: Kevin Wolf > --- > 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) >