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

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

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

I'll split it off.

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

No, because the comment appears as if it applied only to the first
group.

This is how this spot looks at the end of the series:

    /* Set legacy DriveInfo fields */
    dinfo = g_malloc0(sizeof(*dinfo));
    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);

Could do this instead:

    dinfo = g_malloc0(sizeof(*dinfo));
    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);

The comment is next to useless anyway.  Got a preference?

>>      switch(type) {
>>      case IF_IDE: