diff mbox

[5/6] scsi-disk: Remove 'drive_kind'

Message ID m38vrlczs6.fsf@blackfin.pond.sub.org
State New
Headers show

Commit Message

Markus Armbruster July 26, 2011, 6:38 a.m. UTC 
Hannes Reinecke <hare@suse.de> writes:

> On 07/25/2011 05:59 PM, Markus Armbruster wrote:
>> Hannes Reinecke<hare@suse.de>  writes:
>>
>>> Instead of using its own definitions scsi-disk should
>>> be using the device type of the parent device.
>>>
>>> Signed-off-by: Hannes Reinecke<hare@suse.de>
>>> ---
>>>   hw/scsi-defs.h |    6 +++++-
>>>   hw/scsi-disk.c |   48 ++++++++++++++++++++++++------------------------
>>>   2 files changed, 29 insertions(+), 25 deletions(-)
>>>
> [ .. ]
>>> @@ -1217,44 +1214,47 @@ static int scsi_initfn(SCSIDevice *dev, SCSIDriveKind kind)
>>>           return -1;
>>>       }
>>>
>>> -    if (kind == SCSI_CD) {
>>> +    if (scsi_type == TYPE_ROM) {
>>>           s->qdev.blocksize = 2048;
>>> -    } else {
>>> +    } else if (scsi_type == TYPE_DISK) {
>>>           s->qdev.blocksize = s->qdev.conf.logical_block_size;
>>> +    } else {
>>> +        error_report("scsi-disk: Unhandled SCSI type %02x", scsi_type);
>>> +        return -1;
>>>       }
>>>       s->cluster_size = s->qdev.blocksize / 512;
>>>       s->bs->buffer_alignment = s->qdev.blocksize;
>>>
>>> -    s->qdev.type = TYPE_DISK;
>>> +    s->qdev.type = scsi_type;
>>
>> Is this a bug fix?
>>
> No, proper initialisation.
> s->qdev.type is the SCSI type we're told to emulate. So we have to set
> it to the correct value otherwise the emulation will return wrong
> values.

Well, it changes .type from TYPE_DISK to TYPE_ROM for scsi-cd.  I
understand how that is required for your patch to work.  I wonder
whether it has an impact beyond that, given that the old value is
plainly wrong.

>>>       qemu_add_vm_change_state_handler(scsi_dma_restart_cb, s);
>>> -    bdrv_set_removable(s->bs, kind == SCSI_CD);
>>> +    bdrv_set_removable(s->bs, scsi_type == TYPE_ROM);
>>>       add_boot_device_path(s->qdev.conf.bootindex,&dev->qdev, ",0");
>>>       return 0;
>>>   }
>>>
>>>   static int scsi_hd_initfn(SCSIDevice *dev)
>>>   {
>>> -    return scsi_initfn(dev, SCSI_HD);
>>> +    return scsi_initfn(dev, TYPE_DISK);
>>>   }
>>>
>>>   static int scsi_cd_initfn(SCSIDevice *dev)
>>>   {
>>> -    return scsi_initfn(dev, SCSI_CD);
>>> +    return scsi_initfn(dev, TYPE_ROM);
>>>   }
>>>
>>>   static int scsi_disk_initfn(SCSIDevice *dev)
>>>   {
>>> -    SCSIDriveKind kind;
>>>       DriveInfo *dinfo;
>>> +    uint8_t scsi_type = TYPE_DISK;
>>>
>>> -    if (!dev->conf.bs) {
>>> -        kind = SCSI_HD;         /* will die in scsi_initfn() */
>>
>> The comment explains why we don't explicitly fail when !dev->conf.bs,
>> like all the other block device models.  I'd rather keep it.
>>
> Ah. The magic of block devices. By all means, keep it.

Like this?


By the way, I'm afraid you forgot to remove typedef SCSIDriveKind.  Care
to respin this one?
diff mbox

Patch

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index f42a5d1..0925944 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -1251,17 +1251,17 @@  static int scsi_cd_initfn(SCSIDevice *dev)
 
 static int scsi_disk_initfn(SCSIDevice *dev)
 {
-    SCSIDriveKind kind;
+    uint8_t scsi_type;
     DriveInfo *dinfo;
 
     if (!dev->conf.bs) {
-        kind = SCSI_HD;         /* will die in scsi_initfn() */
+        scsi_type = TYPE_DISK;  /* will die in scsi_initfn() */
     } else {
         dinfo = drive_get_by_blockdev(dev->conf.bs);
-        kind = dinfo->media_cd ? SCSI_CD : SCSI_HD;
+        scsi_type = dinfo->media_cd ? TYPE_ROM : TYPE_DISK;
     }
 
-    return scsi_initfn(dev, kind);
+    return scsi_initfn(dev, scsi_type);
 }
 
 #define DEFINE_SCSI_DISK_PROPERTIES()                           \