Re: [Qemu-devel] [PATCH 5/6] scsi-disk: Remove 'drive_kind'

[Markus Armbruster <armbru@redhat.com>]

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?

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()                           \

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