Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben:
> Make the BlockBackend own the DriveInfo. Change blockdev_init() to
> return the BlockBackend instead of the DriveInfo.
>
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
> ---
> block.c | 2 --
> block/block-backend.c | 38 ++++++++++++++++++++++++
> blockdev.c | 73
> ++++++++++++++++++++++++-----------------------
> include/sysemu/blockdev.h | 4 +++
> 4 files changed, 79 insertions(+), 38 deletions(-)
>
> diff --git a/block.c b/block.c
> index 7ccf443..5f7dc45 100644
> --- a/block.c
> +++ b/block.c
> @@ -29,7 +29,6 @@
> #include "qemu/module.h"
> #include "qapi/qmp/qjson.h"
> #include "sysemu/sysemu.h"
> -#include "sysemu/blockdev.h" /* FIXME layering violation */
> #include "qemu/notify.h"
> #include "block/coroutine.h"
> #include "block/qapi.h"
> @@ -2124,7 +2123,6 @@ static void bdrv_delete(BlockDriverState *bs)
> /* remove from list, if necessary */
> bdrv_make_anon(bs);
>
> - drive_info_del(drive_get_by_blockdev(bs));
> g_free(bs);
> }
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index a12215a..9ee57c7 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -12,11 +12,13 @@
>
> #include "sysemu/block-backend.h"
> #include "block/block_int.h"
> +#include "sysemu/blockdev.h"
>
> struct BlockBackend {
> char *name;
> int refcnt;
> BlockDriverState *bs;
> + DriveInfo *legacy_dinfo;
> QTAILQ_ENTRY(BlockBackend) link; /* for blk_backends */
> };
>
> @@ -87,6 +89,7 @@ static void blk_delete(BlockBackend *blk)
> QTAILQ_REMOVE(&blk_backends, blk, link);
> }
> g_free(blk->name);
> + drive_info_del(blk->legacy_dinfo);
> g_free(blk);
> }
>
> @@ -167,6 +170,41 @@ BlockDriverState *blk_bs(BlockBackend *blk)
> }
>
> /*
> + * Return @blk's DriveInfo if any, else null.
> + */
> +DriveInfo *blk_legacy_dinfo(BlockBackend *blk)
> +{
> + return blk->legacy_dinfo;
> +}
> +
> +/*
> + * Set @blk's DriveInfo to @dinfo, and return it.
> + * @blk must not have a DriveInfo set already.
> + * No other BlockBackend may have the same DriveInfo set.
> + */
> +DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo)
> +{
> + assert(!blk->legacy_dinfo);
> + return blk->legacy_dinfo = dinfo;
Ugh, I don't like assignments in a return statement much more than
setters that return something.
Fortunately, nobody uses the return value, so this can become a void
function.
> +}
> +/*
> + * Return the BlockBackend with DriveInfo @dinfo.
> + * It must exist.
> + */
> +BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo)
> +{
> + BlockBackend *blk;
> +
> + QTAILQ_FOREACH(blk, &blk_backends, link) {
> + if (blk->legacy_dinfo == dinfo) {
> + return blk;
> + }
> + }
> + assert(0);
I'm surprised that the compiler doesn't complain here. Seems it
understands that the condition is always false and the libc header sets
the noreturn attribute for the internal function handling a failure.
With NDEBUG set, this wouldn't work any more.
I think abort() is better.
> +}
> +
> +/*
> * Hide @blk.
> * @blk must not have been hidden already.
> * Make attached BlockDriverState, if any, anonymous.
> diff --git a/blockdev.c b/blockdev.c
> index 5da6028..722d083 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -47,8 +47,6 @@
> #include "trace.h"
> #include "sysemu/arch_init.h"
>
> -static QTAILQ_HEAD(drivelist, DriveInfo) drives =
> QTAILQ_HEAD_INITIALIZER(drives);
> -
> static const char *const if_name[IF_COUNT] = {
> [IF_NONE] = "none",
> [IF_IDE] = "ide",
> @@ -89,7 +87,8 @@ static const int if_max_devs[IF_COUNT] = {
> */
> void blockdev_mark_auto_del(BlockDriverState *bs)
> {
> - DriveInfo *dinfo = drive_get_by_blockdev(bs);
> + BlockBackend *blk = bs->blk;
> + DriveInfo *dinfo = blk_legacy_dinfo(blk);
>
> if (dinfo && !dinfo->enable_auto_del) {
> return;
> @@ -105,7 +104,8 @@ void blockdev_mark_auto_del(BlockDriverState *bs)
>
> void blockdev_auto_del(BlockDriverState *bs)
> {
> - DriveInfo *dinfo = drive_get_by_blockdev(bs);
> + BlockBackend *blk = bs->blk;
> + DriveInfo *dinfo = blk_legacy_dinfo(blk);
>
> if (dinfo && dinfo->auto_del) {
> drive_del(dinfo);
> @@ -153,15 +153,15 @@ QemuOpts *drive_add(BlockInterfaceType type, int index,
> const char *file,
>
> DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
> {
> + BlockBackend *blk;
> DriveInfo *dinfo;
>
> - /* seek interface, bus and unit */
> -
> - QTAILQ_FOREACH(dinfo, &drives, next) {
> - if (dinfo->type == type &&
> - dinfo->bus == bus &&
> - dinfo->unit == unit)
> + for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
> + dinfo = blk_legacy_dinfo(blk);
> + if (dinfo && dinfo->type == type
> + && dinfo->bus == bus && dinfo->unit == unit) {
> return dinfo;
> + }
> }
>
> return NULL;
> @@ -177,13 +177,15 @@ DriveInfo *drive_get_by_index(BlockInterfaceType type,
> int index)
> int drive_get_max_bus(BlockInterfaceType type)
> {
> int max_bus;
> + BlockBackend *blk;
> DriveInfo *dinfo;
>
> max_bus = -1;
> - QTAILQ_FOREACH(dinfo, &drives, next) {
> - if(dinfo->type == type &&
> - dinfo->bus > max_bus)
> + for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
> + dinfo = blk_legacy_dinfo(blk);
> + if (dinfo && dinfo->type == type && dinfo->bus > max_bus) {
> max_bus = dinfo->bus;
> + }
> }
> return max_bus;
> }
> @@ -200,11 +202,11 @@ DriveInfo *drive_get_next(BlockInterfaceType type)
>
> DriveInfo *drive_get_by_blockdev(BlockDriverState *bs)
> {
> - DriveInfo *dinfo;
> + BlockBackend *blk;
>
> - QTAILQ_FOREACH(dinfo, &drives, next) {
> - if (dinfo->bdrv == bs) {
> - return dinfo;
> + for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
> + if (blk_bs(blk) == bs) {
> + return blk_legacy_dinfo(blk);
> }
> }
> return NULL;
Why don't you use bs->blk here?
Also, why replace calls to drive_get_by_blockdev() above when the
function stays? Wouldn't it make more sense to remove all callers in a
single patch instead of doing part of it in an unrelated patch?
> @@ -218,6 +220,7 @@ static void bdrv_format_print(void *opaque, const char
> *name)
> void drive_del(DriveInfo *dinfo)
> {
> bdrv_unref(dinfo->bdrv);
> + blk_unref(blk_by_legacy_dinfo(dinfo));
> }
Hmm, let's see. drive_del() has four callers:
* do_drive_del(): A blk_unref() is removed there, so the code remains
equivalent. Good.
* blockdev_auto_del(): Here, the blk_unref() is new. If I'm reading the
code right, unplugging a device doesn't only automagically destroy the
DriveInfo now, but also the BlockBackend. Is this really magic that we
want BlockBackend to inherit? (I don't think so.)
* drive_hot_add(): This is an error paths that frees a previously leaked
BlockBackend created by an indirect drive_new() call. Silent bug fix?
* pci_piix3_xen_ide_unplug(): Xen magic I don't fully understand. The
guest can tell us to remove the IDE drives (because it sees the same
image through the PV driver, I think). Whether or not we want to
remove the BlockBackend, I'm not completely sure.
Kevin
