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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

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

Ah, our friend drive_del again.

As I explained in my reply to your review of PATCH 02, the initial
implementation of drive_del destroyed the root BDS and the DriveInfo
right away, leaving the device model with a dangling pointer to the root
BDS.  Naturally, this was prone to crash eventually, when the memory
gets reused.  But here, it crashes right away, because the DriveInfo no
longer exists.  Commit 0fc0f1f "fixed" that part.  The real fix is
commit d22b2f4.

However, my PATCH 18 will change blockdev-add not to create a DriveInfo,
and then this test will become necessary again.

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

Hmm.

I don't think deleting options and DriveInfo makes the ID available for
reuse.  While you can create new options with the same ID then, you
still can't create a new BDS or BB with the same ID while the old ones
live.

But let's dig deeper.

This hunk changes behavior of drive_del when blk->refcnt > 1: DriveInfo
and the options in qemu_drive_opts stay around.  blk->recfcnt > 1 can't
happen so far, but that's no excuse.

In current master, we have:

    void drive_del(DriveInfo *dinfo)
    {
        if (dinfo->opts) {
            qemu_opts_del(dinfo->opts);
        }

        bdrv_unref(dinfo->bdrv);
        g_free(dinfo->id);
        QTAILQ_REMOVE(&drives, dinfo, next);
        g_free(dinfo->serial);
        g_free(dinfo);
    }

We destroy the DriveInfo and the options unconditionally, but the
BDS only when dinfo->bdrv->refcnt == 1.

Callers:

* drive_hot_add()

  Cleanup on error, thus refcnt == 1; destroys both BDS and DriveInfo.
  Good.

* qmp_blockdev_add() if image is encryped

  Likewise.

* blockdev_auto_del()

  Runs during device model destruction, when we detach it from its BDS.

  If the BDS's refcnt > 0, we destroy the DriveInfo, but not the BDS.
  Code assuming such a BDS always has a DriveInfo could crash.  PATCH 18
  eliminates that assumption, thus shows the code in question: all
  device model initialization.

  If we somehow manage to use the BDS to plug in such a device model
  before the BDS's refcnt drops to zero, we crash.  I figure this is
  possible, because device_add resolves property "drive" via bdrv_find()
  just fine, even when there's no DriveInfo.

* pci_piix3_xen_ide_unplug()

  No idea what this one's supposed to do.  Anyway, it also detaches
  device model from its BDS before drive_del(), so my crash scenario
  should apply.

* do_drive_del() if no device model is attached

  Crash scenario applies.

So my patch is actually a bug fix.  I'll look into separating the bug
fix part into its own patch.

> Do we need a dinfo_attach/detach like for BDSes?

I doubt it.

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

Fixing...

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