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. Signed-off-by: Markus Armbruster <arm...@redhat.com> --- block.c | 10 ++-- block/block-backend.c | 79 +++++++++++++++++++++++++++++++ blockdev.c | 49 +++++++++++--------- hw/block/xen_disk.c | 8 +--- include/block/block_int.h | 2 + include/sysemu/block-backend.h | 5 ++ qemu-img.c | 103 +++++++++++++++++++++-------------------- qemu-io.c | 4 +- qemu-nbd.c | 3 +- 9 files changed, 175 insertions(+), 88 deletions(-) diff --git a/block.c b/block.c index 4b3bcd4..a6c03da 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 833f7d9..deccb54 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -16,6 +16,7 @@ struct BlockBackend { char *name; int refcnt; + BlockDriverState *bs; QTAILQ_ENTRY(BlockBackend) link; /* for blk_backends */ }; @@ -47,9 +48,43 @@ BlockBackend *blk_new(const char *name, Error **errp) return blk; } +/** + * blk_new_with_bs: + * @name: name, must not be %NULL or empty + * @errp: return location for an error to be set on failure, or %NULL + * + * Create a new BlockBackend, with a reference count of one, and + * attach a new BlockDriverState to it, also with a reference count of + * one. Caller owns *both* references. + * TODO Let caller own only the BlockBackend reference + * Fail if @name already exists. + * + * Returns: the BlockBackend on success, %NULL on error + */ +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_named(name, errp); + if (!bs) { + blk_unref(blk); + return NULL; + } + + blk_attach_bs(blk, bs); + return blk; +} + static void blk_delete(BlockBackend *blk) { assert(!blk->refcnt); + blk_detach_bs(blk); QTAILQ_REMOVE(&blk_backends, blk, link); g_free(blk->name); g_free(blk); @@ -70,6 +105,9 @@ void blk_ref(BlockBackend *blk) * * Decrement @blk's reference count. If this drops it to zero, * destroy @blk. + * + * Does *not* touch the attached BlockDriverState's reference count. + * TODO Decrement it! */ void blk_unref(BlockBackend *blk) { @@ -108,3 +146,44 @@ BlockBackend *blk_next(BlockBackend *blk) { return blk ? QTAILQ_NEXT(blk, link) : QTAILQ_FIRST(&blk_backends); } + +/** + * blk_attach_bs: + * + * Attach @bs to @blk, taking over the caller's reference to @bs. + */ +void blk_attach_bs(BlockBackend *blk, BlockDriverState *bs) +{ + assert(!blk->bs && !bs->blk); + blk->bs = bs; + bs->blk = blk; +} + +/** + * blk_bs: + * + * Returns: the BlockDriverState attached to @blk, or %NULL + */ +BlockDriverState *blk_bs(BlockBackend *blk) +{ + return blk->bs; +} + +/** + * blk_detach_bs: + * + * Detach @blk's BlockDriverState, giving up its reference to the + * caller. + * + * Returns: the detached BlockDriverState + */ +BlockDriverState *blk_detach_bs(BlockBackend *blk) +{ + BlockDriverState *bs = blk->bs; + + if (bs) { + bs->blk = NULL; + blk->bs = NULL; + } + return bs; +} diff --git a/blockdev.c b/blockdev.c index 86596bc..0a0b95e 100644 --- a/blockdev.c +++ b/blockdev.c @@ -304,6 +304,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts, int bdrv_flags = 0; int on_read_error, on_write_error; BlockBackend *blk; + BlockDriverState *bs; DriveInfo *dinfo; ThrottleConfig cfg; int snapshot = 0; @@ -459,30 +460,28 @@ 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 = blk_bs(blk); + bs->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0; + bs->read_only = ro; + bs->detect_zeroes = detect_zeroes; + + bdrv_set_on_error(bs, on_read_error, on_write_error); + + /* disk I/O throttling */ + if (throttle_enabled(&cfg)) { + bdrv_io_limits_enable(bs); + bdrv_set_io_limits(bs, &cfg); + } + dinfo = g_malloc0(sizeof(*dinfo)); dinfo->id = g_strdup(qemu_opts_id(opts)); - dinfo->bdrv = bdrv_new_named(dinfo->id, &error); - if (error) { - error_propagate(errp, error); - goto bdrv_new_err; - } - dinfo->bdrv->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0; - dinfo->bdrv->read_only = ro; - dinfo->bdrv->detect_zeroes = detect_zeroes; + dinfo->bdrv = bs; QTAILQ_INSERT_TAIL(&drives, dinfo, next); - bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error); - - /* disk I/O throttling */ - if (throttle_enabled(&cfg)) { - bdrv_io_limits_enable(dinfo->bdrv); - bdrv_set_io_limits(dinfo->bdrv, &cfg); - } - if (!file || !*file) { if (has_driver_specific_opts) { file = NULL; @@ -509,7 +508,8 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts, bdrv_flags |= ro ? 0 : BDRV_O_RDWR; QINCREF(bs_opts); - ret = bdrv_open(&dinfo->bdrv, file, NULL, bs_opts, bdrv_flags, drv, &error); + ret = bdrv_open(&bs, file, NULL, bs_opts, bdrv_flags, drv, &error); + assert(bs == blk_bs(blk)); if (ret < 0) { error_setg(errp, "could not open disk image %s: %s", @@ -518,8 +518,9 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts, goto err; } - if (bdrv_key_required(dinfo->bdrv)) + if (bdrv_key_required(bs)) { autostart = 0; + } QDECREF(bs_opts); qemu_opts_del(opts); @@ -529,7 +530,6 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts, err: bdrv_unref(dinfo->bdrv); QTAILQ_REMOVE(&drives, dinfo, next); -bdrv_new_err: g_free(dinfo->id); g_free(dinfo); blk_unref(blk); @@ -1746,15 +1746,17 @@ 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; 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); aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); @@ -1777,8 +1779,9 @@ 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)) { + blk_detach_bs(blk); + blk_unref(blk); bdrv_make_anon(bs); - blk_unref(blk_by_name(id)); /* 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 730a021..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_named(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..539b96f 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); +void blk_attach_bs(BlockBackend *blk, BlockDriverState *bs); +BlockDriverState *blk_detach_bs(BlockBackend *blk); +BlockDriverState *blk_bs(BlockBackend *blk); + #endif diff --git a/qemu-img.c b/qemu-img.c index bad3f64..9fba5a1 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_named(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; } @@ -651,11 +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) { + 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); @@ -761,11 +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) { + 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: @@ -1019,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 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 out2; } + bs2 = blk_bs(blk2); buf1 = qemu_blockalign(bs1, IO_BUF_SIZE); buf2 = qemu_blockalign(bs2, IO_BUF_SIZE); @@ -1375,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", @@ -1501,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; @@ -1893,12 +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 = 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) { @@ -2158,11 +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) { + 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"); @@ -2281,11 +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) { + blk = img_open("image", filename, NULL, bdrv_oflags, true, quiet); + if (!blk) { return 1; } + bs = blk_bs(blk); /* Perform the requested action */ switch(action) { @@ -2427,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; @@ -2460,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_named("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); @@ -2472,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_named("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) { @@ -2749,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; @@ -2867,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 45e5494..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_named("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 94b9b49..8eff588 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -687,8 +687,7 @@ int main(int argc, char **argv) drv = NULL; } - blk_new("hda", &error_abort); - bs = bdrv_new_named("hda", &error_abort); + bs = blk_bs(blk_new_with_bs("hda", &error_abort)); srcpath = argv[optind]; ret = bdrv_open(&bs, srcpath, NULL, NULL, flags, drv, &local_err); -- 1.9.3