[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] [PATCH v2 03/23] block: Connect BlockBackend to BlockDriver
|
From: |
Markus Armbruster |
|
Subject: |
[Qemu-devel] [PATCH v2 03/23] block: Connect BlockBackend to BlockDriverState |
|
Date: |
Sat, 13 Sep 2014 17:00:07 +0200 |
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 <address@hidden>
---
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)
{
@@ -95,7 +131,9 @@ BlockBackend *blk_next(BlockBackend *blk)
}
/*
- * Return @blk's name, a non-null, non-empty string.
+ * Return @blk's name, a non-null string.
+ * Wart: the name is empty iff @blk has been hidden with
+ * blk_hide_on_behalf_of_do_drive_del().
*/
const char *blk_name(BlockBackend *blk)
{
@@ -117,3 +155,29 @@ BlockBackend *blk_by_name(const char *name)
}
return NULL;
}
+
+/*
+ * Return the BlockDriverState attached to @blk if any, else null.
+ */
+BlockDriverState *blk_bs(BlockBackend *blk)
+{
+ return blk->bs;
+}
+
+/*
+ * Hide @blk.
+ * @blk must not have been hidden already.
+ * Make attached BlockDriverState, if any, anonymous.
+ * Once hidden, @blk is invisible to all functions that don't receive
+ * it as argument. For example, blk_by_name() won't return it.
+ * Strictly for use by do_drive_del().
+ * TODO get rid of it!
+ */
+void blk_hide_on_behalf_of_do_drive_del(BlockBackend *blk)
+{
+ QTAILQ_REMOVE(&blk_backends, blk, link);
+ blk->name[0] = 0;
+ if (blk->bs) {
+ bdrv_make_anon(blk->bs);
+ }
+}
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));
g_free(dinfo->id);
QTAILQ_REMOVE(&drives, dinfo, next);
g_free(dinfo->serial);
@@ -474,14 +467,11 @@ static DriveInfo *blockdev_init(const char *file, QDict
*bs_opts,
}
/* init */
- blk = blk_new(qemu_opts_id(opts), errp);
+ blk = blk_new_with_bs(qemu_opts_id(opts), errp);
if (!blk) {
goto early_err;
}
- bs = bdrv_new_root(qemu_opts_id(opts), errp);
- if (!bs) {
- goto bdrv_new_err;
- }
+ bs = blk_bs(blk);
bs->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
bs->read_only = ro;
bs->detect_zeroes = detect_zeroes;
@@ -546,7 +536,6 @@ static DriveInfo *blockdev_init(const char *file, QDict
*bs_opts,
err:
bdrv_unref(bs);
-bdrv_new_err:
blk_unref(blk);
early_err:
qemu_opts_del(opts);
@@ -1761,16 +1750,18 @@ void qmp_block_set_io_throttle(const char *device,
int64_t bps, int64_t bps_rd,
int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
{
const char *id = qdict_get_str(qdict, "id");
+ BlockBackend *blk;
BlockDriverState *bs;
DriveInfo *dinfo;
AioContext *aio_context;
Error *local_err = NULL;
- bs = bdrv_find(id);
- if (!bs) {
+ blk = blk_by_name(id);
+ if (!blk) {
error_report("Device '%s' not found", id);
return -1;
}
+ bs = blk_bs(blk);
dinfo = drive_get_by_blockdev(bs);
if (dinfo && !dinfo->enable_auto_del) {
@@ -1800,8 +1791,7 @@ int do_drive_del(Monitor *mon, const QDict *qdict,
QObject **ret_data)
* then we can just get rid of the block driver state right here.
*/
if (bdrv_get_attached_dev(bs)) {
- bdrv_make_anon(bs);
- blk_unref(blk_by_name(id));
+ blk_hide_on_behalf_of_do_drive_del(blk);
/* Further I/O must not pause the guest */
bdrv_set_on_error(bs, BLOCKDEV_ON_ERROR_REPORT,
BLOCKDEV_ON_ERROR_REPORT);
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 088e354..51f4f3a 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -858,15 +858,11 @@ static int blk_connect(struct XenDevice *xendev)
/* setup via xenbus -> create new block driver instance */
xen_be_printf(&blkdev->xendev, 2, "create new bdrv (xenbus setup)\n");
- blk = blk_new(blkdev->dev, NULL);
+ blk = blk_new_with_bs(blkdev->dev, NULL);
if (!blk) {
return -1;
}
- blkdev->bs = bdrv_new_root(blkdev->dev, NULL);
- if (!blkdev->bs) {
- blk_unref(blk);
- return -1;
- }
+ blkdev->bs = blk_bs(blk);
drv = bdrv_find_whitelisted_format(blkdev->fileproto, readonly);
if (bdrv_open(&blkdev->bs, blkdev->filename, NULL, NULL, qflags,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8a61215..a04c852 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -323,6 +323,8 @@ struct BlockDriverState {
BlockDriver *drv; /* NULL means no media */
void *opaque;
+ BlockBackend *blk; /* owning backend, if any */
+
void *dev; /* attached device model, if any */
/* TODO change to DeviceState when all users are qdevified */
const BlockDevOps *dev_ops;
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 3f8371c..fa8f623 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -17,10 +17,15 @@
#include "qapi/error.h"
BlockBackend *blk_new(const char *name, Error **errp);
+BlockBackend *blk_new_with_bs(const char *name, Error **errp);
void blk_ref(BlockBackend *blk);
void blk_unref(BlockBackend *blk);
const char *blk_name(BlockBackend *blk);
BlockBackend *blk_by_name(const char *name);
BlockBackend *blk_next(BlockBackend *blk);
+BlockDriverState *blk_bs(BlockBackend *blk);
+
+void blk_hide_on_behalf_of_do_drive_del(BlockBackend *blk);
+
#endif
diff --git a/qemu-img.c b/qemu-img.c
index acb272e..206a513 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -284,20 +284,19 @@ static int print_block_option_help(const char *filename,
const char *fmt)
return 0;
}
-static BlockDriverState *bdrv_new_open(const char *id,
- const char *filename,
- const char *fmt,
- int flags,
- bool require_io,
- bool quiet)
+static BlockBackend *img_open(const char *id, const char *filename,
+ const char *fmt, int flags,
+ bool require_io, bool quiet)
{
+ BlockBackend *blk;
BlockDriverState *bs;
BlockDriver *drv;
char password[256];
Error *local_err = NULL;
int ret;
- bs = bdrv_new_root(id, &error_abort);
+ blk = blk_new_with_bs(id, &error_abort);
+ bs = blk_bs(blk);
if (fmt) {
drv = bdrv_find_format(fmt);
@@ -328,9 +327,10 @@ static BlockDriverState *bdrv_new_open(const char *id,
goto fail;
}
}
- return bs;
+ return blk;
fail:
bdrv_unref(bs);
+ blk_unref(blk);
return NULL;
}
@@ -580,7 +580,7 @@ static int img_check(int argc, char **argv)
BlockDriverState *bs;
int fix = 0;
int flags = BDRV_O_FLAGS | BDRV_O_CHECK;
- ImageCheck *check = NULL;
+ ImageCheck *check;
bool quiet = false;
fmt = NULL;
@@ -651,12 +651,11 @@ static int img_check(int argc, char **argv)
return 1;
}
- blk = blk_new("image", &error_abort);
- bs = bdrv_new_open("image", filename, fmt, flags, true, quiet);
- if (!bs) {
- ret = 1;
- goto fail;
+ blk = img_open("image", filename, fmt, flags, true, quiet);
+ if (!blk) {
+ return 1;
}
+ bs = blk_bs(blk);
check = g_new0(ImageCheck, 1);
ret = collect_image_check(bs, check, filename, fmt, fix);
@@ -762,12 +761,12 @@ static int img_commit(int argc, char **argv)
return 1;
}
- blk = blk_new("image", &error_abort);
- bs = bdrv_new_open("image", filename, fmt, flags, true, quiet);
- if (!bs) {
- ret = -1;
- goto out;
+ blk = img_open("image", filename, fmt, flags, true, quiet);
+ if (!blk) {
+ return 1;
}
+ bs = blk_bs(blk);
+
ret = bdrv_commit(bs);
switch(ret) {
case 0:
@@ -787,7 +786,6 @@ static int img_commit(int argc, char **argv)
break;
}
-out:
bdrv_unref(bs);
blk_unref(blk);
if (ret) {
@@ -1022,21 +1020,21 @@ static int img_compare(int argc, char **argv)
goto out3;
}
- blk1 = blk_new("image 1", &error_abort);
- bs1 = bdrv_new_open("image 1", filename1, fmt1, flags, true, quiet);
- if (!bs1) {
+ blk1 = img_open("image 1", filename1, fmt1, flags, true, quiet);
+ if (!blk1) {
error_report("Can't open file %s", filename1);
ret = 2;
- goto out2;
+ goto out3;
}
+ bs1 = blk_bs(blk1);
- blk2 = blk_new("image 2", &error_abort);
- bs2 = bdrv_new_open("image 2", filename2, fmt2, flags, true, quiet);
- if (!bs2) {
+ blk2 = img_open("image 2", filename2, fmt2, flags, true, quiet);
+ if (!blk2) {
error_report("Can't open file %s", filename2);
ret = 2;
- goto out1;
+ goto out2;
}
+ bs2 = blk_bs(blk2);
buf1 = qemu_blockalign(bs1, IO_BUF_SIZE);
buf2 = qemu_blockalign(bs2, IO_BUF_SIZE);
@@ -1198,7 +1196,6 @@ static int img_compare(int argc, char **argv)
out:
qemu_vfree(buf1);
qemu_vfree(buf2);
-out1:
bdrv_unref(bs2);
blk_unref(blk2);
out2:
@@ -1379,15 +1376,15 @@ static int img_convert(int argc, char **argv)
for (bs_i = 0; bs_i < bs_n; bs_i++) {
char *id = bs_n > 1 ? g_strdup_printf("source %d", bs_i)
: g_strdup("source");
- blk[bs_i] = blk_new(id, &error_abort);
- bs[bs_i] = bdrv_new_open(id, argv[optind + bs_i], fmt, src_flags,
- true, quiet);
+ blk[bs_i] = img_open(id, argv[optind + bs_i], fmt, src_flags,
+ true, quiet);
g_free(id);
- if (!bs[bs_i]) {
+ if (!blk[bs_i]) {
error_report("Could not open '%s'", argv[optind + bs_i]);
ret = -1;
goto out;
}
+ bs[bs_i] = blk_bs(blk[bs_i]);
bs_sectors[bs_i] = bdrv_nb_sectors(bs[bs_i]);
if (bs_sectors[bs_i] < 0) {
error_report("Could not get size of %s: %s",
@@ -1505,12 +1502,12 @@ static int img_convert(int argc, char **argv)
goto out;
}
- out_blk = blk_new("target", &error_abort);
- out_bs = bdrv_new_open("target", out_filename, out_fmt, flags, true,
quiet);
- if (!out_bs) {
+ out_blk = img_open("target", out_filename, out_fmt, flags, true, quiet);
+ if (!out_blk) {
ret = -1;
goto out;
}
+ out_bs = blk_bs(out_blk);
bs_i = 0;
bs_offset = 0;
@@ -1897,13 +1894,12 @@ static ImageInfoList *collect_image_info_list(const
char *filename,
}
g_hash_table_insert(filenames, (gpointer)filename, NULL);
- blk = blk_new("image", &error_abort);
- bs = bdrv_new_open("image", filename, fmt,
- BDRV_O_FLAGS | BDRV_O_NO_BACKING, false, false);
- if (!bs) {
- blk_unref(blk);
+ blk = img_open("image", filename, fmt,
+ BDRV_O_FLAGS | BDRV_O_NO_BACKING, false, false);
+ if (!blk) {
goto err;
}
+ bs = blk_bs(blk);
bdrv_query_image_info(bs, &info, &err);
if (err) {
@@ -2163,12 +2159,11 @@ static int img_map(int argc, char **argv)
return 1;
}
- blk = blk_new("image", &error_abort);
- bs = bdrv_new_open("image", filename, fmt, BDRV_O_FLAGS, true, false);
- if (!bs) {
- ret = -1;
- goto out;
+ blk = img_open("image", filename, fmt, BDRV_O_FLAGS, true, false);
+ if (!blk) {
+ return 1;
}
+ bs = blk_bs(blk);
if (output_format == OFORMAT_HUMAN) {
printf("%-16s%-16s%-16s%s\n", "Offset", "Length", "Mapped to", "File");
@@ -2287,12 +2282,11 @@ static int img_snapshot(int argc, char **argv)
filename = argv[optind++];
/* Open the image */
- blk = blk_new("image", &error_abort);
- bs = bdrv_new_open("image", filename, NULL, bdrv_oflags, true, quiet);
- if (!bs) {
- ret = -1;
- goto out;
+ blk = img_open("image", filename, NULL, bdrv_oflags, true, quiet);
+ if (!blk) {
+ return 1;
}
+ bs = blk_bs(blk);
/* Perform the requested action */
switch(action) {
@@ -2335,7 +2329,6 @@ static int img_snapshot(int argc, char **argv)
}
/* Cleanup */
-out:
bdrv_unref(bs);
blk_unref(blk);
if (ret) {
@@ -2435,12 +2428,12 @@ static int img_rebase(int argc, char **argv)
* Ignore the old backing file for unsafe rebase in case we want to correct
* the reference to a renamed or moved backing file.
*/
- blk = blk_new("image", &error_abort);
- bs = bdrv_new_open("image", filename, fmt, flags, true, quiet);
- if (!bs) {
+ blk = img_open("image", filename, fmt, flags, true, quiet);
+ if (!blk) {
ret = -1;
goto out;
}
+ bs = blk_bs(blk);
/* Find the right drivers for the backing files */
old_backing_drv = NULL;
@@ -2468,8 +2461,8 @@ static int img_rebase(int argc, char **argv)
if (!unsafe) {
char backing_name[1024];
- blk_old_backing = blk_new("old_backing", &error_abort);
- bs_old_backing = bdrv_new_root("old_backing", &error_abort);
+ blk_old_backing = blk_new_with_bs("old_backing", &error_abort);
+ bs_old_backing = blk_bs(blk_old_backing);
bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
ret = bdrv_open(&bs_old_backing, backing_name, NULL, NULL, src_flags,
old_backing_drv, &local_err);
@@ -2480,8 +2473,8 @@ static int img_rebase(int argc, char **argv)
goto out;
}
if (out_baseimg[0]) {
- blk_new_backing = blk_new("new_backing", &error_abort);
- bs_new_backing = bdrv_new_root("new_backing", &error_abort);
+ blk_new_backing = blk_new_with_bs("new_backing", &error_abort);
+ bs_new_backing = blk_bs(blk_new_backing);
ret = bdrv_open(&bs_new_backing, out_baseimg, NULL, NULL,
src_flags,
new_backing_drv, &local_err);
if (ret) {
@@ -2757,13 +2750,13 @@ static int img_resize(int argc, char **argv)
n = qemu_opt_get_size(param, BLOCK_OPT_SIZE, 0);
qemu_opts_del(param);
- blk = blk_new("image", &error_abort);
- bs = bdrv_new_open("image", filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR,
- true, quiet);
- if (!bs) {
+ blk = img_open("image", filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR,
+ true, quiet);
+ if (!blk) {
ret = -1;
goto out;
}
+ bs = blk_bs(blk);
if (relative) {
total_size = bdrv_getlength(bs) + n * relative;
@@ -2875,13 +2868,13 @@ static int img_amend(int argc, char **argv)
goto out;
}
- blk = blk_new("image", &error_abort);
- bs = bdrv_new_open("image", filename, fmt, flags, true, quiet);
- if (!bs) {
+ blk = img_open("image", filename, fmt, flags, true, quiet);
+ if (!blk) {
error_report("Could not open image '%s'", filename);
ret = -1;
goto out;
}
+ bs = blk_bs(blk);
fmt = bs->drv->format_name;
diff --git a/qemu-io.c b/qemu-io.c
index 57090de..ef1d3ea 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -62,8 +62,8 @@ static int openfile(char *name, int flags, int growable,
QDict *opts)
return 1;
}
- qemuio_blk = blk_new("hda", &error_abort);
- qemuio_bs = bdrv_new_root("hda", &error_abort);
+ qemuio_blk = blk_new_with_bs("hda", &error_abort);
+ qemuio_bs = blk_bs(qemuio_blk);
if (growable) {
flags |= BDRV_O_PROTOCOL;
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);
srcpath = argv[optind];
ret = bdrv_open(&bs, srcpath, NULL, NULL, flags, drv, &local_err);
--
1.9.3
- Re: [Qemu-devel] [PATCH v2 02/23] block: New BlockBackend, (continued)
[Qemu-devel] [PATCH v2 08/23] block: Eliminate BlockDriverState member device_name[], Markus Armbruster, 2014/09/13
[Qemu-devel] [PATCH v2 03/23] block: Connect BlockBackend to BlockDriverState,
Markus Armbruster <=
[Qemu-devel] [PATCH v2 12/23] virtio-blk: Drop redundant VirtIOBlock member conf, Markus Armbruster, 2014/09/13
[Qemu-devel] [PATCH v2 13/23] virtio-blk: Rename VirtIOBlkConf variables to conf, Markus Armbruster, 2014/09/13
[Qemu-devel] [PATCH v2 01/23] block: Split bdrv_new_root() off bdrv_new(), Markus Armbruster, 2014/09/13