Am 10.09.2014 um 10:13 hat Markus Armbruster geschrieben: > 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>
I'm wondering whether this is the right direction to take. In creating a block device, we have the following operations: 1. Create a BlockBackend 2. Create a BlockDriverState 3. Open the BlockDriverState bdrv_open() can do 2 and 3, which is probably what we want to have in the end - new/create and delete/close should be a single operation for BDSes. Combining 1 and 2 into a single function might make it harder to actually use bdrv_open() in this way. > 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; This looks correct, but it's really doing two things at once (on the one hand changing code to use bs instead of dinfo->bdrv and reordering, on the other hand introducing blk_new_with_bs()). Might be worth doing in separate patches. > @@ -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); We're not doing real refcounting yet, but if we did, and if the refcount didn't drop to zero here, why would detaching the bs still be correct? blk_delete() already takes care of detaching after this patch, so I suppose removing the call here would be right. > 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); Kevin