Re: [Qemu-devel] [PATCH v3 18/23] blockdev: Fix blockdev-add not to create IDE drive (0, 0)

[Kevin Wolf <kwolf@redhat.com>]

Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben:
> blockdev_init() always creates a DriveInfo, but only drive_new() fills
> it in.  qmp_blockdev_add() leaves it blank.  This results in a drive
> with type = IF_IDE, bus = 0, unit = 0.  Screwed up in commit ee13ed1c.
> 
> Board initialization code looking for IDE drive (0,0) can pick up one
> of these bogus drives.  Not sure whether getting the QMP command
> executed early enough is likely in practice, though.
> 
> Fix by creating DriveInfo in drive_new().  Block backends created by
> blockdev-add don't get one.
> 
> A few places assume a block backend always has a DriveInfo.  Fix them
> up.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/block-backend.c |  4 +---
>  blockdev.c            | 10 ++--------
>  hw/block/block.c      | 16 ++++++++++------
>  hw/ide/qdev.c         |  2 +-
>  hw/scsi/scsi-disk.c   |  2 +-
>  5 files changed, 15 insertions(+), 19 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 141a31b..b55f0b4 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -101,9 +101,7 @@ static void drive_info_del(DriveInfo *dinfo)
>      if (!dinfo) {
>          return;
>      }
> -    if (dinfo->opts) {
> -        qemu_opts_del(dinfo->opts);
> -    }
> +    qemu_opts_del(dinfo->opts);
>      g_free(dinfo->serial);
>      g_free(dinfo);
>  }

Doesn't look directly related?

> diff --git a/blockdev.c b/blockdev.c
> index 2def256..0d99be0 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -285,7 +285,6 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
>      int on_read_error, on_write_error;
>      BlockBackend *blk;
>      BlockDriverState *bs;
> -    DriveInfo *dinfo;
>      ThrottleConfig cfg;
>      int snapshot = 0;
>      bool copy_on_read;
> @@ -457,9 +456,6 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
>          bdrv_set_io_limits(bs, &cfg);
>      }
>  
> -    dinfo = g_malloc0(sizeof(*dinfo));
> -    blk_set_legacy_dinfo(blk, dinfo);
> -
>      if (!file || !*file) {
>          if (has_driver_specific_opts) {
>              file = NULL;
> @@ -903,21 +899,19 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>      }
>  
>      /* Set legacy DriveInfo fields */
> -    dinfo = blk_legacy_dinfo(blk);
> +    dinfo = g_malloc0(sizeof(*dinfo));
>      dinfo->enable_auto_del = true;
>      dinfo->opts = all_opts;
> -
>      dinfo->cyls = cyls;
>      dinfo->heads = heads;
>      dinfo->secs = secs;
>      dinfo->trans = translation;
> -
>      dinfo->type = type;
>      dinfo->bus = bus_id;
>      dinfo->unit = unit_id;
>      dinfo->devaddr = devaddr;
> -
>      dinfo->serial = g_strdup(serial);
> +    blk_set_legacy_dinfo(blk, dinfo);

You don't like the grouping?

>      switch(type) {
>      case IF_IDE:

Kevin