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

[Kevin Wolf <kwolf@redhat.com>]

Am 10.09.2014 um 10:13 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/block-backend.c     | 38 +++++++++++++++++++++++
>  blockdev.c                | 79 +++++++++++++++++++++--------------------------
>  include/sysemu/blockdev.h |  4 +++
>  3 files changed, 78 insertions(+), 43 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index deccb54..2a22660 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -12,14 +12,18 @@
>  
>  #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 */
>  };
>  
> +static void drive_info_del(DriveInfo *dinfo);
> +
>  static QTAILQ_HEAD(, BlockBackend) blk_backends =
>      QTAILQ_HEAD_INITIALIZER(blk_backends);
>  
> @@ -87,6 +91,7 @@ static void blk_delete(BlockBackend *blk)
>      blk_detach_bs(blk);
>      QTAILQ_REMOVE(&blk_backends, blk, link);
>      g_free(blk->name);
> +    drive_info_del(blk->legacy_dinfo);
>      g_free(blk);
>  }
>  
> @@ -119,6 +124,16 @@ void blk_unref(BlockBackend *blk)
>      }
>  }
>  
> +static void drive_info_del(DriveInfo *dinfo)
> +{
> +    if (dinfo) {
> +        qemu_opts_del(dinfo->opts);
> +        g_free(dinfo->id);
> +        g_free(dinfo->serial);
> +        g_free(dinfo);
> +    }
> +}
> +
>  const char *blk_name(BlockBackend *blk)
>  {
>      return blk->name;
> @@ -187,3 +202,26 @@ BlockDriverState *blk_detach_bs(BlockBackend *blk)
>      }
>      return bs;
>  }
> +
> +DriveInfo *blk_legacy_dinfo(BlockBackend *blk)
> +{
> +    return blk->legacy_dinfo;
> +}
> +
> +DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo)
> +{
> +    assert(!blk->legacy_dinfo);
> +    return blk->legacy_dinfo = dinfo;
> +}
> +
> +BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo)
> +{
> +    BlockBackend *blk;
> +
> +    QTAILQ_FOREACH(blk, &blk_backends, link) {
> +        if (blk->legacy_dinfo == dinfo) {
> +            return blk;
> +        }
> +    }
> +    assert(0);
> +}
> diff --git a/blockdev.c b/blockdev.c
> index 0a0b95e..73e2da9 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);

The if condition looks as if there were cases where dinfo was NULL. To
be precise, this was introduced in commit 0fc0f1fa, so that hot unplug
after a forced drive_del wouldn't segfault.

Does such a device that doesn't have a dinfo at least have a
BlockBackend? If it doesn't, we'll segfault in blk_legacy_dinfo(). I
won't say that I understand that part of the code, but I can't see why
BlockBackend would still be there when DriveInfo isn't any more.

> @@ -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;
> @@ -217,16 +219,10 @@ static void bdrv_format_print(void *opaque, const char *name)
>  
>  void drive_del(DriveInfo *dinfo)
>  {
> -    if (dinfo->opts) {
> -        qemu_opts_del(dinfo->opts);
> -    }
> +    BlockBackend *blk = dinfo->bdrv->blk;
>  
>      bdrv_unref(dinfo->bdrv);
> -    blk_unref(blk_by_name(dinfo->id));
> -    g_free(dinfo->id);
> -    QTAILQ_REMOVE(&drives, dinfo, next);
> -    g_free(dinfo->serial);
> -    g_free(dinfo);
> +    blk_unref(blk);
>  }

Now the dinfo stays alive until the last reference for blk is gone. We
probably don't care much about the memory that isn't used any more, but
not freed yet. What about dinfo->opts? Does it block IDs that should be
available again?

Do we need a dinfo_attach/detach like for BDSes?

>  typedef struct {
> @@ -296,8 +292,8 @@ static bool check_throttle_config(ThrottleConfig *cfg, Error **errp)
>  typedef enum { MEDIA_DISK, MEDIA_CDROM } DriveMediaType;
>  
>  /* Takes the ownership of bs_opts */
> -static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
> -                                Error **errp)
> +static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
> +                                   Error **errp)
>  {
>      const char *buf;
>      int ro = 0;
> @@ -480,7 +476,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>      dinfo = g_malloc0(sizeof(*dinfo));
>      dinfo->id = g_strdup(qemu_opts_id(opts));
>      dinfo->bdrv = bs;
> -    QTAILQ_INSERT_TAIL(&drives, dinfo, next);
> +    blk_set_legacy_dinfo(blk, dinfo);
>  
>      if (!file || !*file) {
>          if (has_driver_specific_opts) {
> @@ -488,7 +484,7 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>          } else {
>              QDECREF(bs_opts);
>              qemu_opts_del(opts);
> -            return dinfo;
> +            return blk;
>          }
>      }
>      if (snapshot) {
> @@ -525,13 +521,10 @@ static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
>      QDECREF(bs_opts);
>      qemu_opts_del(opts);
>  
> -    return dinfo;
> +    return blk;
>  
>  err:
>      bdrv_unref(dinfo->bdrv);
> -    QTAILQ_REMOVE(&drives, dinfo, next);
> -    g_free(dinfo->id);
> -    g_free(dinfo);
>      blk_unref(blk);
>  early_err:
>      qemu_opts_del(opts);
> @@ -635,6 +628,7 @@ QemuOptsList qemu_legacy_drive_opts = {
>  DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>  {
>      const char *value;
> +    BlockBackend *blk;
>      DriveInfo *dinfo = NULL;
>      QDict *bs_opts;
>      QemuOpts *legacy_opts;
> @@ -917,19 +911,17 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>      }
>  
>      /* Actual block device init: Functionality shared with blockdev-add */
> -    dinfo = blockdev_init(filename, bs_opts, &local_err);
> +    blk = blockdev_init(filename, bs_opts, &local_err);
>      bs_opts = NULL;
> -    if (dinfo == NULL) {
> -        if (local_err) {
> -            error_report("%s", error_get_pretty(local_err));
> -            error_free(local_err);
> -        }
> +    if (!blk) {
> +        error_report("%s", error_get_pretty(local_err));

Now local_err is leaked.

>          goto fail;
>      } else {
>          assert(!local_err);
>      }

Kevin