[Qemu-devel] [PATCH 0/8] Split ide-drive and scsi-disk qdevs, and more

[Qemu-devel] [PATCH 0/8] Split ide-drive and scsi-disk qdevs, and more

[Markus Armbruster]

This patch series is about purging the "type hint" from the block
layer.  My previous series cleaned up improper uses it.  Remaining
uses are info block and qdevs ide-drive, scsidisk.

Remove the type hint from info block.  Its value is unreliable anyway.

ide-drive and scsi-disk can either act as disk or as CD drive.  They
use their drive's type hint to decide between disk and CD.  This is
unclean.  Disk vs. CD needs to be in qdev, not BlockDriverState,
because it belongs to the drive's guest part.

Split them into separate devices for disk and CD.  Keep the old ones
for backward compatibility.

Bonus fix: reject empty drives unless media is removable (1-3/8).

This patch series is available at
git://repo.or.cz/qemu/armbru.git
tag block-qdev-split: this series, based on
tag block-fixes-2-v2: my previous series, based on
tag blockdev-base, which the current kevin/block

Markus Armbruster (8):
  virtio-pci: Check for virtio_blk_init() failure
  virtio-blk: Fix virtio-blk-s390 to require drive
  ide scsi virtio-blk: Reject empty drives unless media is removable
  block QMP: Drop query-block member "type" (type= in info block)
  ide: Split qdev "ide-drive" into "ide-hd" and "ide-cd"
  scsi: Split qdev "scsi-disk" into "scsi-hd" and "scsi-cd"
  blockdev: Store -drive option media in DriveInfo
  block: Remove type hint

 block.c            |   32 +------------
 block.h            |    5 --
 block_int.h        |    1 -
 blockdev.c         |    5 +-
 blockdev.h         |    1 +
 hw/ide/core.c      |   14 ++++--
 hw/ide/internal.h  |    2 +-
 hw/ide/qdev.c      |   70 +++++++++++++++++++++++------
 hw/scsi-disk.c     |  124 +++++++++++++++++++++++++++++++++++++++++-----------
 hw/virtio-blk.c    |   10 ++++
 hw/virtio-pci.c    |    5 +-
 hw/xen_devconfig.c |    2 +-
 qemu-monitor.hx    |    6 ---
 13 files changed, 185 insertions(+), 92 deletions(-)

[Qemu-devel] [PATCH 1/8] virtio-pci: Check for virtio_blk_init() failure

[Markus Armbruster]

It can't actually fail now, but the next commit will change that.

s390_virtio_blk_init() already checks for failure, but
virtio_blk_init_pci() doesn't.  Fix that.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/virtio-pci.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index c6ef825..c6edcc2 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -552,6 +552,9 @@ static int virtio_blk_init_pci(PCIDevice *pci_dev)
         return -1;
     }
     vdev = virtio_blk_init(&pci_dev->qdev, &proxy->block);
+    if (!vdev) {
+        return -1;
+    }
     vdev->nvectors = proxy->nvectors;
     virtio_init_pci(proxy, vdev,
                     PCI_VENDOR_ID_REDHAT_QUMRANET,
-- 
1.6.6.1

[Qemu-devel] [PATCH 2/8] virtio-blk: Fix virtio-blk-s390 to require drive

[Markus Armbruster]

Move the check from virtio_blk_init_pci(), where it protects only
virtio-blk-pci, to virtio_blk_init().  Without that, virtio-blk-s390
initializes without a drive.  I figure that can lead to null pointer
dereferences.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/virtio-blk.c |    6 ++++++
 hw/virtio-pci.c |    4 ----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 0bd57b5..2de1a5a 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -12,6 +12,7 @@
  */
 
 #include <qemu-common.h>
+#include "qemu-error.h"
 #include "virtio-blk.h"
 #ifdef __linux__
 # include <scsi/sg.h>
@@ -490,6 +491,11 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf)
     static int virtio_blk_id;
     DriveInfo *dinfo;
 
+    if (!conf->bs) {
+        error_report("virtio-blk-pci: drive property not set");
+        return NULL;
+    }
+
     s = (VirtIOBlock *)virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
                                           sizeof(struct virtio_blk_config),
                                           sizeof(VirtIOBlock));
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index c6edcc2..a4d6d6b 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -547,10 +547,6 @@ static int virtio_blk_init_pci(PCIDevice *pci_dev)
         proxy->class_code != PCI_CLASS_STORAGE_OTHER)
         proxy->class_code = PCI_CLASS_STORAGE_SCSI;
 
-    if (!proxy->block.bs) {
-        error_report("virtio-blk-pci: drive property not set");
-        return -1;
-    }
     vdev = virtio_blk_init(&pci_dev->qdev, &proxy->block);
     if (!vdev) {
         return -1;
-- 
1.6.6.1

[Qemu-devel] [PATCH 3/8] ide scsi virtio-blk: Reject empty drives unless media is removable

[Markus Armbruster]

Disks without media make no sense.  For SCSI, a Linux guest kernel
complains during boot.  I didn't try other combinations.

scsi-generic doesn't need the additional check, because it already
requires bdrv_is_sg(), which fails without media.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/core.c   |    4 ++++
 hw/scsi-disk.c  |    5 +++++
 hw/virtio-blk.c |    4 ++++
 3 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index af52c2c..e20f2e7 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2630,6 +2630,10 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs,
         s->drive_kind = IDE_CD;
         bdrv_set_change_cb(bs, cdrom_change_cb, s);
     } else {
+        if (!bdrv_is_inserted(s->bs)) {
+            error_report("Device needs media, but drive is empty");
+            return -1;
+        }
         if (bdrv_is_read_only(bs)) {
             error_report("Can't use a read-only drive");
             return -1;
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index c30709c..f43f2d0 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -1059,6 +1059,11 @@ static int scsi_disk_initfn(SCSIDevice *dev)
     s->bs = s->qdev.conf.bs;
     is_cd = bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM;
 
+    if (!is_cd && !bdrv_is_inserted(s->bs)) {
+        error_report("Device needs media, but drive is empty");
+        return -1;
+    }
+
     if (bdrv_get_on_error(s->bs, 1) != BLOCK_ERR_REPORT) {
         error_report("Device doesn't support drive option rerror");
         return -1;
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 2de1a5a..89ea20e 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -495,6 +495,10 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf)
         error_report("virtio-blk-pci: drive property not set");
         return NULL;
     }
+    if (!bdrv_is_inserted(conf->bs)) {
+        error_report("Device needs media, but drive is empty");
+        return NULL;
+    }
 
     s = (VirtIOBlock *)virtio_common_init("virtio-blk", VIRTIO_ID_BLOCK,
                                           sizeof(struct virtio_blk_config),
-- 
1.6.6.1

[Qemu-devel] [PATCH 4/8] block QMP: Drop query-block member "type" (type= in info block)

[Markus Armbruster]

Its value is unreliable: a block device used as floppy has type
"floppy" if created with if=floppy, but type "hd" if created with
if=none.

That's because with if=none, the type is at best a declaration of
intent: the drive can be connected to any guest device.  Its type is
really the guest device's business.  Reporting it here is wrong.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block.c         |   20 +++-----------------
 qemu-monitor.hx |    6 ------
 2 files changed, 3 insertions(+), 23 deletions(-)

diff --git a/block.c b/block.c
index 65cf4dc..6d419b9 100644
--- a/block.c
+++ b/block.c
@@ -1533,9 +1533,8 @@ static void bdrv_print_dict(QObject *obj, void *opaque)
 
     bs_dict = qobject_to_qdict(obj);
 
-    monitor_printf(mon, "%s: type=%s removable=%d",
+    monitor_printf(mon, "%s: removable=%d",
                         qdict_get_str(bs_dict, "device"),
-                        qdict_get_str(bs_dict, "type"),
                         qdict_get_bool(bs_dict, "removable"));
 
     if (qdict_get_bool(bs_dict, "removable")) {
@@ -1576,23 +1575,10 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
 
     QTAILQ_FOREACH(bs, &bdrv_states, list) {
         QObject *bs_obj;
-        const char *type = "unknown";
-
-        switch(bs->type) {
-        case BDRV_TYPE_HD:
-            type = "hd";
-            break;
-        case BDRV_TYPE_CDROM:
-            type = "cdrom";
-            break;
-        case BDRV_TYPE_FLOPPY:
-            type = "floppy";
-            break;
-        }
 
-        bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': %s, "
+        bs_obj = qobject_from_jsonf("{ 'device': %s, "
                                     "'removable': %i, 'locked': %i }",
-                                    bs->device_name, type, bs->removable,
+                                    bs->device_name, bs->removable,
                                     bs->locked);
 
         if (bs->drv) {
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 9f62b94..6ba8abc 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -1723,8 +1723,6 @@ is a json-array of all devices.
 Each json-object contain the following:
 
 - "device": device name (json-string)
-- "type": device type (json-string)
-         - Possible values: "hd", "cdrom", "floppy", "unknown"
 - "removable": true if the device is removable, false otherwise (json-bool)
 - "locked": true if the device is locked, false otherwise (json-bool)
 - "inserted": only present if the device is inserted, it is a json-object
@@ -1755,25 +1753,21 @@ Example:
                "encrypted":false,
                "file":"disks/test.img"
             },
-            "type":"hd"
          },
          {
             "device":"ide1-cd0",
             "locked":false,
             "removable":true,
-            "type":"cdrom"
          },
          {
             "device":"floppy0",
             "locked":false,
             "removable":true,
-            "type": "floppy"
          },
          {
             "device":"sd0",
             "locked":false,
             "removable":true,
-            "type":"floppy"
          }
       ]
    }
-- 
1.6.6.1

[Qemu-devel] [PATCH 5/8] ide: Split qdev "ide-drive" into "ide-hd" and "ide-cd"

[Markus Armbruster]

Disk vs. CD needs to be in qdev, because it belongs to the drive's
guest part.

Keep ide-drive for backward compatibility.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ide/core.c     |   11 +++++---
 hw/ide/internal.h |    2 +-
 hw/ide/qdev.c     |   72 ++++++++++++++++++++++++++++++++++++++++++----------
 3 files changed, 66 insertions(+), 19 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index e20f2e7..1287f11 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2595,13 +2595,15 @@ void ide_bus_reset(IDEBus *bus)
     ide_clear_hob(bus);
 }
 
-int ide_init_drive(IDEState *s, BlockDriverState *bs,
+int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
                    const char *version, const char *serial)
 {
     int cylinders, heads, secs;
     uint64_t nb_sectors;
 
     s->bs = bs;
+    s->drive_kind = kind;
+
     bdrv_get_geometry(bs, &nb_sectors);
     bdrv_guess_geometry(bs, &cylinders, &heads, &secs);
     if (cylinders < 1 || cylinders > 16383) {
@@ -2626,8 +2628,7 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs,
     s->smart_autosave = 1;
     s->smart_errors = 0;
     s->smart_selftest_count = 0;
-    if (bdrv_get_type_hint(bs) == BDRV_TYPE_CDROM) {
-        s->drive_kind = IDE_CD;
+    if (kind == IDE_CD) {
         bdrv_set_change_cb(bs, cdrom_change_cb, s);
     } else {
         if (!bdrv_is_inserted(s->bs)) {
@@ -2692,7 +2693,9 @@ void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
         dinfo = i == 0 ? hd0 : hd1;
         ide_init1(bus, i);
         if (dinfo) {
-            if (ide_init_drive(&bus->ifs[i], dinfo->bdrv, NULL,
+            if (ide_init_drive(&bus->ifs[i], dinfo->bdrv,
+                               bdrv_get_type_hint(dinfo->bdrv) == BDRV_TYPE_CDROM ? IDE_CD : IDE_HD,
+                               NULL,
                                *dinfo->serial ? dinfo->serial : NULL) < 0) {
                 error_report("Can't set up IDE drive %s", dinfo->id);
                 exit(1);
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 4165543..d5de33b 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -556,7 +556,7 @@ uint32_t ide_data_readw(void *opaque, uint32_t addr);
 void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
 uint32_t ide_data_readl(void *opaque, uint32_t addr);
 
-int ide_init_drive(IDEState *s, BlockDriverState *bs,
+int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
                    const char *version, const char *serial);
 void ide_init2(IDEBus *bus, qemu_irq irq);
 void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 53468ed..a7f0b22 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -82,7 +82,9 @@ IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive)
 {
     DeviceState *dev;
 
-    dev = qdev_create(&bus->qbus, "ide-drive");
+    dev = qdev_create(&bus->qbus,
+                      bdrv_get_type_hint(drive->bdrv) == BDRV_TYPE_CDROM
+                      ? "ide-hd" : "ide-cd");
     qdev_prop_set_uint32(dev, "unit", unit);
     qdev_prop_set_drive_nofail(dev, "drive", drive->bdrv);
     qdev_init_nofail(dev);
@@ -102,7 +104,7 @@ typedef struct IDEDrive {
     IDEDevice dev;
 } IDEDrive;
 
-static int ide_drive_initfn(IDEDevice *dev)
+static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
 {
     IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
     IDEState *s = bus->ifs + dev->unit;
@@ -118,7 +120,7 @@ static int ide_drive_initfn(IDEDevice *dev)
         }
     }
 
-    if (ide_init_drive(s, dev->conf.bs, dev->version, serial) < 0) {
+    if (ide_init_drive(s, dev->conf.bs, kind, dev->version, serial) < 0) {
         return -1;
     }
 
@@ -131,21 +133,63 @@ static int ide_drive_initfn(IDEDevice *dev)
     return 0;
 }
 
-static IDEDeviceInfo ide_drive_info = {
-    .qdev.name  = "ide-drive",
-    .qdev.size  = sizeof(IDEDrive),
-    .init       = ide_drive_initfn,
-    .qdev.props = (Property[]) {
-        DEFINE_PROP_UINT32("unit", IDEDrive, dev.unit, -1),
-        DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),
-        DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),
-        DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial),
-        DEFINE_PROP_END_OF_LIST(),
+static int ide_hd_initfn(IDEDevice *dev)
+{
+    return ide_dev_initfn(dev, IDE_HD);
+}
+
+static int ide_cd_initfn(IDEDevice *dev)
+{
+    return ide_dev_initfn(dev, IDE_CD);
+}
+
+static int ide_drive_initfn(IDEDevice *dev)
+{
+    return ide_dev_initfn(dev,
+                          bdrv_get_type_hint(dev->conf.bs) == BDRV_TYPE_CDROM
+                          ? IDE_CD : IDE_HD);
+}
+
+#define DEFINE_IDE_DRIVE_PROPERTIES()                           \
+    DEFINE_PROP_UINT32("unit", IDEDrive, dev.unit, -1),         \
+    DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),                \
+    DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),          \
+    DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial)
+    
+static IDEDeviceInfo ide_info[] = {
+    {
+        .qdev.name  = "ide-hd",
+        .qdev.size  = sizeof(IDEDrive),
+        .init       = ide_hd_initfn,
+        .qdev.props = (Property[]) {
+            DEFINE_IDE_DRIVE_PROPERTIES(),
+            DEFINE_PROP_END_OF_LIST(),
+        }
+    },{
+        .qdev.name  = "ide-cd",
+        .qdev.size  = sizeof(IDEDrive),
+        .init       = ide_cd_initfn,
+        .qdev.props = (Property[]) {
+            DEFINE_IDE_DRIVE_PROPERTIES(),
+            DEFINE_PROP_END_OF_LIST(),
+        }
+    },{
+        .qdev.name  = "ide-drive", /* legacy -device ide-drive */
+        .qdev.size  = sizeof(IDEDrive),
+        .init       = ide_drive_initfn,
+        .qdev.props = (Property[]) {
+            DEFINE_IDE_DRIVE_PROPERTIES(),
+            DEFINE_PROP_END_OF_LIST(),
+        }
     }
 };
 
 static void ide_drive_register(void)
 {
-    ide_qdev_register(&ide_drive_info);
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(ide_info); i++) {
+        ide_qdev_register(&ide_info[i]);
+    }
 }
 device_init(ide_drive_register);
-- 
1.6.6.1

[Qemu-devel] [PATCH 6/8] scsi: Split qdev "scsi-disk" into "scsi-hd" and "scsi-cd"

[Markus Armbruster]

Disk vs. CD needs to be in qdev, because it belongs to the drive's
guest part.

Keep scsi-disk for backward compatibility.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/scsi-disk.c |  117 +++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 91 insertions(+), 26 deletions(-)

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index f43f2d0..ebefcba 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -67,6 +67,7 @@ struct SCSIDiskState
     QEMUBH *bh;
     char *version;
     char *serial;
+    int is_cd;
 };
 
 static SCSIDiskReq *scsi_new_request(SCSIDevice *d, uint32_t tag, uint32_t lun)
@@ -339,7 +340,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             return -1;
         }
 
-        if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
+        if (s->is_cd) {
             outbuf[buflen++] = 5;
         } else {
             outbuf[buflen++] = 0;
@@ -452,7 +453,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
         return buflen;
     }
 
-    if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
+    if (s->is_cd) {
         outbuf[0] = 5;
         outbuf[1] = 0x80;
         memcpy(&outbuf[16], "QEMU CD-ROM     ", 16);
@@ -566,7 +567,7 @@ static int mode_sense_page(SCSIRequest *req, int page, uint8_t *p)
         return 20;
 
     case 0x2a: /* CD Capabilities and Mechanical Status page. */
-        if (bdrv_get_type_hint(bdrv) != BDRV_TYPE_CDROM)
+        if (!s->is_cd)
             return 0;
         p[0] = 0x2a;
         p[1] = 0x14;
@@ -755,7 +756,7 @@ static int scsi_disk_emulate_command(SCSIRequest *req, uint8_t *outbuf)
             goto illegal_request;
         break;
     case START_STOP:
-        if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM && (req->cmd.buf[4] & 2)) {
+        if (s->is_cd && (req->cmd.buf[4] & 2)) {
             /* load/eject medium */
             bdrv_eject(s->bs, !(req->cmd.buf[4] & 1));
         }
@@ -1046,10 +1047,9 @@ static void scsi_destroy(SCSIDevice *dev)
     blockdev_mark_auto_del(s->qdev.conf.bs);
 }
 
-static int scsi_disk_initfn(SCSIDevice *dev)
+static int scsi_initfn(SCSIDevice *dev, int is_cd)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
-    int is_cd;
     DriveInfo *dinfo;
 
     if (!s->qdev.conf.bs) {
@@ -1057,7 +1057,7 @@ static int scsi_disk_initfn(SCSIDevice *dev)
         return -1;
     }
     s->bs = s->qdev.conf.bs;
-    is_cd = bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM;
+    s->is_cd = is_cd;
 
     if (!is_cd && !bdrv_is_inserted(s->bs)) {
         error_report("Device needs media, but drive is empty");
@@ -1097,28 +1097,93 @@ static int scsi_disk_initfn(SCSIDevice *dev)
     return 0;
 }
 
-static SCSIDeviceInfo scsi_disk_info = {
-    .qdev.name    = "scsi-disk",
-    .qdev.desc    = "virtual scsi disk or cdrom",
-    .qdev.size    = sizeof(SCSIDiskState),
-    .qdev.reset   = scsi_disk_reset,
-    .init         = scsi_disk_initfn,
-    .destroy      = scsi_destroy,
-    .send_command = scsi_send_command,
-    .read_data    = scsi_read_data,
-    .write_data   = scsi_write_data,
-    .cancel_io    = scsi_cancel_io,
-    .get_buf      = scsi_get_buf,
-    .qdev.props   = (Property[]) {
-        DEFINE_BLOCK_PROPERTIES(SCSIDiskState, qdev.conf),
-        DEFINE_PROP_STRING("ver",  SCSIDiskState, version),
-        DEFINE_PROP_STRING("serial",  SCSIDiskState, serial),
-        DEFINE_PROP_END_OF_LIST(),
-    },
+static int scsi_hd_initfn(SCSIDevice *dev)
+{
+    return scsi_initfn(dev, 0);
+}
+
+static int scsi_cd_initfn(SCSIDevice *dev)
+{
+    return scsi_initfn(dev, 1);
+}
+
+static int scsi_disk_initfn(SCSIDevice *dev)
+{
+    int is_cd;
+
+    if (!dev->conf.bs) {
+        is_cd = 0;              /* will die in scsi_initfn() */
+    } else {
+        is_cd = bdrv_get_type_hint(dev->conf.bs) == BDRV_TYPE_CDROM;
+    }
+
+    return scsi_initfn(dev, is_cd);
+}
+
+static SCSIDeviceInfo scsi_disk_info[] = {
+    {
+        .qdev.name    = "scsi-hd",
+        .qdev.desc    = "virtual scsi disk",
+        .qdev.size    = sizeof(SCSIDiskState),
+        .qdev.reset   = scsi_disk_reset,
+        .init         = scsi_hd_initfn,
+        .destroy      = scsi_destroy,
+        .send_command = scsi_send_command,
+        .read_data    = scsi_read_data,
+        .write_data   = scsi_write_data,
+        .cancel_io    = scsi_cancel_io,
+        .get_buf      = scsi_get_buf,
+        .qdev.props   = (Property[]) {
+            DEFINE_BLOCK_PROPERTIES(SCSIDiskState, qdev.conf),
+            DEFINE_PROP_STRING("ver",  SCSIDiskState, version),
+            DEFINE_PROP_STRING("serial",  SCSIDiskState, serial),
+            DEFINE_PROP_END_OF_LIST(),
+        }
+    },{
+        .qdev.name    = "scsi-cd",
+        .qdev.desc    = "virtual scsi cdrom",
+        .qdev.size    = sizeof(SCSIDiskState),
+        .qdev.reset   = scsi_disk_reset,
+        .init         = scsi_cd_initfn,
+        .destroy      = scsi_destroy,
+        .send_command = scsi_send_command,
+        .read_data    = scsi_read_data,
+        .write_data   = scsi_write_data,
+        .cancel_io    = scsi_cancel_io,
+        .get_buf      = scsi_get_buf,
+        .qdev.props   = (Property[]) {
+            DEFINE_BLOCK_PROPERTIES(SCSIDiskState, qdev.conf),
+            DEFINE_PROP_STRING("ver",  SCSIDiskState, version),
+            DEFINE_PROP_STRING("serial",  SCSIDiskState, serial),
+            DEFINE_PROP_END_OF_LIST(),
+        },
+    },{
+        .qdev.name    = "scsi-disk", /* legacy -device scsi-disk */
+        .qdev.desc    = "virtual scsi disk or cdrom (legacy)",
+        .qdev.size    = sizeof(SCSIDiskState),
+        .qdev.reset   = scsi_disk_reset,
+        .init         = scsi_disk_initfn,
+        .destroy      = scsi_destroy,
+        .send_command = scsi_send_command,
+        .read_data    = scsi_read_data,
+        .write_data   = scsi_write_data,
+        .cancel_io    = scsi_cancel_io,
+        .get_buf      = scsi_get_buf,
+        .qdev.props   = (Property[]) {
+            DEFINE_BLOCK_PROPERTIES(SCSIDiskState, qdev.conf),
+            DEFINE_PROP_STRING("ver",  SCSIDiskState, version),
+            DEFINE_PROP_STRING("serial",  SCSIDiskState, serial),
+            DEFINE_PROP_END_OF_LIST(),
+        }
+    }
 };
 
 static void scsi_disk_register_devices(void)
 {
-    scsi_qdev_register(&scsi_disk_info);
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(scsi_disk_info); i++) {
+        scsi_qdev_register(&scsi_disk_info[i]);
+    }
 }
 device_init(scsi_disk_register_devices)
-- 
1.6.6.1

[Qemu-devel] [PATCH 7/8] blockdev: Store -drive option media in DriveInfo

[Markus Armbruster]

Use it instead of bdrv_get_type_hint() to pick HD vs. CD for IDE, SCSI
and XEN drives.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c         |    1 +
 blockdev.h         |    1 +
 hw/ide/core.c      |    3 +--
 hw/ide/qdev.c      |   10 ++++------
 hw/scsi-disk.c     |    4 +++-
 hw/xen_devconfig.c |    2 +-
 6 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index cca3eec..02b4c22 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -437,6 +437,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
 	    break;
 	case MEDIA_CDROM:
             bdrv_set_type_hint(dinfo->bdrv, BDRV_TYPE_CDROM);
+            dinfo->media_cd = 1;
 	    break;
 	}
         break;
diff --git a/blockdev.h b/blockdev.h
index 37f3a01..2f3684c 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -32,6 +32,7 @@ typedef struct DriveInfo {
     int bus;
     int unit;
     int auto_del;               /* see blockdev_mark_auto_del() */
+    int media_cd;
     QemuOpts *opts;
     char serial[BLOCK_SERIAL_STRLEN + 1];
     QTAILQ_ENTRY(DriveInfo) next;
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 1287f11..e3ab22c 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2694,8 +2694,7 @@ void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
         ide_init1(bus, i);
         if (dinfo) {
             if (ide_init_drive(&bus->ifs[i], dinfo->bdrv,
-                               bdrv_get_type_hint(dinfo->bdrv) == BDRV_TYPE_CDROM ? IDE_CD : IDE_HD,
-                               NULL,
+                               dinfo->media_cd ? IDE_CD : IDE_HD, NULL,
                                *dinfo->serial ? dinfo->serial : NULL) < 0) {
                 error_report("Can't set up IDE drive %s", dinfo->id);
                 exit(1);
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index a7f0b22..30a1a8c 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -82,9 +82,7 @@ IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive)
 {
     DeviceState *dev;
 
-    dev = qdev_create(&bus->qbus,
-                      bdrv_get_type_hint(drive->bdrv) == BDRV_TYPE_CDROM
-                      ? "ide-hd" : "ide-cd");
+    dev = qdev_create(&bus->qbus, drive->media_cd ? "ide-hd" : "ide-cd");
     qdev_prop_set_uint32(dev, "unit", unit);
     qdev_prop_set_drive_nofail(dev, "drive", drive->bdrv);
     qdev_init_nofail(dev);
@@ -145,9 +143,9 @@ static int ide_cd_initfn(IDEDevice *dev)
 
 static int ide_drive_initfn(IDEDevice *dev)
 {
-    return ide_dev_initfn(dev,
-                          bdrv_get_type_hint(dev->conf.bs) == BDRV_TYPE_CDROM
-                          ? IDE_CD : IDE_HD);
+    DriveInfo *dinfo = drive_get_by_blockdev(dev->conf.bs);
+
+    return ide_dev_initfn(dev, dinfo->media_cd ? IDE_CD : IDE_HD);
 }
 
 #define DEFINE_IDE_DRIVE_PROPERTIES()                           \
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index ebefcba..06330e5 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -1110,11 +1110,13 @@ static int scsi_cd_initfn(SCSIDevice *dev)
 static int scsi_disk_initfn(SCSIDevice *dev)
 {
     int is_cd;
+    DriveInfo *dinfo;
 
     if (!dev->conf.bs) {
         is_cd = 0;              /* will die in scsi_initfn() */
     } else {
-        is_cd = bdrv_get_type_hint(dev->conf.bs) == BDRV_TYPE_CDROM;
+        dinfo = drive_get_by_blockdev(dev->conf.bs);
+        is_cd = dinfo->media_cd;
     }
 
     return scsi_initfn(dev, is_cd);
diff --git a/hw/xen_devconfig.c b/hw/xen_devconfig.c
index ea8f8c4..5d5ce65 100644
--- a/hw/xen_devconfig.c
+++ b/hw/xen_devconfig.c
@@ -94,7 +94,7 @@ int xen_config_dev_blk(DriveInfo *disk)
 {
     char fe[256], be[256];
     int vdev = 202 * 256 + 16 * disk->unit;
-    int cdrom = disk->bdrv->type == BDRV_TYPE_CDROM;
+    int cdrom = disk->media_cd;
     const char *devtype = cdrom ? "cdrom" : "disk";
     const char *mode    = cdrom ? "r"     : "w";
 
-- 
1.6.6.1

[Qemu-devel] [PATCH 8/8] block: Remove type hint

[Markus Armbruster]

No users left.

bdrv_set_type_hint() can make the media removable by side effect.
Make that explicit.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block.c     |   12 ------------
 block.h     |    5 -----
 block_int.h |    1 -
 blockdev.c  |    4 ++--
 4 files changed, 2 insertions(+), 20 deletions(-)

diff --git a/block.c b/block.c
index 6d419b9..5d3c676 100644
--- a/block.c
+++ b/block.c
@@ -1260,13 +1260,6 @@ void bdrv_set_geometry_hint(BlockDriverState *bs,
     bs->secs = secs;
 }
 
-void bdrv_set_type_hint(BlockDriverState *bs, int type)
-{
-    bs->type = type;
-    bs->removable = ((type == BDRV_TYPE_CDROM ||
-                      type == BDRV_TYPE_FLOPPY));
-}
-
 void bdrv_set_translation_hint(BlockDriverState *bs, int translation)
 {
     bs->translation = translation;
@@ -1280,11 +1273,6 @@ void bdrv_get_geometry_hint(BlockDriverState *bs,
     *psecs = bs->secs;
 }
 
-int bdrv_get_type_hint(BlockDriverState *bs)
-{
-    return bs->type;
-}
-
 int bdrv_get_translation_hint(BlockDriverState *bs)
 {
     return bs->translation;
diff --git a/block.h b/block.h
index c2a7e4c..4471b96 100644
--- a/block.h
+++ b/block.h
@@ -150,9 +150,6 @@ int bdrv_has_zero_init(BlockDriverState *bs);
 int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
 	int *pnum);
 
-#define BDRV_TYPE_HD     0
-#define BDRV_TYPE_CDROM  1
-#define BDRV_TYPE_FLOPPY 2
 #define BIOS_ATA_TRANSLATION_AUTO   0
 #define BIOS_ATA_TRANSLATION_NONE   1
 #define BIOS_ATA_TRANSLATION_LBA    2
@@ -161,11 +158,9 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
 
 void bdrv_set_geometry_hint(BlockDriverState *bs,
                             int cyls, int heads, int secs);
-void bdrv_set_type_hint(BlockDriverState *bs, int type);
 void bdrv_set_translation_hint(BlockDriverState *bs, int translation);
 void bdrv_get_geometry_hint(BlockDriverState *bs,
                             int *pcyls, int *pheads, int *psecs);
-int bdrv_get_type_hint(BlockDriverState *bs);
 int bdrv_get_translation_hint(BlockDriverState *bs);
 void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error,
                        BlockErrorAction on_write_error);
diff --git a/block_int.h b/block_int.h
index 877e1e5..df06409 100644
--- a/block_int.h
+++ b/block_int.h
@@ -186,7 +186,6 @@ struct BlockDriverState {
     /* NOTE: the following infos are only hints for real hardware
        drivers. They are not used by the block driver */
     int cyls, heads, secs, translation;
-    int type;
     BlockErrorAction on_read_error, on_write_error;
     char device_name[32];
     unsigned long *dirty_bitmap;
diff --git a/blockdev.c b/blockdev.c
index 02b4c22..506a68c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -436,7 +436,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
             }
 	    break;
 	case MEDIA_CDROM:
-            bdrv_set_type_hint(dinfo->bdrv, BDRV_TYPE_CDROM);
+            bdrv_set_removable(dinfo->bdrv, 1);
             dinfo->media_cd = 1;
 	    break;
 	}
@@ -445,7 +445,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
         /* FIXME: This isn't really a floppy, but it's a reasonable
            approximation.  */
     case IF_FLOPPY:
-        bdrv_set_type_hint(dinfo->bdrv, BDRV_TYPE_FLOPPY);
+        bdrv_set_removable(dinfo->bdrv, 1);
         break;
     case IF_PFLASH:
     case IF_MTD:
-- 
1.6.6.1

[Qemu-devel] Re: [PATCH 4/8] block QMP: Drop query-block member "type" (type= in info block)

[Kevin Wolf]

Am 06.07.2010 14:37, schrieb Markus Armbruster:
> Its value is unreliable: a block device used as floppy has type
> "floppy" if created with if=floppy, but type "hd" if created with
> if=none.
> 
> That's because with if=none, the type is at best a declaration of
> intent: the drive can be connected to any guest device.  Its type is
> really the guest device's business.  Reporting it here is wrong.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Dan, I'd like to have your Acked-by for this patch before applying it.
Can libvirt handle such a change in the monitor output, or does it even
use info block?

Kevin

> ---
>  block.c         |   20 +++-----------------
>  qemu-monitor.hx |    6 ------
>  2 files changed, 3 insertions(+), 23 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 65cf4dc..6d419b9 100644
> --- a/block.c
> +++ b/block.c
> @@ -1533,9 +1533,8 @@ static void bdrv_print_dict(QObject *obj, void *opaque)
>  
>      bs_dict = qobject_to_qdict(obj);
>  
> -    monitor_printf(mon, "%s: type=%s removable=%d",
> +    monitor_printf(mon, "%s: removable=%d",
>                          qdict_get_str(bs_dict, "device"),
> -                        qdict_get_str(bs_dict, "type"),
>                          qdict_get_bool(bs_dict, "removable"));
>  
>      if (qdict_get_bool(bs_dict, "removable")) {
> @@ -1576,23 +1575,10 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
>  
>      QTAILQ_FOREACH(bs, &bdrv_states, list) {
>          QObject *bs_obj;
> -        const char *type = "unknown";
> -
> -        switch(bs->type) {
> -        case BDRV_TYPE_HD:
> -            type = "hd";
> -            break;
> -        case BDRV_TYPE_CDROM:
> -            type = "cdrom";
> -            break;
> -        case BDRV_TYPE_FLOPPY:
> -            type = "floppy";
> -            break;
> -        }
>  
> -        bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': %s, "
> +        bs_obj = qobject_from_jsonf("{ 'device': %s, "
>                                      "'removable': %i, 'locked': %i }",
> -                                    bs->device_name, type, bs->removable,
> +                                    bs->device_name, bs->removable,
>                                      bs->locked);
>  
>          if (bs->drv) {
> diff --git a/qemu-monitor.hx b/qemu-monitor.hx
> index 9f62b94..6ba8abc 100644
> --- a/qemu-monitor.hx
> +++ b/qemu-monitor.hx
> @@ -1723,8 +1723,6 @@ is a json-array of all devices.
>  Each json-object contain the following:
>  
>  - "device": device name (json-string)
> -- "type": device type (json-string)
> -         - Possible values: "hd", "cdrom", "floppy", "unknown"
>  - "removable": true if the device is removable, false otherwise (json-bool)
>  - "locked": true if the device is locked, false otherwise (json-bool)
>  - "inserted": only present if the device is inserted, it is a json-object
> @@ -1755,25 +1753,21 @@ Example:
>                 "encrypted":false,
>                 "file":"disks/test.img"
>              },
> -            "type":"hd"
>           },
>           {
>              "device":"ide1-cd0",
>              "locked":false,
>              "removable":true,
> -            "type":"cdrom"
>           },
>           {
>              "device":"floppy0",
>              "locked":false,
>              "removable":true,
> -            "type": "floppy"
>           },
>           {
>              "device":"sd0",
>              "locked":false,
>              "removable":true,
> -            "type":"floppy"
>           }
>        ]
>     }

[Qemu-devel] Re: [PATCH 4/8] block QMP: Drop query-block member "type" (type= in info block)

[Daniel P. Berrange]

On Tue, Jul 06, 2010 at 06:39:53PM +0200, Kevin Wolf wrote:
> Am 06.07.2010 14:37, schrieb Markus Armbruster:
> > Its value is unreliable: a block device used as floppy has type
> > "floppy" if created with if=floppy, but type "hd" if created with
> > if=none.
> > 
> > That's because with if=none, the type is at best a declaration of
> > intent: the drive can be connected to any guest device.  Its type is
> > really the guest device's business.  Reporting it here is wrong.
> > 
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> Dan, I'd like to have your Acked-by for this patch before applying it.
> Can libvirt handle such a change in the monitor output, or does it even
> use info block?

We don't use the 'info block' or query-block commands at this
point in time, only 'info blockstats'/'query-blockstats'. So 
it should be fine to drop this field from libvirt's POV.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

[Qemu-devel] Re: [PATCH 1/8] virtio-pci: Check for virtio_blk_init() failure

[Christoph Hellwig]

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

[Qemu-devel] Re: [PATCH 2/8] virtio-blk: Fix virtio-blk-s390 to require drive

[Christoph Hellwig]

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

[Qemu-devel] Re: [PATCH 3/8] ide scsi virtio-blk: Reject empty drives unless media is removable

[Christoph Hellwig]

On Tue, Jul 06, 2010 at 02:37:44PM +0200, Markus Armbruster wrote:
> Disks without media make no sense.  For SCSI, a Linux guest kernel
> complains during boot.  I didn't try other combinations.
> 
> scsi-generic doesn't need the additional check, because it already
> requires bdrv_is_sg(), which fails without media.

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

[Qemu-devel] Re: [PATCH 4/8] block QMP: Drop query-block member "type" (type= in info block)

[Christoph Hellwig]

Looks correct to me,


Reviewed-by: Christoph Hellwig <hch@lst.de>

[Qemu-devel] Re: [PATCH 5/8] ide: Split qdev "ide-drive" into "ide-hd" and "ide-cd"

[Christoph Hellwig]

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

[Qemu-devel] Re: [PATCH 6/8] scsi: Split qdev "scsi-disk" into "scsi-hd" and "scsi-cd"

[Christoph Hellwig]

On Tue, Jul 06, 2010 at 02:37:47PM +0200, Markus Armbruster wrote:
> Disk vs. CD needs to be in qdev, because it belongs to the drive's
> guest part.

Looks good, but the scsi-hd name feelds kinda awkward.  This is one
case we're I'm really wondering if the compatiblity is worth it or
if we should just keep using scsi-disk for the real disk.

Reviewed-by: Christoph Hellwig <hch@lst.de>

[Qemu-devel] Re: [PATCH 7/8] blockdev: Store -drive option media in DriveInfo

[Christoph Hellwig]

I'd rather have an enum about the drive type instead of the is_cdrom
flag.  In addition I think patch 8 should be folded into this one.

[Qemu-devel] Re: [PATCH 6/8] scsi: Split qdev "scsi-disk" into "scsi-hd" and "scsi-cd"

[Kevin Wolf]

Am 07.07.2010 03:37, schrieb Christoph Hellwig:
> On Tue, Jul 06, 2010 at 02:37:47PM +0200, Markus Armbruster wrote:
>> Disk vs. CD needs to be in qdev, because it belongs to the drive's
>> guest part.
> 
> Looks good, but the scsi-hd name feelds kinda awkward.  This is one
> case we're I'm really wondering if the compatiblity is worth it or
> if we should just keep using scsi-disk for the real disk.

In any case the name should be consistent with ide-hd. And I'm not sure
if it's really helpful to have ide-drive and ide-disk.

Kevin

[Qemu-devel] Re: [PATCH 6/8] scsi: Split qdev "scsi-disk" into "scsi-hd" and "scsi-cd"

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 07.07.2010 03:37, schrieb Christoph Hellwig:
>> On Tue, Jul 06, 2010 at 02:37:47PM +0200, Markus Armbruster wrote:
>>> Disk vs. CD needs to be in qdev, because it belongs to the drive's
>>> guest part.
>> 
>> Looks good, but the scsi-hd name feelds kinda awkward.  This is one
>> case we're I'm really wondering if the compatiblity is worth it or
>> if we should just keep using scsi-disk for the real disk.
>
> In any case the name should be consistent with ide-hd. And I'm not sure
> if it's really helpful to have ide-drive and ide-disk.

{ide,scsi}-{hd,cd} is the best consistent set of names I could find
within the backward compatibility straightjacket.

By the way, we could use a way to mark qdevs and properties deprecated.

[Qemu-devel] Re: [PATCH 5/8] ide: Split qdev "ide-drive" into "ide-hd" and "ide-cd"

[Kevin Wolf]

Am 06.07.2010 14:37, schrieb Markus Armbruster:
> Disk vs. CD needs to be in qdev, because it belongs to the drive's
> guest part.
> 
> Keep ide-drive for backward compatibility.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/ide/core.c     |   11 +++++---
>  hw/ide/internal.h |    2 +-
>  hw/ide/qdev.c     |   72 ++++++++++++++++++++++++++++++++++++++++++----------
>  3 files changed, 66 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index e20f2e7..1287f11 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2595,13 +2595,15 @@ void ide_bus_reset(IDEBus *bus)
>      ide_clear_hob(bus);
>  }
>  
> -int ide_init_drive(IDEState *s, BlockDriverState *bs,
> +int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
>                     const char *version, const char *serial)
>  {
>      int cylinders, heads, secs;
>      uint64_t nb_sectors;
>  
>      s->bs = bs;
> +    s->drive_kind = kind;
> +
>      bdrv_get_geometry(bs, &nb_sectors);
>      bdrv_guess_geometry(bs, &cylinders, &heads, &secs);
>      if (cylinders < 1 || cylinders > 16383) {
> @@ -2626,8 +2628,7 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs,
>      s->smart_autosave = 1;
>      s->smart_errors = 0;
>      s->smart_selftest_count = 0;
> -    if (bdrv_get_type_hint(bs) == BDRV_TYPE_CDROM) {
> -        s->drive_kind = IDE_CD;
> +    if (kind == IDE_CD) {
>          bdrv_set_change_cb(bs, cdrom_change_cb, s);
>      } else {
>          if (!bdrv_is_inserted(s->bs)) {
> @@ -2692,7 +2693,9 @@ void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
>          dinfo = i == 0 ? hd0 : hd1;
>          ide_init1(bus, i);
>          if (dinfo) {
> -            if (ide_init_drive(&bus->ifs[i], dinfo->bdrv, NULL,
> +            if (ide_init_drive(&bus->ifs[i], dinfo->bdrv,
> +                               bdrv_get_type_hint(dinfo->bdrv) == BDRV_TYPE_CDROM ? IDE_CD : IDE_HD,
> +                               NULL,
>                                 *dinfo->serial ? dinfo->serial : NULL) < 0) {
>                  error_report("Can't set up IDE drive %s", dinfo->id);
>                  exit(1);
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 4165543..d5de33b 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -556,7 +556,7 @@ uint32_t ide_data_readw(void *opaque, uint32_t addr);
>  void ide_data_writel(void *opaque, uint32_t addr, uint32_t val);
>  uint32_t ide_data_readl(void *opaque, uint32_t addr);
>  
> -int ide_init_drive(IDEState *s, BlockDriverState *bs,
> +int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
>                     const char *version, const char *serial);
>  void ide_init2(IDEBus *bus, qemu_irq irq);
>  void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
> diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
> index 53468ed..a7f0b22 100644
> --- a/hw/ide/qdev.c
> +++ b/hw/ide/qdev.c
> @@ -82,7 +82,9 @@ IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive)
>  {
>      DeviceState *dev;
>  
> -    dev = qdev_create(&bus->qbus, "ide-drive");
> +    dev = qdev_create(&bus->qbus,
> +                      bdrv_get_type_hint(drive->bdrv) == BDRV_TYPE_CDROM
> +                      ? "ide-hd" : "ide-cd");
>      qdev_prop_set_uint32(dev, "unit", unit);
>      qdev_prop_set_drive_nofail(dev, "drive", drive->bdrv);
>      qdev_init_nofail(dev);
> @@ -102,7 +104,7 @@ typedef struct IDEDrive {
>      IDEDevice dev;
>  } IDEDrive;
>  
> -static int ide_drive_initfn(IDEDevice *dev)
> +static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
>  {
>      IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus);
>      IDEState *s = bus->ifs + dev->unit;
> @@ -118,7 +120,7 @@ static int ide_drive_initfn(IDEDevice *dev)
>          }
>      }
>  
> -    if (ide_init_drive(s, dev->conf.bs, dev->version, serial) < 0) {
> +    if (ide_init_drive(s, dev->conf.bs, kind, dev->version, serial) < 0) {
>          return -1;
>      }
>  
> @@ -131,21 +133,63 @@ static int ide_drive_initfn(IDEDevice *dev)
>      return 0;
>  }
>  
> -static IDEDeviceInfo ide_drive_info = {
> -    .qdev.name  = "ide-drive",
> -    .qdev.size  = sizeof(IDEDrive),
> -    .init       = ide_drive_initfn,
> -    .qdev.props = (Property[]) {
> -        DEFINE_PROP_UINT32("unit", IDEDrive, dev.unit, -1),
> -        DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),
> -        DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),
> -        DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial),
> -        DEFINE_PROP_END_OF_LIST(),
> +static int ide_hd_initfn(IDEDevice *dev)
> +{
> +    return ide_dev_initfn(dev, IDE_HD);
> +}
> +
> +static int ide_cd_initfn(IDEDevice *dev)
> +{
> +    return ide_dev_initfn(dev, IDE_CD);
> +}
> +
> +static int ide_drive_initfn(IDEDevice *dev)
> +{
> +    return ide_dev_initfn(dev,
> +                          bdrv_get_type_hint(dev->conf.bs) == BDRV_TYPE_CDROM
> +                          ? IDE_CD : IDE_HD);
> +}
> +
> +#define DEFINE_IDE_DRIVE_PROPERTIES()                           \
> +    DEFINE_PROP_UINT32("unit", IDEDrive, dev.unit, -1),         \
> +    DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),                \
> +    DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),          \
> +    DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial)
> +    

Whitespace error.

If you don't need to respin, I'll fix it when applying the series.

Kevin

[Qemu-devel] Re: [PATCH 0/8] Split ide-drive and scsi-disk qdevs, and more

[Kevin Wolf]

Am 06.07.2010 14:37, schrieb Markus Armbruster:
> This patch series is about purging the "type hint" from the block
> layer.  My previous series cleaned up improper uses it.  Remaining
> uses are info block and qdevs ide-drive, scsidisk.
> 
> Remove the type hint from info block.  Its value is unreliable anyway.
> 
> ide-drive and scsi-disk can either act as disk or as CD drive.  They
> use their drive's type hint to decide between disk and CD.  This is
> unclean.  Disk vs. CD needs to be in qdev, not BlockDriverState,
> because it belongs to the drive's guest part.
> 
> Split them into separate devices for disk and CD.  Keep the old ones
> for backward compatibility.
> 
> Bonus fix: reject empty drives unless media is removable (1-3/8).
> 
> This patch series is available at
> git://repo.or.cz/qemu/armbru.git
> tag block-qdev-split: this series, based on
> tag block-fixes-2-v2: my previous series, based on
> tag blockdev-base, which the current kevin/block
> 
> Markus Armbruster (8):
>   virtio-pci: Check for virtio_blk_init() failure
>   virtio-blk: Fix virtio-blk-s390 to require drive
>   ide scsi virtio-blk: Reject empty drives unless media is removable

Thanks, applied patches 1-3 to the block branch.

>   block QMP: Drop query-block member "type" (type= in info block)
>   ide: Split qdev "ide-drive" into "ide-hd" and "ide-cd"
>   scsi: Split qdev "scsi-disk" into "scsi-hd" and "scsi-cd"
>   blockdev: Store -drive option media in DriveInfo
>   block: Remove type hint

As discussed on IRC last week I'll wait for a respin for the remaining ones.

Kevin