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

[Hannes Reinecke]

On 07/26/2011 08:38 AM, Markus Armbruster wrote:
> Hannes Reinecke<address@hidden>  writes:
>
> > On 07/25/2011 05:59 PM, Markus Armbruster wrote:
> > > Hannes Reinecke<address@hidden>   writes:
> > >
> > > > Instead of using its own definitions scsi-disk should
> > > > be using the device type of the parent device.
> > > >
> > > > Signed-off-by: Hannes Reinecke<address@hidden>
> > > > ---
> > > >    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?
Here you go.

Cheers,

Hannes

--
Dr. Hannes Reinecke                   zSeries & Storage
address@hidden                        +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

scsi-disk-Remove-drive_kind.patch
Description: Text Data