From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59114) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XRevG-00008Q-Ct for qemu-devel@nongnu.org; Wed, 10 Sep 2014 06:15:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XReuu-0005MX-6t for qemu-devel@nongnu.org; Wed, 10 Sep 2014 06:14:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55337) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XReut-0005Jd-Ms for qemu-devel@nongnu.org; Wed, 10 Sep 2014 06:14:24 -0400 Date: Wed, 10 Sep 2014 12:14:18 +0200 From: Kevin Wolf Message-ID: <20140910101418.GA844@noname.str.redhat.com> References: <1410336832-22160-1-git-send-email-armbru@redhat.com> <1410336832-22160-6-git-send-email-armbru@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1410336832-22160-6-git-send-email-armbru@redhat.com> Subject: Re: [Qemu-devel] [PATCH 05/23] block: Make BlockBackend own its BlockDriverState List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: benoit.canet@irqsave.net, famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com Am 10.09.2014 um 10:13 hat Markus Armbruster geschrieben: > On BlockBackend destruction, unref its BlockDriverState. Replaces the > callers' unrefs. > > Signed-off-by: Markus Armbruster > --- > block/block-backend.c | 9 ++------- > blockdev.c | 11 +++-------- > hw/block/xen_disk.c | 6 +++--- > qemu-img.c | 35 +---------------------------------- > qemu-io.c | 5 ----- > 5 files changed, 9 insertions(+), 57 deletions(-) > > diff --git a/block/block-backend.c b/block/block-backend.c > index 2a22660..ae51f7f 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -58,10 +58,7 @@ BlockBackend *blk_new(const char *name, Error **errp) > * @errp: return location for an error to be set on failure, or %NULL > * > * Create a new BlockBackend, with a reference count of one, and > - * attach a new BlockDriverState to it, also with a reference count of > - * one. Caller owns *both* references. > - * TODO Let caller own only the BlockBackend reference > - * Fail if @name already exists. > + * a new BlockDriverState attached. Fail if @name already exists. > * > * Returns: the BlockBackend on success, %NULL on error > */ > @@ -88,6 +85,7 @@ BlockBackend *blk_new_with_bs(const char *name, Error **errp) > static void blk_delete(BlockBackend *blk) > { > assert(!blk->refcnt); > + bdrv_unref(blk->bs); > blk_detach_bs(blk); I think the bdrv_unref() should really be part of blk_detach_bs(). The same way it would be more logical to have bdrv_ref() as part of blk_attach_bs(). For blk_new_with_bs() this might mean bdrv_new, blk_attach_bs, bdrv_unref, which looks a bit odd, but if blk_attach_bs() is ever called from somewhere else, it probably makes more sense (if it isn't, it should be static). Kevin