Re: [Qemu-devel] [PATCH v3 04/23] block: Connect BlockBackend and DriveInfo

[Kevin Wolf <kwolf@redhat.com>]

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 <armbru@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