From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39144) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XSvZd-0004Nb-0w for qemu-devel@nongnu.org; Sat, 13 Sep 2014 18:13:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XSvZW-0005cW-N3 for qemu-devel@nongnu.org; Sat, 13 Sep 2014 18:13:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:61051) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XSvZW-0005cR-FM for qemu-devel@nongnu.org; Sat, 13 Sep 2014 18:13:34 -0400 Message-ID: <5414C17E.1050201@redhat.com> Date: Sun, 14 Sep 2014 00:13:18 +0200 From: Max Reitz MIME-Version: 1.0 References: <1410620427-20089-1-git-send-email-armbru@redhat.com> <1410620427-20089-4-git-send-email-armbru@redhat.com> In-Reply-To: <1410620427-20089-4-git-send-email-armbru@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 03/23] block: Connect BlockBackend to BlockDriverState List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , qemu-devel@nongnu.org Cc: kwolf@redhat.com, stefanha@redhat.com, benoit.canet@nodalink.com On 13.09.2014 17:00, Markus Armbruster wrote: > The pointer from BlockBackend to BlockDriverState is a strong > reference, managed with bdrv_ref() / bdrv_unref(), the back-pointer is > a weak one. > > Convenience function blk_new_with_bs() creates a BlockBackend with its > BlockDriverState. Callers have to unref both. The commit after next > will relieve them of the need to unref the BlockDriverState. > > Complication: due to the silly way drive_del works, we need a way to > hide a BlockBackend, just like bdrv_make_anon(). To emphasize its > "special" status, give the function a suitably off-putting name: So you're trying to force people with a sense of aesthetics to solve this problem? I don't know why this isn't taught in Software Engineering in university, but it definitely should be. > blk_hide_on_behalf_of_do_drive_del(). Unfortunately, hiding turns the > BlockBackend's name into the empty string. Can't avoid that without > breaking the blk->bs->device_name equals blk->name invariant. > > Signed-off-by: Markus Armbruster > --- > block.c | 10 ++-- > block/block-backend.c | 70 ++++++++++++++++++++++- > blockdev.c | 26 +++------ > hw/block/xen_disk.c | 8 +-- > include/block/block_int.h | 2 + > include/sysemu/block-backend.h | 5 ++ > qemu-img.c | 125 +++++++++++++++++++---------------------- > qemu-io.c | 4 +- > qemu-nbd.c | 2 +- > 9 files changed, 152 insertions(+), 100 deletions(-) [snip] > diff --git a/block/block-backend.c b/block/block-backend.c > index 919dd4c..b118b38 100644 > --- a/block/block-backend.c > +++ b/block/block-backend.c > @@ -16,10 +16,11 @@ > struct BlockBackend { > char *name; > int refcnt; > + BlockDriverState *bs; > QTAILQ_ENTRY(BlockBackend) link; /* for blk_backends */ > }; > > -/* All the BlockBackends */ > +/* All the BlockBackends (except for hidden ones) */ > static QTAILQ_HEAD(, BlockBackend) blk_backends = > QTAILQ_HEAD_INITIALIZER(blk_backends); > > @@ -47,10 +48,43 @@ BlockBackend *blk_new(const char *name, Error **errp) > return blk; > } > > +/* > + * Create a new BlockBackend with a new BlockDriverState attached. > + * Both have a reference count of one. Caller owns *both* references. > + * TODO Let caller own only the BlockBackend reference > + * Otherwise just like blk_new(), which see. Could be my lack of profoundness in English, but I don't understand what "which see" is supposed to mean or how its grammar works. An imperative? > + */ > +BlockBackend *blk_new_with_bs(const char *name, Error **errp) > +{ > + BlockBackend *blk; > + BlockDriverState *bs; > + > + blk = blk_new(name, errp); > + if (!blk) { > + return NULL; > + } > + > + bs = bdrv_new_root(name, errp); > + if (!bs) { > + blk_unref(blk); > + return NULL; > + } > + > + blk->bs = bs; > + bs->blk = blk; > + return blk; > +} > + [snip] > diff --git a/blockdev.c b/blockdev.c > index 5873205..21f4c67 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -229,14 +229,7 @@ void drive_info_del(DriveInfo *dinfo) > qemu_opts_del(dinfo->opts); > } > > - /* > - * Hairy special case: if do_drive_del() has made dinfo->bdrv > - * anonymous, it also unref'ed the associated BlockBackend. > - */ > - if (dinfo->bdrv->device_name[0]) { > - blk_unref(blk_by_name(dinfo->bdrv->device_name)); > - } > - > + blk_unref(blk_by_name(dinfo->bdrv->device_name)); So if !device_name[0], the BB is hidden. Hidden BBs are removed from the BB list and therefore not returned by blk_by_name(). Therefore, the BB is leaked here. I guess this will be fixed up in a later patch, though... > g_free(dinfo->id); > QTAILQ_REMOVE(&drives, dinfo, next); > g_free(dinfo->serial); [snip] > diff --git a/qemu-nbd.c b/qemu-nbd.c > index ff95da6..fa8a7d0 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -689,7 +689,7 @@ int main(int argc, char **argv) > } > > blk = blk_new("hda", &error_abort); > - bs = bdrv_new_root("hda", &error_abort); > + bs = blk_bs(blk); Shouldn't that be a blk_new_with_bs() then, just like every other case? > srcpath = argv[optind]; > ret = bdrv_open(&bs, srcpath, NULL, NULL, flags, drv, &local_err); Max