From 7e7d56d9e05b340290669442cfa05f5869204572 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 7 Oct 2014 13:59:05 +0200 Subject: [PATCH] block: Connect BlockBackend to BlockDriverState 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 Reviewed-by: Max Reitz Signed-off-by: Kevin Wolf --- block.c | 12 ++-- block/block-backend.c | 71 ++++++++++++++++++++++- blockdev.c | 19 +++---- 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 | 4 +- 9 files changed, 156 insertions(+), 94 deletions(-) diff --git a/block.c b/block.c index 42659ec..e11a3ec 100644 --- a/block.c +++ b/block.c @@ -2025,6 +2025,8 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest, pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name), bs_src->device_name); bs_dest->device_list = bs_src->device_list; + bs_dest->blk = bs_src->blk; + memcpy(bs_dest->op_blockers, bs_src->op_blockers, sizeof(bs_dest->op_blockers)); } @@ -2037,7 +2039,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. */ @@ -2056,8 +2058,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); @@ -2073,8 +2076,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); @@ -2101,7 +2105,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 e89caa9..a12215a 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,44 @@ 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; + } + /* Avoid double-remove after blk_hide_on_behalf_of_do_drive_del() */ + if (blk->name[0]) { + QTAILQ_REMOVE(&blk_backends, blk, link); + } g_free(blk->name); g_free(blk); } @@ -68,6 +103,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 +132,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) { @@ -118,3 +157,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 508188e..12fabcc 100644 --- a/blockdev.c +++ b/blockdev.c @@ -279,7 +279,7 @@ static void bdrv_format_print(void *opaque, const char *name) void drive_del(DriveInfo *dinfo) { - BlockBackend *blk = blk_by_name(dinfo->id); + BlockBackend *blk = dinfo->bdrv->blk; bdrv_unref(dinfo->bdrv); blk_unref(blk); @@ -528,14 +528,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; @@ -600,7 +597,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); @@ -1840,16 +1836,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) { @@ -1879,8 +1877,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_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 265cf13..0022083 100644 --- a/hw/block/xen_disk.c +++ b/hw/block/xen_disk.c @@ -860,15 +860,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 8d86a6c..14e0b7c 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -324,6 +324,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 198062a..b97a794 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 bdb95a4..5548637 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; @@ -1895,13 +1892,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) { @@ -2161,12 +2157,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"); @@ -2285,12 +2280,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) { @@ -2333,7 +2327,6 @@ static int img_snapshot(int argc, char **argv) } /* Cleanup */ -out: bdrv_unref(bs); blk_unref(blk); if (ret) { @@ -2433,12 +2426,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; @@ -2466,8 +2459,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); @@ -2478,8 +2471,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) { @@ -2755,13 +2748,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; @@ -2873,13 +2866,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 a8dc973..8380734 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 634ba11..f741973 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -692,8 +692,8 @@ int main(int argc, char **argv) drv = NULL; } - blk = blk_new("hda", &error_abort); - bs = bdrv_new_root("hda", &error_abort); + blk = blk_new_with_bs("hda", &error_abort); + bs = blk_bs(blk); srcpath = argv[optind]; ret = bdrv_open(&bs, srcpath, NULL, NULL, flags, drv, &local_err); -- 2.7.4