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 <arm...@redhat.com>
---
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
