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

[Qemu-devel] [PATCH v2 0/5] 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, scsi-disk.

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.

Remove the type hint from info block.  Its value is unreliable anyway.
libvirt doesn't use it.

I posted v1 quite some time ago.  Since we were working towards a
release then, we decided to take only the bonus bug fixes (PATCH 1-3),
and revisit the rest later.  Which has turned out to be "somewhat"
later than anticpiated.  Sorry about that.

v3:
* Trivially rebased
* fix if=ide: ide_create_drive() got HD vs. CD backwards (one-liner)
* ide-cd and scsi-cd devices suppress default CD-ROM (PATCH 6/6)

v2:
* Rebased
* Review comments addressed
* Reordered so that potentially controversial parts come later in the
  series
* More verbose commit messages

v1: http://lists.gnu.org/archive/html/qemu-devel/2010-07/msg00304.html

Markus Armbruster (6):
  ide: Split qdev "ide-drive" into "ide-hd" and "ide-cd"
  scsi: Split qdev "scsi-disk" into "scsi-hd" and "scsi-cd"
  block QMP: Drop query-block member "type" (type= in info block)
  blockdev: Store -drive option media in DriveInfo
  block: Remove type hint, it's guest matter, doesn't belong here
  defaults: ide-cd and scsi-cd devices suppress default CD-ROM

 block.c            |   32 +-----------
 block.h            |    5 --
 block_int.h        |    1 -
 blockdev.c         |    5 +-
 blockdev.h         |    1 +
 hw/ide/core.c      |   10 ++--
 hw/ide/internal.h  |    2 +-
 hw/ide/qdev.c      |   81 ++++++++++++++++++++++++-------
 hw/scsi-disk.c     |  137 +++++++++++++++++++++++++++++++++++++++-------------
 hw/xen_devconfig.c |    2 +-
 qmp-commands.hx    |    6 --
 vl.c               |    2 +
 12 files changed, 185 insertions(+), 99 deletions(-)

-- 
1.7.2.3

[Qemu-devel] [PATCH v3 1/6] ide: Split qdev "ide-drive" into "ide-hd" and "ide-cd"

[Markus Armbruster]

An "ide-drive" is either a hard disk or a CD-ROM, depending on the
associated BlockDriverState's type hint.  Unclean; disk vs. CD belongs
to the guest part, not the host part.

Have separate qdevs "ide-hd" and "ide-cd" to model disk vs. CD in
the guest part.

Keep ide-drive for backward compatibility.

"ide-disk" would perhaps be a nicer name than "ide-hd", but there's
already "scsi-disk", which is like "ide-drive", and will be likewise
split in the next commit.  {ide,scsi}-{hd,cd} is the best consistent
set of names I could find within the backward compatibility
straightjacket.

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

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 90f553b..542ed65 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1592,13 +1592,15 @@ void ide_bus_reset(IDEBus *bus)
     bus->dma->ops->reset(bus->dma);
 }
 
-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) {
@@ -1623,8 +1625,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);
         bs->buffer_alignment = 2048;
     } else {
@@ -1729,7 +1730,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 aa198b6..c2b35ec 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -558,7 +558,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 2bb5c27..3bca726 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -98,7 +98,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-cd" : "ide-hd");
     qdev_prop_set_uint32(dev, "unit", unit);
     qdev_prop_set_drive_nofail(dev, "drive", drive->bdrv);
     qdev_init_nofail(dev);
@@ -118,7 +120,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;
@@ -134,7 +136,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;
     }
 
@@ -151,22 +153,69 @@ static int ide_drive_initfn(IDEDevice *dev)
     return 0;
 }
 
-static IDEDeviceInfo ide_drive_info = {
-    .qdev.name  = "ide-drive",
-    .qdev.fw_name  = "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_DEV_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_dev_info[] = {
+    {
+        .qdev.name    = "ide-hd",
+        .qdev.fw_name = "drive",
+        .qdev.desc    = "virtual IDE disk",
+        .qdev.size    = sizeof(IDEDrive),
+        .init         = ide_hd_initfn,
+        .qdev.props   = (Property[]) {
+            DEFINE_IDE_DEV_PROPERTIES(),
+            DEFINE_PROP_END_OF_LIST(),
+        }
+    },{
+        .qdev.name    = "ide-cd",
+        .qdev.fw_name = "drive",
+        .qdev.desc    = "virtual IDE CD-ROM",
+        .qdev.size    = sizeof(IDEDrive),
+        .init         = ide_cd_initfn,
+        .qdev.props   = (Property[]) {
+            DEFINE_IDE_DEV_PROPERTIES(),
+            DEFINE_PROP_END_OF_LIST(),
+        }
+    },{
+        .qdev.name    = "ide-drive", /* legacy -device ide-drive */
+        .qdev.fw_name = "drive",
+        .qdev.desc    = "virtual IDE disk or CD-ROM (legacy)",
+        .qdev.size    = sizeof(IDEDrive),
+        .init         = ide_drive_initfn,
+        .qdev.props   = (Property[]) {
+            DEFINE_IDE_DEV_PROPERTIES(),
+            DEFINE_PROP_END_OF_LIST(),
+        }
     }
 };
 
-static void ide_drive_register(void)
+static void ide_dev_register(void)
 {
-    ide_qdev_register(&ide_drive_info);
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(ide_dev_info); i++) {
+        ide_qdev_register(&ide_dev_info[i]);
+    }
 }
-device_init(ide_drive_register);
+device_init(ide_dev_register);
-- 
1.7.2.3

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

[Markus Armbruster]

A "scsi-disk" is either a hard disk or a CD-ROM, depending on the
associated BlockDriverState's type hint.  Unclean; disk vs. CD belongs
to the guest part, not the host part.

Have separate qdevs "scsi-hd" and "scsi-cd" to model disk vs. CD in
the guest part.

Keep scsi-disk for backward compatibility.

Don't copy scsi-disk property removable to scsi-cd.  It's not used and
always zero(!) there.

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

diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index b05e654..8df8518 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -65,6 +65,8 @@ typedef struct SCSIDiskReq {
     uint32_t status;
 } SCSIDiskReq;
 
+typedef enum { SCSI_HD, SCSI_CD } SCSIDriveKind;
+
 struct SCSIDiskState
 {
     SCSIDevice qdev;
@@ -78,6 +80,7 @@ struct SCSIDiskState
     char *version;
     char *serial;
     SCSISense sense;
+    SCSIDriveKind drive_kind;
 };
 
 static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type);
@@ -406,7 +409,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->drive_kind == SCSI_CD) {
             outbuf[buflen++] = 5;
         } else {
             outbuf[buflen++] = 0;
@@ -424,7 +427,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             outbuf[buflen++] = 0x00; // list of supported pages (this page)
             outbuf[buflen++] = 0x80; // unit serial number
             outbuf[buflen++] = 0x83; // device identification
-            if (bdrv_get_type_hint(s->bs) != BDRV_TYPE_CDROM) {
+            if (s->drive_kind == SCSI_HD) {
                 outbuf[buflen++] = 0xb0; // block limits
                 outbuf[buflen++] = 0xb2; // thin provisioning
             }
@@ -477,7 +480,7 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf)
             unsigned int opt_io_size =
                     s->qdev.conf.opt_io_size / s->qdev.blocksize;
 
-            if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) {
+            if (s->drive_kind == SCSI_CD) {
                 DPRINTF("Inquiry (EVPD[%02X] not supported for CDROM\n",
                         page_code);
                 return -1;
@@ -547,7 +550,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->drive_kind == SCSI_CD) {
         outbuf[0] = 5;
         outbuf[1] = 0x80;
         memcpy(&outbuf[16], "QEMU CD-ROM     ", 16);
@@ -678,7 +681,7 @@ static int mode_sense_page(SCSIRequest *req, int page, uint8_t *p,
         return p[1] + 2;
 
     case 0x2a: /* CD Capabilities and Mechanical Status page. */
-        if (bdrv_get_type_hint(bdrv) != BDRV_TYPE_CDROM)
+        if (s->drive_kind != SCSI_CD)
             return 0;
         p[0] = 0x2a;
         p[1] = 0x14;
@@ -905,7 +908,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, 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->drive_kind == SCSI_CD && (req->cmd.buf[4] & 2)) {
             /* load/eject medium */
             bdrv_eject(s->bs, !(req->cmd.buf[4] & 1));
         }
@@ -1232,10 +1235,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, SCSIDriveKind kind)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
-    int is_cd;
     DriveInfo *dinfo;
 
     if (!s->qdev.conf.bs) {
@@ -1243,9 +1245,9 @@ 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->drive_kind = kind;
 
-    if (!is_cd && !bdrv_is_inserted(s->bs)) {
+    if (kind == SCSI_HD && !bdrv_is_inserted(s->bs)) {
         error_report("Device needs media, but drive is empty");
         return -1;
     }
@@ -1265,7 +1267,7 @@ static int scsi_disk_initfn(SCSIDevice *dev)
         return -1;
     }
 
-    if (is_cd) {
+    if (kind == SCSI_CD) {
         s->qdev.blocksize = 2048;
     } else {
         s->qdev.blocksize = s->qdev.conf.logical_block_size;
@@ -1275,35 +1277,103 @@ static int scsi_disk_initfn(SCSIDevice *dev)
 
     s->qdev.type = TYPE_DISK;
     qemu_add_vm_change_state_handler(scsi_dma_restart_cb, s);
-    bdrv_set_removable(s->bs, is_cd);
+    bdrv_set_removable(s->bs, kind == SCSI_CD);
     add_boot_device_path(s->qdev.conf.bootindex, &dev->qdev, ",0");
     return 0;
 }
 
-static SCSIDeviceInfo scsi_disk_info = {
-    .qdev.name    = "scsi-disk",
-    .qdev.fw_name = "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_BIT("removable", SCSIDiskState, removable, 0, false),
-        DEFINE_PROP_END_OF_LIST(),
-    },
+static int scsi_hd_initfn(SCSIDevice *dev)
+{
+    return scsi_initfn(dev, SCSI_HD);
+}
+
+static int scsi_cd_initfn(SCSIDevice *dev)
+{
+    return scsi_initfn(dev, SCSI_CD);
+}
+
+static int scsi_disk_initfn(SCSIDevice *dev)
+{
+    SCSIDriveKind kind;
+
+    if (!dev->conf.bs) {
+        kind = SCSI_HD;         /* will die in scsi_initfn() */
+    } else {
+        kind = bdrv_get_type_hint(dev->conf.bs) == BDRV_TYPE_CDROM
+            ? SCSI_CD : SCSI_HD;
+    }
+
+    return scsi_initfn(dev, kind);
+}
+
+#define DEFINE_SCSI_DISK_PROPERTIES()                           \
+    DEFINE_BLOCK_PROPERTIES(SCSIDiskState, qdev.conf),          \
+    DEFINE_PROP_STRING("ver",  SCSIDiskState, version),         \
+    DEFINE_PROP_STRING("serial",  SCSIDiskState, serial)
+
+static SCSIDeviceInfo scsi_disk_info[] = {
+    {
+        .qdev.name    = "scsi-hd",
+        .qdev.fw_name = "disk",
+        .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_SCSI_DISK_PROPERTIES(),
+            DEFINE_PROP_BIT("removable", SCSIDiskState, removable, 0, false),
+            DEFINE_PROP_END_OF_LIST(),
+        }
+    },{
+        .qdev.name    = "scsi-cd",
+        .qdev.fw_name = "disk",
+        .qdev.desc    = "virtual SCSI CD-ROM",
+        .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_SCSI_DISK_PROPERTIES(),
+            DEFINE_PROP_END_OF_LIST(),
+        },
+    },{
+        .qdev.name    = "scsi-disk", /* legacy -device scsi-disk */
+        .qdev.fw_name = "disk",
+        .qdev.desc    = "virtual SCSI disk or CD-ROM (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_SCSI_DISK_PROPERTIES(),
+            DEFINE_PROP_BIT("removable", SCSIDiskState, removable, 0, false),
+            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.7.2.3

[Qemu-devel] [PATCH v3 3/6] 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 +++-----------------
 qmp-commands.hx |    6 ------
 2 files changed, 3 insertions(+), 23 deletions(-)

diff --git a/block.c b/block.c
index f731c7a..c6dc42a 100644
--- a/block.c
+++ b/block.c
@@ -1704,9 +1704,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")) {
@@ -1747,23 +1746,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/qmp-commands.hx b/qmp-commands.hx
index fbd98ee..b1c8206 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1038,8 +1038,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
@@ -1070,25 +1068,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.7.2.3

[Qemu-devel] [PATCH v3 4/6] blockdev: Store -drive option media in DriveInfo

[Markus Armbruster]

DriveInfo is closely tied to -drive, and like -drive, it mixes
information about host and guest part of the block device.  Unlike
DriveInfo, BlockDriverState should be about the host part only.

One of the remaining guest bits there is the "type hint".  -drive
option media sets it, and qdevs "ide-drive", "scsi-disk" and non-qdev
IF_XEN devices check it to pick HD vs. CD.

Communicate -drive option media via new DriveInfo member media_cd
instead.

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     |    5 +++--
 hw/xen_devconfig.c |    2 +-
 6 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 5429621..28727df 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -488,6 +488,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
 	    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 2c9e780..3587786 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -33,6 +33,7 @@ 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 542ed65..45410e8 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1731,8 +1731,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 3bca726..3f9dc89 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -98,9 +98,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-cd" : "ide-hd");
+    dev = qdev_create(&bus->qbus, drive->media_cd ? "ide-cd" : "ide-hd");
     qdev_prop_set_uint32(dev, "unit", unit);
     qdev_prop_set_drive_nofail(dev, "drive", drive->bdrv);
     qdev_init_nofail(dev);
@@ -165,9 +163,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_DEV_PROPERTIES()                     \
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 8df8518..397b9d6 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -1295,12 +1295,13 @@ static int scsi_cd_initfn(SCSIDevice *dev)
 static int scsi_disk_initfn(SCSIDevice *dev)
 {
     SCSIDriveKind kind;
+    DriveInfo *dinfo;
 
     if (!dev->conf.bs) {
         kind = SCSI_HD;         /* will die in scsi_initfn() */
     } else {
-        kind = bdrv_get_type_hint(dev->conf.bs) == BDRV_TYPE_CDROM
-            ? SCSI_CD : SCSI_HD;
+        dinfo = drive_get_by_blockdev(dev->conf.bs);
+        kind = dinfo->media_cd ? SCSI_CD : SCSI_HD;
     }
 
     return scsi_initfn(dev, kind);
diff --git a/hw/xen_devconfig.c b/hw/xen_devconfig.c
index 8d50216..3a92155 100644
--- a/hw/xen_devconfig.c
+++ b/hw/xen_devconfig.c
@@ -96,7 +96,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.7.2.3

[Qemu-devel] [PATCH v3 5/6] block: Remove type hint, it's guest matter, doesn't belong here

[Markus Armbruster]

No users of bdrv_get_type_hint() 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 c6dc42a..cf3c8b7 100644
--- a/block.c
+++ b/block.c
@@ -1305,13 +1305,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;
@@ -1428,11 +1421,6 @@ void bdrv_get_floppy_geometry_hint(BlockDriverState *bs, int *nb_heads,
     }
 }
 
-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 52e9cad..da7d39c 100644
--- a/block.h
+++ b/block.h
@@ -152,9 +152,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
@@ -163,7 +160,6 @@ 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);
@@ -177,7 +173,6 @@ typedef enum FDriveType {
 void bdrv_get_floppy_geometry_hint(BlockDriverState *bs, int *nb_heads,
                                    int *max_track, int *last_sect,
                                    FDriveType drive_in, FDriveType *drive);
-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 545ad11..fa91337 100644
--- a/block_int.h
+++ b/block_int.h
@@ -194,7 +194,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 28727df..6e0eb83 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -487,7 +487,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
             }
 	    break;
 	case MEDIA_CDROM:
-            bdrv_set_type_hint(dinfo->bdrv, BDRV_TYPE_CDROM);
+            bdrv_set_removable(dinfo->bdrv, 1);
             dinfo->media_cd = 1;
 	    break;
 	}
@@ -496,7 +496,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
         /* 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.7.2.3

[Qemu-devel] [PATCH v3 6/6] defaults: ide-cd and scsi-cd devices suppress default CD-ROM

[Markus Armbruster]

ide-hd does *not* suppress the default CD-ROM, unlike legacy
ide-drive.

scsi-cd *does* suppress it, unlike legacy scsi-disk.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 vl.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/vl.c b/vl.c
index 6b9a2f6..8bd7cc6 100644
--- a/vl.c
+++ b/vl.c
@@ -277,7 +277,9 @@ static struct {
     { .driver = "isa-serial",           .flag = &default_serial    },
     { .driver = "isa-parallel",         .flag = &default_parallel  },
     { .driver = "isa-fdc",              .flag = &default_floppy    },
+    { .driver = "ide-cd",               .flag = &default_cdrom     },
     { .driver = "ide-drive",            .flag = &default_cdrom     },
+    { .driver = "scsi-cd",              .flag = &default_cdrom     },
     { .driver = "virtio-serial-pci",    .flag = &default_virtcon   },
     { .driver = "virtio-serial-s390",   .flag = &default_virtcon   },
     { .driver = "virtio-serial",        .flag = &default_virtcon   },
-- 
1.7.2.3

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

[Markus Armbruster]

Pasto: this is PATCH v3.  Sorry.

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

[Luiz Capitulino]

On Thu, 12 May 2011 17:05:12 +0200
Markus Armbruster <armbru@redhat.com> wrote:

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

It reports how the guest is using the device, right? I'd say that's what
users/clients are interested in knowing.

Also, we can't just drop it from QMP. We should first note it's deprecated.

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

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 12 May 2011 17:05:12 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> 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.
>
> It reports how the guest is using the device, right? I'd say that's what
> users/clients are interested in knowing.

The value is *unreliable*.  It may or may not match how the guest is
using the device.  I doubt users are interested in unreliable
information.

> Also, we can't just drop it from QMP. We should first note it's deprecated.

Would you accept a change to the more honest value "unknown" for the
deprecation period?

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

[Luiz Capitulino]

On Thu, 12 May 2011 19:12:56 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Thu, 12 May 2011 17:05:12 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> 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.
> >
> > It reports how the guest is using the device, right? I'd say that's what
> > users/clients are interested in knowing.
> 
> The value is *unreliable*.  It may or may not match how the guest is
> using the device.  I doubt users are interested in unreliable
> information.

Can't it be fixed? And how are users/clients supposed to find out how
the guest is using its block devices?

> > Also, we can't just drop it from QMP. We should first note it's deprecated.
> 
> Would you accept a change to the more honest value "unknown" for the
> deprecation period?

We have to avoid breaking the protocol. Changing something that has always
been reported as 'cdrom' to 'unknown' will likely cause as many as damages
as dropping the command.

The best solution I can think of is noting in the documentation that the
information is unreliable and explain what clients interested in knowing
this info should do.

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

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 12 May 2011 19:12:56 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Thu, 12 May 2011 17:05:12 +0200
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> 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.
>> >
>> > It reports how the guest is using the device, right? I'd say that's what
>> > users/clients are interested in knowing.
>> 
>> The value is *unreliable*.  It may or may not match how the guest is
>> using the device.  I doubt users are interested in unreliable
>> information.
>
> Can't it be fixed? And how are users/clients supposed to find out how
> the guest is using its block devices?

To find out more about the guest's devices, examine the guest's devices:
info qtree.

You don't expect to find the guest serial devices in in "info chardev",
either.  query-block's type member is a mistake, because it mixes up
guest device info with the host device info.  Dropping it is a bug fix.
The fact that its value is unreliable is merely icing on the cake.

>> > Also, we can't just drop it from QMP. We should first note it's deprecated.
>> 
>> Would you accept a change to the more honest value "unknown" for the
>> deprecation period?
>
> We have to avoid breaking the protocol. Changing something that has always
> been reported as 'cdrom' to 'unknown' will likely cause as many as damages
> as dropping the command.

I can cause damage only if somebody is using it.  Which I doubt.
Remember, the value is unreliable.  It's a *lie*.  We can stop lying in
two ways: shut up (drop member "type"), or tell the truth (change the
value to "unknown", which is a documented value of "type").

> The best solution I can think of is noting in the documentation that the
> information is unreliable and explain what clients interested in knowing
> this info should do.

I'd be much more willing to jump through compatibility hoops if there
was *one* known user of this particular detail of QMP.

But if you insist on us continuing to lie, I'll find a way to continue
to lie.  I'm resisting it, because I think it's a disservice to our
users.

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

[Luiz Capitulino]

On Thu, 12 May 2011 19:54:40 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Thu, 12 May 2011 19:12:56 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > On Thu, 12 May 2011 17:05:12 +0200
> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >
> >> >> 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.
> >> >
> >> > It reports how the guest is using the device, right? I'd say that's what
> >> > users/clients are interested in knowing.
> >> 
> >> The value is *unreliable*.  It may or may not match how the guest is
> >> using the device.  I doubt users are interested in unreliable
> >> information.
> >
> > Can't it be fixed? And how are users/clients supposed to find out how
> > the guest is using its block devices?
> 
> To find out more about the guest's devices, examine the guest's devices:
> info qtree.

Which is not converted to QMP yet.

> You don't expect to find the guest serial devices in in "info chardev",
> either.  query-block's type member is a mistake, because it mixes up
> guest device info with the host device info.  Dropping it is a bug fix.

I understand why you're dropping it, what I don't want to do is to break
working clients. For example, there might be clients out there using it
in a way that it's expected to work (eg. if=floppy).

Of course that I'm assuming that such a client exist.

> The fact that its value is unreliable is merely icing on the cake.
> 
> >> > Also, we can't just drop it from QMP. We should first note it's deprecated.
> >> 
> >> Would you accept a change to the more honest value "unknown" for the
> >> deprecation period?
> >
> > We have to avoid breaking the protocol. Changing something that has always
> > been reported as 'cdrom' to 'unknown' will likely cause as many as damages
> > as dropping the command.
> 
> I can cause damage only if somebody is using it.  Which I doubt.

Me too and I'd agree with this patch if I was 100% sure. But it's impossible
to be sure, unless we do it by trial and error which is harmful.

> Remember, the value is unreliable.  It's a *lie*.  We can stop lying in
> two ways: shut up (drop member "type"), or tell the truth (change the
> value to "unknown", which is a documented value of "type").

Can we set it to 'unknown' when if=none?

> > The best solution I can think of is noting in the documentation that the
> > information is unreliable and explain what clients interested in knowing
> > this info should do.
> 
> I'd be much more willing to jump through compatibility hoops if there
> was *one* known user of this particular detail of QMP.

Ideally, yes, but it's the type of thing we'll never know.

> But if you insist on us continuing to lie, I'll find a way to continue
> to lie.  I'm resisting it, because I think it's a disservice to our
> users.

I also want to do the best for our users and I don't want to ignore the bug,
but we don't know whether there are clients using the field. If we drop it
and a client does use it, then we'll have failed in doing the best.

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

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 12 May 2011 19:54:40 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Thu, 12 May 2011 19:12:56 +0200
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> 
>> >> > On Thu, 12 May 2011 17:05:12 +0200
>> >> > Markus Armbruster <armbru@redhat.com> wrote:
>> >> >
>> >> >> 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.
>> >> >
>> >> > It reports how the guest is using the device, right? I'd say that's what
>> >> > users/clients are interested in knowing.
>> >> 
>> >> The value is *unreliable*.  It may or may not match how the guest is
>> >> using the device.  I doubt users are interested in unreliable
>> >> information.
>> >
>> > Can't it be fixed? And how are users/clients supposed to find out how
>> > the guest is using its block devices?
>> 
>> To find out more about the guest's devices, examine the guest's devices:
>> info qtree.
>
> Which is not converted to QMP yet.

It's where the information is.  No use looking for it where it isn't.
If users need it, we better convert info qtree.

>> You don't expect to find the guest serial devices in in "info chardev",
>> either.  query-block's type member is a mistake, because it mixes up
>> guest device info with the host device info.  Dropping it is a bug fix.
>
> I understand why you're dropping it, what I don't want to do is to break
> working clients. For example, there might be clients out there using it
> in a way that it's expected to work (eg. if=floppy).
>
> Of course that I'm assuming that such a client exist.

A client new enough to use QMP, old enough to use legacy if=floppy,
silly enough to query for information it already knows (where did I put
that drive again?), and brittle enough to break hard when it gets no
type or type "unknown".

>> The fact that its value is unreliable is merely icing on the cake.
>> 
>> >> > Also, we can't just drop it from QMP. We should first note it's deprecated.
>> >> 
>> >> Would you accept a change to the more honest value "unknown" for the
>> >> deprecation period?
>> >
>> > We have to avoid breaking the protocol. Changing something that has always
>> > been reported as 'cdrom' to 'unknown' will likely cause as many as damages
>> > as dropping the command.
>> 
>> I can cause damage only if somebody is using it.  Which I doubt.
>
> Me too and I'd agree with this patch if I was 100% sure. But it's impossible
> to be sure, unless we do it by trial and error which is harmful.
>
>> Remember, the value is unreliable.  It's a *lie*.  We can stop lying in
>> two ways: shut up (drop member "type"), or tell the truth (change the
>> value to "unknown", which is a documented value of "type").
>
> Can we set it to 'unknown' when if=none?

Maybe.  Makes query-block mix up host and guest information again.
Purging guest information from block.c is the point of this series.
Therefore, query-block can't be done in block.c anymore.  It needs to
move to blockdev.c, where the mixed-up-by-design DriveInfo is available.
It could move back when we finally clean up query-block.

Even with such compatibility gymnastics, it could still break your
hypothetical client.

>> > The best solution I can think of is noting in the documentation that the
>> > information is unreliable and explain what clients interested in knowing
>> > this info should do.
>> 
>> I'd be much more willing to jump through compatibility hoops if there
>> was *one* known user of this particular detail of QMP.
>
> Ideally, yes, but it's the type of thing we'll never know.
>
>> But if you insist on us continuing to lie, I'll find a way to continue
>> to lie.  I'm resisting it, because I think it's a disservice to our
>> users.
>
> I also want to do the best for our users and I don't want to ignore the bug,
> but we don't know whether there are clients using the field. If we drop it
> and a client does use it, then we'll have failed in doing the best.

Good enough is good enough.

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

[Luiz Capitulino]

On Fri, 13 May 2011 10:26:38 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Thu, 12 May 2011 19:54:40 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > On Thu, 12 May 2011 19:12:56 +0200
> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >
> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> >> 
> >> >> > On Thu, 12 May 2011 17:05:12 +0200
> >> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >> >
> >> >> >> 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.
> >> >> >
> >> >> > It reports how the guest is using the device, right? I'd say that's what
> >> >> > users/clients are interested in knowing.
> >> >> 
> >> >> The value is *unreliable*.  It may or may not match how the guest is
> >> >> using the device.  I doubt users are interested in unreliable
> >> >> information.
> >> >
> >> > Can't it be fixed? And how are users/clients supposed to find out how
> >> > the guest is using its block devices?
> >> 
> >> To find out more about the guest's devices, examine the guest's devices:
> >> info qtree.
> >
> > Which is not converted to QMP yet.
> 
> It's where the information is.  No use looking for it where it isn't.
> If users need it, we better convert info qtree.

Sure, we should do that.

> >> You don't expect to find the guest serial devices in in "info chardev",
> >> either.  query-block's type member is a mistake, because it mixes up
> >> guest device info with the host device info.  Dropping it is a bug fix.
> >
> > I understand why you're dropping it, what I don't want to do is to break
> > working clients. For example, there might be clients out there using it
> > in a way that it's expected to work (eg. if=floppy).
> >
> > Of course that I'm assuming that such a client exist.
> 
> A client new enough to use QMP, old enough to use legacy if=floppy,

Legacy option != unsupported option. I wouldn't blame anyone for doing weird
mixtures with our options, as our external interfaces are not easy to use.

And that's *our* problem, not user's. If the user did something stupid, it
probably means *we* did something stupid.

One could argue that this is an argument in favor of this change, and I'd
promptly agree, as my main point is not about not dropping it, but respecting
our deprecation policy.

> silly enough to query for information it already knows (where did I put
> that drive again?),

This assumes the client itself has started the VM, or it knows the VM
beforehand. What if it doesn't? Don't we support connecting to an already
started VM then?

> and brittle enough to break hard when it gets no
> type or type "unknown".

I said that changing something that always reported 'cdrom' to 'unknown'
is likely to cause damage. I can't tell whether it's going to break hard
or not.

> >> The fact that its value is unreliable is merely icing on the cake.
> >> 
> >> >> > Also, we can't just drop it from QMP. We should first note it's deprecated.
> >> >> 
> >> >> Would you accept a change to the more honest value "unknown" for the
> >> >> deprecation period?
> >> >
> >> > We have to avoid breaking the protocol. Changing something that has always
> >> > been reported as 'cdrom' to 'unknown' will likely cause as many as damages
> >> > as dropping the command.
> >> 
> >> I can cause damage only if somebody is using it.  Which I doubt.
> >
> > Me too and I'd agree with this patch if I was 100% sure. But it's impossible
> > to be sure, unless we do it by trial and error which is harmful.
> >
> >> Remember, the value is unreliable.  It's a *lie*.  We can stop lying in
> >> two ways: shut up (drop member "type"), or tell the truth (change the
> >> value to "unknown", which is a documented value of "type").
> >
> > Can we set it to 'unknown' when if=none?
> 
> Maybe.  Makes query-block mix up host and guest information again.

It's temporary, just to respect our deprecation policy and give us time
to provide a viable alternative.

> Purging guest information from block.c is the point of this series.
> Therefore, query-block can't be done in block.c anymore.  It needs to
> move to blockdev.c, where the mixed-up-by-design DriveInfo is available.
> It could move back when we finally clean up query-block.
> 
> Even with such compatibility gymnastics, it could still break your
> hypothetical client.

Our deprecation policy is not hypothetical. There isn't case by case. Either
we respect it or we don't.

> >> > The best solution I can think of is noting in the documentation that the
> >> > information is unreliable and explain what clients interested in knowing
> >> > this info should do.
> >> 
> >> I'd be much more willing to jump through compatibility hoops if there
> >> was *one* known user of this particular detail of QMP.
> >
> > Ideally, yes, but it's the type of thing we'll never know.
> >
> >> But if you insist on us continuing to lie, I'll find a way to continue
> >> to lie.  I'm resisting it, because I think it's a disservice to our
> >> users.
> >
> > I also want to do the best for our users and I don't want to ignore the bug,
> > but we don't know whether there are clients using the field. If we drop it
> > and a client does use it, then we'll have failed in doing the best.
> 
> Good enough is good enough.

Breaking our deprecation policy is not what I'd call good enough.

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

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 13 May 2011 10:26:38 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Thu, 12 May 2011 19:54:40 +0200
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> 
>> >> > On Thu, 12 May 2011 19:12:56 +0200
>> >> > Markus Armbruster <armbru@redhat.com> wrote:
>> >> >
>> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
[...]
>> >> >> > Also, we can't just drop it from QMP. We should first note it's deprecated.
>> >> >> 
>> >> >> Would you accept a change to the more honest value "unknown" for the
>> >> >> deprecation period?
>> >> >
>> >> > We have to avoid breaking the protocol. Changing something that has always
>> >> > been reported as 'cdrom' to 'unknown' will likely cause as many as damages
>> >> > as dropping the command.
>> >> 
>> >> I can cause damage only if somebody is using it.  Which I doubt.
>> >
>> > Me too and I'd agree with this patch if I was 100% sure. But it's impossible
>> > to be sure, unless we do it by trial and error which is harmful.
>> >
>> >> Remember, the value is unreliable.  It's a *lie*.  We can stop lying in
>> >> two ways: shut up (drop member "type"), or tell the truth (change the
>> >> value to "unknown", which is a documented value of "type").
>> >
>> > Can we set it to 'unknown' when if=none?
>> 
>> Maybe.  Makes query-block mix up host and guest information again.
>
> It's temporary, just to respect our deprecation policy and give us time
> to provide a viable alternative.
>
>> Purging guest information from block.c is the point of this series.
>> Therefore, query-block can't be done in block.c anymore.  It needs to
>> move to blockdev.c, where the mixed-up-by-design DriveInfo is available.
>> It could move back when we finally clean up query-block.
>> 
>> Even with such compatibility gymnastics, it could still break your
>> hypothetical client.
>
> Our deprecation policy is not hypothetical. There isn't case by case. Either
> we respect it or we don't.

My point is: even your deprecation policy cannot protect your
hypothetical client.

The only way to ensure not even the silliest hypothetical client breaks
is not to change anything.  Which means we cannot fix query-block not to
lie about the type, period.  Which means we have to deprecate
query-block wholesale to fix that bug.

Kevin, please apply patches 1+2+6.  Feel free to drop 3-5.

[...]

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

[Luiz Capitulino]

On Fri, 13 May 2011 16:13:55 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Fri, 13 May 2011 10:26:38 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > On Thu, 12 May 2011 19:54:40 +0200
> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >
> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> >> 
> >> >> > On Thu, 12 May 2011 19:12:56 +0200
> >> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >> >
> >> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> [...]
> >> >> >> > Also, we can't just drop it from QMP. We should first note it's deprecated.
> >> >> >> 
> >> >> >> Would you accept a change to the more honest value "unknown" for the
> >> >> >> deprecation period?
> >> >> >
> >> >> > We have to avoid breaking the protocol. Changing something that has always
> >> >> > been reported as 'cdrom' to 'unknown' will likely cause as many as damages
> >> >> > as dropping the command.
> >> >> 
> >> >> I can cause damage only if somebody is using it.  Which I doubt.
> >> >
> >> > Me too and I'd agree with this patch if I was 100% sure. But it's impossible
> >> > to be sure, unless we do it by trial and error which is harmful.
> >> >
> >> >> Remember, the value is unreliable.  It's a *lie*.  We can stop lying in
> >> >> two ways: shut up (drop member "type"), or tell the truth (change the
> >> >> value to "unknown", which is a documented value of "type").
> >> >
> >> > Can we set it to 'unknown' when if=none?
> >> 
> >> Maybe.  Makes query-block mix up host and guest information again.
> >
> > It's temporary, just to respect our deprecation policy and give us time
> > to provide a viable alternative.
> >
> >> Purging guest information from block.c is the point of this series.
> >> Therefore, query-block can't be done in block.c anymore.  It needs to
> >> move to blockdev.c, where the mixed-up-by-design DriveInfo is available.
> >> It could move back when we finally clean up query-block.
> >> 
> >> Even with such compatibility gymnastics, it could still break your
> >> hypothetical client.
> >
> > Our deprecation policy is not hypothetical. There isn't case by case. Either
> > we respect it or we don't.
> 
> My point is: even your deprecation policy cannot protect your
> hypothetical client.
> 
> The only way to ensure not even the silliest hypothetical client breaks
> is not to change anything.  Which means we cannot fix query-block not to
> lie about the type, period.  Which means we have to deprecate
> query-block wholesale to fix that bug.

Why don't we force the known broken cases (like if=none) to 'unknown',
note in the doc that the field is unreliable and specify that it can be
dropped in 2 or 3 releases?

> 
> Kevin, please apply patches 1+2+6.  Feel free to drop 3-5.
> 
> [...]
>