From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54890) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YxZXw-00066M-Li for qemu-devel@nongnu.org; Wed, 27 May 2015 07:30:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YxZXv-0008Jr-Di for qemu-devel@nongnu.org; Wed, 27 May 2015 07:30:52 -0400 Date: Wed, 27 May 2015 13:30:44 +0200 From: Kevin Wolf Message-ID: <20150527113044.GC4669@noname.str.redhat.com> References: <1431105726-3682-1-git-send-email-kwolf@redhat.com> <1431105726-3682-9-git-send-email-kwolf@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1431105726-3682-9-git-send-email-kwolf@redhat.com> Subject: Re: [Qemu-devel] [PATCH 08/34] block: Add list of children to BlockDriverState List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-block@nongnu.org Cc: qemu-devel@nongnu.org, armbru@redhat.com, mreitz@redhat.com Am 08.05.2015 um 19:21 hat Kevin Wolf geschrieben: > This allows iterating over all children of a given BDS, not only > including bs->file and bs->backing_hd, but also driver-specific > ones like VMDK extents or Quorum children. > > Signed-off-by: Kevin Wolf > --- > block.c | 27 +++++++++++++++++++++++++++ > include/block/block_int.h | 8 ++++++++ > 2 files changed, 35 insertions(+) > > diff --git a/block.c b/block.c > index c4f0fb4..59f54ed 100644 > --- a/block.c > +++ b/block.c > @@ -1301,6 +1301,19 @@ out: > return ret; > } > > +static void bdrv_attach_child(BlockDriverState *parent_bs, > + BlockDriverState *child_bs, > + const BdrvChildRole *child_role) > +{ > + BdrvChild *child = g_new(BdrvChild, 1); > + *child = (BdrvChild) { > + .bs = child_bs, > + .role = child_role, > + }; > + > + QLIST_INSERT_HEAD(&parent_bs->children, child, next); > +} > + > /* > * Opens a disk image (raw, qcow2, vmdk, ...) > * > @@ -1353,6 +1366,9 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename, > return -ENODEV; > } > bdrv_ref(bs); > + if (child_role) { > + bdrv_attach_child(parent, bs, child_role); > + } > *pbs = bs; > return 0; > } > @@ -1495,6 +1511,10 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename, > goto close_and_fail; > } > > + if (child_role) { > + bdrv_attach_child(parent, bs, child_role); > + } > + > QDECREF(options); > *pbs = bs; > return 0; > @@ -1789,6 +1809,12 @@ void bdrv_close(BlockDriverState *bs) > notifier_list_notify(&bs->close_notifiers, bs); > > if (bs->drv) { > + BdrvChild *child, *next; > + > + QLIST_FOREACH_SAFE(child, &bs->children, next, next) { > + g_free(child); > + } Max already pointed out this place, but we both didn't see the real bug here: Without a QLIST_REMOVE(), we get use after free on the next open of this BDS. After the latest rebase, the floppy media change qtest ended up failing because of this. Who said that time invested in fdc is wasted? ;-) Kevin