Re: [Qemu-devel] [PATCH v2 03/23] block: Connect BlockBackend to BlockDriverState
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); Is a blk_new_with_bs converssion missing here ? -bs = bdrv_new_root(hda, error_abort); +bs = blk_bs(blk); srcpath = argv[optind]; ret = bdrv_open(bs, srcpath, NULL, NULL, flags, drv, local_err); -- 1.9.3
Re: [Qemu-devel] [PATCH v2 03/23] block: Connect BlockBackend to BlockDriverState
Benoît Canet benoit.ca...@nodalink.com writes: 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); Is a blk_new_with_bs converssion missing here ? Yes. Max spotted it, too :) -bs = bdrv_new_root(hda, error_abort); +bs = blk_bs(blk); srcpath = argv[optind]; ret = bdrv_open(bs, srcpath, NULL, NULL, flags, drv, local_err); -- 1.9.3
[Qemu-devel] [PATCH v2 03/23] block: Connect BlockBackend to BlockDriverState
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:
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(-)
diff --git a/block.c b/block.c
index a05d0e3..92f84d2 100644
--- a/block.c
+++ b/block.c
@@ -2032,7 +2032,7 @@ static void bdrv_move_feature_fields(BlockDriverState
*bs_dest,
* This will modify the BlockDriverState fields, and swap contents
* between bs_new and bs_old. Both bs_new and bs_old are modified.
*
- * bs_new is required to be anonymous.
+ * bs_new must be nameless and not attached to a BlockBackend.
*
* This function does not create any image files.
*/
@@ -2051,8 +2051,9 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState
*bs_old)
QTAILQ_REMOVE(graph_bdrv_states, bs_old, node_list);
}
-/* bs_new must be anonymous and shouldn't have anything fancy enabled */
+/* bs_new must be nameless and shouldn't have anything fancy enabled */
assert(bs_new-device_name[0] == '\0');
+assert(!bs_new-blk);
assert(QLIST_EMPTY(bs_new-dirty_bitmaps));
assert(bs_new-job == NULL);
assert(bs_new-dev == NULL);
@@ -2068,8 +2069,9 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState
*bs_old)
bdrv_move_feature_fields(bs_old, bs_new);
bdrv_move_feature_fields(bs_new, tmp);
-/* bs_new shouldn't be in bdrv_states even after the swap! */
+/* bs_new must remain nameless and unattached */
assert(bs_new-device_name[0] == '\0');
+assert(!bs_new-blk);
/* Check a few fields that should remain attached to the device */
assert(bs_new-dev == NULL);
@@ -2096,7 +2098,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState
*bs_old)
* This will modify the BlockDriverState fields, and swap contents
* between bs_new and bs_top. Both bs_new and bs_top are modified.
*
- * bs_new is required to be anonymous.
+ * bs_new must be nameless and not attached to a BlockBackend.
*
* This function does not create any image files.
*/
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.
+ */
+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;
+}
+
static void blk_delete(BlockBackend *blk)
{
assert(!blk-refcnt);
-QTAILQ_REMOVE(blk_backends, blk, link);
+if (blk-bs) {
+blk-bs-blk = NULL;
+blk-bs = NULL;
+}
+if (blk-name[0]) {
+QTAILQ_REMOVE(blk_backends, blk, link);
+}
g_free(blk-name);
g_free(blk);
}
@@ -68,6 +102,8 @@ void blk_ref(BlockBackend *blk)
* Decrement @blk's reference count.
* If this drops it to zero, destroy @blk.
* For convenience, do nothing if @blk is null.
+ * Does *not* touch the attached BlockDriverState's reference count.
+ * TODO Decrement it!
*/
void blk_unref(BlockBackend *blk)
{
Re: [Qemu-devel] [PATCH v2 03/23] block: Connect BlockBackend to BlockDriverState
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
