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

["Benoît Canet" <benoit.canet@irqsave.net>]

The Wednesday 10 Sep 2014 à 10:13:33 (+0200), Markus Armbruster wrote :
> 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);

Is there any technical reason not to just put the
drive_info_del above the blk_delete function ?
I don't see any possible circular references between the two.

Some people like Eric frown upon static function prototypes
in final code that's why I am asking.

> +
>  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);
> @@ -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)) {

Here I understand why you didn't made blk_next pure round robin circling in a loop.
Maybe the comments of the blk_next function should say it's just an iterator.

> +        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);
>  }
>  
>  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;
Here you fix the leak I found on a previous patch.

>          }
>      }
>      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) {

Just down the code an existing test does if (local_err) { after
blk = blockdev_init(NULL, qdict, &local_err);.

Is the prefered convention checking blk or local_err ?

> +        error_report("%s", error_get_pretty(local_err));
>          goto fail;
>      } else {
>          assert(!local_err);
>      }
>  
>      /* Set legacy DriveInfo fields */
> +    dinfo = blk_legacy_dinfo(blk);
>      dinfo->enable_auto_del = true;
>      dinfo->opts = all_opts;
>  
> @@ -2478,7 +2470,7 @@ void qmp_change_backing_file(const char *device,
>  void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
>  {
>      QmpOutputVisitor *ov = qmp_output_visitor_new();
> -    DriveInfo *dinfo;
> +    BlockBackend *blk;
>      QObject *obj;
>      QDict *qdict;
>      Error *local_err = NULL;
> @@ -2516,14 +2508,15 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
>  
>      qdict_flatten(qdict);
>  
> -    dinfo = blockdev_init(NULL, qdict, &local_err);
> +    blk = blockdev_init(NULL, qdict, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          goto fail;
>      }
>  
> -    if (bdrv_key_required(dinfo->bdrv)) {
> -        drive_del(dinfo);
> +    if (bdrv_key_required(blk_bs(blk))) {
> +        bdrv_unref(blk_bs(blk));
> +        blk_unref(blk);
>          error_setg(errp, "blockdev-add doesn't support encrypted devices");
>          goto fail;
>      }
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 23a5d10..2ed297b 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -45,6 +45,10 @@ struct DriveInfo {
>      QTAILQ_ENTRY(DriveInfo) next;
>  };
>  
> +DriveInfo *blk_legacy_dinfo(BlockBackend *blk);
> +DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo);
> +BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo);
> +
>  DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
>  DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
>  int drive_get_max_bus(BlockInterfaceType type);
> -- 
> 1.9.3
>