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

[Markus Armbruster]

Benoît Canet <address@hidden> writes:

> 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 <address@hidden>
>> ---
>>  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.

Placing functions before their callers makes the program easier to read
when you need to see the functions definition before you can understand
their use.

Placing the functions after callers makes the program easier to read
when the gist of what they do is obvious from the call.  You're omitting
unnecessary detail there, to be flesh it own further down.  Saving a
function declaration is immaterial compared to that.

Before I put the function where I don't want it, I'd inline it :)

>> +
>>  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.

It works exactly like bdrv_next().

> Maybe the comments of the blk_next function should say it's just an iterator.

I could add example usage to the comment there.

>> +        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.

I don't think it leaked.

>>          }
>>      }
>>      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 ?

Both

    foop = foo_new(bar, &local_err);
    if (local_err) {
        assert(!foop);
        error_propagate(errp, local_err)
        goto fail;
    }
    assert(!local_err);

and

    foop = foo_new(bar, &errp);
    if (!foop) {
        assert(!errp || *errp);
        goto fail;
    }
    assert(!errp || *errp);

work.  Of course, nobody bothers with the assertions; the error handling
is enough verbiage as it is.

However, I dislike the first variant intensely.  Not least because it's
prone to get Coverity worry about foop being null in code following it.

>> +        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;
>>  
[...]