[Qemu-devel] [PATCH v2 00/11] -drive/drive_add fixes and cleanups

[Qemu-devel] [PATCH v2 00/11] -drive/drive_add fixes and cleanups

[Markus Armbruster]

Kevin found a bug in my recent "[PATCH 3+5/5] -drive/drive_add fixes".
This is a rework of those two patches, plus a fix for the -drive
if=scsi,index=N regression, plus the odd bonus fix found on the way.

v2: fix for the -drive if=scsi,index=N regression.  PATCH 4 fixed up
slightly, PATCH 5 inserted, rest trivially rediffed.

Markus Armbruster (11):
  scsi hotplug: Set DriveInfo member bus correctly
  blockdev: New drive_get_next(), replacing qdev_init_bdrv()
  blockdev: Move BlockInterfaceType from qemu-common.h to blockdev.h
  blockdev: Put BlockInterfaceType names and max_devs in tables
  blockdev: Fix regression in -drive if=scsi,index=N
  blockdev: Make drive_add() take explicit type, index parameters
  blockdev: Replace drive_add()'s fmt, ... by optstr parameter
  blockdev: Factor drive_index_to_{bus,unit}_id out of drive_init()
  blockdev: New drive_get_by_index()
  blockdev: Reject multiple definitions for the same drive
  blockdev: Fix drive_add for drives without media

 blockdev.c          |  143 +++++++++++++++++++++++++++++++--------------------
 blockdev.h          |   18 +++++--
 hw/device-hotplug.c |    5 +-
 hw/ide.h            |    2 +
 hw/ide/ahci.c       |    1 -
 hw/pci-hotplug.c    |    1 +
 hw/pl181.c          |    7 ++-
 hw/qdev.c           |   15 -----
 hw/qdev.h           |    2 -
 hw/scsi.h           |    3 +-
 hw/ssi-sd.c         |    7 ++-
 hw/usb-msd.c        |    3 +-
 qemu-common.h       |    6 --
 savevm.c            |    1 -
 vl.c                |   94 +++++++++++++++++++--------------
 15 files changed, 171 insertions(+), 137 deletions(-)

-- 
1.7.2.3

[Qemu-devel] [PATCH v2 01/11] scsi hotplug: Set DriveInfo member bus correctly

[Markus Armbruster]

drive_init() picks the first free bus and unit number, unless the user
specifies them.

This isn't a good fit for the drive_add monitor command, because there
we specify the controller by PCI address instead of using bus number
set by drive_init().

scsi_hot_add() takes care to replace the unit number set by
drive_init() by the real one, but it neglects to replace the bus
number.  Thus, bus/unit in DriveInfo may be bogus.  Affects
drive_get() and drive_get_max_bus().  I'm not aware of anything bad
happening because of that; looks like by the time we're hot-plugging,
the two functions aren't used anymore.  Fix it anyway.

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

diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index 270a982..b6dcbda 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -90,6 +90,7 @@ static int scsi_hot_add(Monitor *mon, DeviceState *adapter,
      * specified).
      */
     dinfo->unit = qemu_opt_get_number(dinfo->opts, "unit", -1);
+    dinfo->bus = scsibus->busnr;
     scsidev = scsi_bus_legacy_add_drive(scsibus, dinfo->bdrv, dinfo->unit, false);
     if (!scsidev) {
         return -1;
-- 
1.7.2.3

[Qemu-devel] [PATCH v2 02/11] blockdev: New drive_get_next(), replacing qdev_init_bdrv()

[Markus Armbruster]

qdev_init_bdrv() doesn't belong into qdev.c; it's about drives, not
qdevs.  Rename to drive_get_next, move to blockdev.c, drop the bogus
DeviceState argument, and return DriveInfo instead of
BlockDriverState.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c  |   10 ++++++++++
 blockdev.h  |    1 +
 hw/pl181.c  |    7 ++++---
 hw/qdev.c   |   14 --------------
 hw/qdev.h   |    2 --
 hw/ssi-sd.c |    7 ++++---
 6 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index f7f591f..883117a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -93,6 +93,16 @@ int drive_get_max_bus(BlockInterfaceType type)
     return max_bus;
 }
 
+/* Get a block device.  This should only be used for single-drive devices
+   (e.g. SD/Floppy/MTD).  Multi-disk devices (scsi/ide) should use the
+   appropriate bus.  */
+DriveInfo *drive_get_next(BlockInterfaceType type)
+{
+    static int next_block_unit[IF_COUNT];
+
+    return drive_get(type, 0, next_block_unit[type]++);
+}
+
 DriveInfo *drive_get_by_blockdev(BlockDriverState *bs)
 {
     DriveInfo *dinfo;
diff --git a/blockdev.h b/blockdev.h
index 4536b5c..5c2144c 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -36,6 +36,7 @@ struct DriveInfo {
 
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
 int drive_get_max_bus(BlockInterfaceType type);
+DriveInfo *drive_get_next(BlockInterfaceType type);
 void drive_uninit(DriveInfo *dinfo);
 DriveInfo *drive_get_by_blockdev(BlockDriverState *bs);
 
diff --git a/hw/pl181.c b/hw/pl181.c
index 3e5f92f..36d9d02 100644
--- a/hw/pl181.c
+++ b/hw/pl181.c
@@ -7,6 +7,7 @@
  * This code is licenced under the GPL.
  */
 
+#include "blockdev.h"
 #include "sysbus.h"
 #include "sd.h"
 
@@ -449,15 +450,15 @@ static int pl181_init(SysBusDevice *dev)
 {
     int iomemtype;
     pl181_state *s = FROM_SYSBUS(pl181_state, dev);
-    BlockDriverState *bd;
+    DriveInfo *dinfo;
 
     iomemtype = cpu_register_io_memory(pl181_readfn, pl181_writefn, s,
                                        DEVICE_NATIVE_ENDIAN);
     sysbus_init_mmio(dev, 0x1000, iomemtype);
     sysbus_init_irq(dev, &s->irq[0]);
     sysbus_init_irq(dev, &s->irq[1]);
-    bd = qdev_init_bdrv(&dev->qdev, IF_SD);
-    s->card = sd_init(bd, 0);
+    dinfo = drive_get_next(IF_SD);
+    s->card = sd_init(dinfo ? dinfo->bdrv : NULL, 0);
     qemu_register_reset(pl181_reset, s);
     pl181_reset(s);
     /* ??? Save/restore.  */
diff --git a/hw/qdev.c b/hw/qdev.c
index 5b8d374..0c94fb2 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -458,20 +458,6 @@ void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd)
     }
 }
 
-static int next_block_unit[IF_COUNT];
-
-/* Get a block device.  This should only be used for single-drive devices
-   (e.g. SD/Floppy/MTD).  Multi-disk devices (scsi/ide) should use the
-   appropriate bus.  */
-BlockDriverState *qdev_init_bdrv(DeviceState *dev, BlockInterfaceType type)
-{
-    int unit = next_block_unit[type]++;
-    DriveInfo *dinfo;
-
-    dinfo = drive_get(type, 0, unit);
-    return dinfo ? dinfo->bdrv : NULL;
-}
-
 BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
 {
     BusState *bus;
diff --git a/hw/qdev.h b/hw/qdev.h
index e520aaa..9808f85 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -137,8 +137,6 @@ bool qdev_machine_modified(void);
 qemu_irq qdev_get_gpio_in(DeviceState *dev, int n);
 void qdev_connect_gpio_out(DeviceState *dev, int n, qemu_irq pin);
 
-BlockDriverState *qdev_init_bdrv(DeviceState *dev, BlockInterfaceType type);
-
 BusState *qdev_get_child_bus(DeviceState *dev, const char *name);
 
 /*** Device API.  ***/
diff --git a/hw/ssi-sd.c b/hw/ssi-sd.c
index a1a63b2..fb4b649 100644
--- a/hw/ssi-sd.c
+++ b/hw/ssi-sd.c
@@ -7,6 +7,7 @@
  * This code is licenced under the GNU GPL v2.
  */
 
+#include "blockdev.h"
 #include "ssi.h"
 #include "sd.h"
 
@@ -231,11 +232,11 @@ static int ssi_sd_load(QEMUFile *f, void *opaque, int version_id)
 static int ssi_sd_init(SSISlave *dev)
 {
     ssi_sd_state *s = FROM_SSI_SLAVE(ssi_sd_state, dev);
-    BlockDriverState *bs;
+    DriveInfo *dinfo;
 
     s->mode = SSI_SD_CMD;
-    bs = qdev_init_bdrv(&dev->qdev, IF_SD);
-    s->sd = sd_init(bs, 1);
+    dinfo = drive_get_next(IF_SD);
+    s->sd = sd_init(dinfo ? dinfo->bdrv : NULL, 1);
     register_savevm(&dev->qdev, "ssi_sd", -1, 1, ssi_sd_save, ssi_sd_load, s);
     return 0;
 }
-- 
1.7.2.3

[Qemu-devel] [PATCH v2 03/11] blockdev: Move BlockInterfaceType from qemu-common.h to blockdev.h

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.h    |    6 ++++++
 qemu-common.h |    6 ------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/blockdev.h b/blockdev.h
index 5c2144c..f616855 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -18,6 +18,12 @@ void blockdev_auto_del(BlockDriverState *bs);
 
 #define BLOCK_SERIAL_STRLEN 20
 
+typedef enum {
+    IF_NONE,
+    IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
+    IF_COUNT
+} BlockInterfaceType;
+
 struct DriveInfo {
     BlockDriverState *bdrv;
     char *id;
diff --git a/qemu-common.h b/qemu-common.h
index c351131..9cde519 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -267,12 +267,6 @@ typedef struct VirtIODevice VirtIODevice;
 
 typedef uint64_t pcibus_t;
 
-typedef enum {
-    IF_NONE,
-    IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
-    IF_COUNT
-} BlockInterfaceType;
-
 void cpu_exec_init_all(unsigned long tb_size);
 
 /* CPU save/load.  */
-- 
1.7.2.3

[Qemu-devel] [PATCH v2 04/11] blockdev: Put BlockInterfaceType names and max_devs in tables

[Markus Armbruster]

Turns drive_init()'s lengthy conditional into a concise loop, and
makes the data available elsewhere.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c |   51 +++++++++++++++++++++------------------------------
 1 files changed, 21 insertions(+), 30 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 883117a..7156cd1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -19,6 +19,23 @@
 
 static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
 
+static const char *const if_name[IF_COUNT] = {
+    [IF_NONE] = "none",
+    [IF_IDE] = "ide",
+    [IF_SCSI] = "scsi",
+    [IF_FLOPPY] = "floppy",
+    [IF_PFLASH] = "pflash",
+    [IF_MTD] = "mtd",
+    [IF_SD] = "sd",
+    [IF_VIRTIO] = "virtio",
+    [IF_XEN] = "xen",
+};
+
+static const int if_max_devs[IF_COUNT] = {
+    [IF_IDE] = MAX_IDE_DEVS,
+    [IF_SCSI] = MAX_SCSI_DEVS,
+};
+
 /*
  * We automatically delete the drive when a device using it gets
  * unplugged.  Questionable feature, but we can't just drop it.
@@ -173,11 +190,9 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
 
     if (default_to_scsi) {
         type = IF_SCSI;
-        max_devs = MAX_SCSI_DEVS;
         pstrcpy(devname, sizeof(devname), "scsi");
     } else {
         type = IF_IDE;
-        max_devs = MAX_IDE_DEVS;
         pstrcpy(devname, sizeof(devname), "ide");
     }
     media = MEDIA_DISK;
@@ -199,38 +214,14 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
 
     if ((buf = qemu_opt_get(opts, "if")) != NULL) {
         pstrcpy(devname, sizeof(devname), buf);
-        if (!strcmp(buf, "ide")) {
-	    type = IF_IDE;
-            max_devs = MAX_IDE_DEVS;
-        } else if (!strcmp(buf, "scsi")) {
-	    type = IF_SCSI;
-            max_devs = MAX_SCSI_DEVS;
-        } else if (!strcmp(buf, "floppy")) {
-	    type = IF_FLOPPY;
-            max_devs = 0;
-        } else if (!strcmp(buf, "pflash")) {
-	    type = IF_PFLASH;
-            max_devs = 0;
-	} else if (!strcmp(buf, "mtd")) {
-	    type = IF_MTD;
-            max_devs = 0;
-	} else if (!strcmp(buf, "sd")) {
-	    type = IF_SD;
-            max_devs = 0;
-        } else if (!strcmp(buf, "virtio")) {
-            type = IF_VIRTIO;
-            max_devs = 0;
-	} else if (!strcmp(buf, "xen")) {
-	    type = IF_XEN;
-            max_devs = 0;
-	} else if (!strcmp(buf, "none")) {
-	    type = IF_NONE;
-            max_devs = 0;
-	} else {
+        for (type = 0; type < IF_COUNT && strcmp(buf, if_name[type]); type++)
+            ;
+        if (type == IF_COUNT) {
             error_report("unsupported bus type '%s'", buf);
             return NULL;
 	}
     }
+    max_devs = if_max_devs[type];
 
     if (cyls || heads || secs) {
         if (cyls < 1 || (type == IF_IDE && cyls > 16383)) {
-- 
1.7.2.3

[Qemu-devel] [PATCH v2 05/11] blockdev: Fix regression in -drive if=scsi, index=N

[Markus Armbruster]

Before commit 622b520f, index=12 meant bus=1,unit=5.

Since the commit, it means bus=0,unit=12.  The drive is created, but
not the guest device.  That's because the controllers we use with
if=scsi drives (lsi53c895a and esp) support only 7 units, and
scsi_bus_legacy_handle_cmdline() ignores drives with unit numbers
exceeding that limit.

Changing the mapping of index to bus, unit is a regression.  Breaking
-drive invocations that used to work just makes it worse.

Revert the part of commit 622b520f that causes this, and clean up
some.

Note that the fix only affects if=scsi.  You can still put more than 7
units on a SCSI bus with -device & friends.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c    |   18 ++++++++++++++++--
 blockdev.h    |    3 ---
 hw/ide.h      |    2 ++
 hw/ide/ahci.c |    1 -
 hw/qdev.c     |    1 -
 hw/scsi.h     |    3 ++-
 savevm.c      |    1 -
 7 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 7156cd1..5c64549 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -32,8 +32,22 @@ static const char *const if_name[IF_COUNT] = {
 };
 
 static const int if_max_devs[IF_COUNT] = {
-    [IF_IDE] = MAX_IDE_DEVS,
-    [IF_SCSI] = MAX_SCSI_DEVS,
+    /*
+     * Do not change these numbers!  They govern how drive option
+     * index maps to unit and bus.  That mapping is ABI.
+     *
+     * All controllers used to imlement if=T drives need to support
+     * if_max_devs[T] units, for any T with if_max_devs[T] != 0.
+     * Otherwise, some index values map to "impossible" bus, unit
+     * values.
+     *
+     * For instance, if you change [IF_SCSI] to 255, -drive
+     * if=scsi,index=12 no longer means bus=1,unit=5, but
+     * bus=0,unit=12.  With an lsi53c895a controller (7 units max),
+     * the drive can't be set up.  Regression.
+     */
+    [IF_IDE] = 2,
+    [IF_SCSI] = 7,
 };
 
 /*
diff --git a/blockdev.h b/blockdev.h
index f616855..6e09a0b 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -37,9 +37,6 @@ struct DriveInfo {
     QTAILQ_ENTRY(DriveInfo) next;
 };
 
-#define MAX_IDE_DEVS	2
-#define MAX_SCSI_DEVS	255
-
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
 int drive_get_max_bus(BlockInterfaceType type);
 DriveInfo *drive_get_next(BlockInterfaceType type);
diff --git a/hw/ide.h b/hw/ide.h
index 2b5ae7c..73fb550 100644
--- a/hw/ide.h
+++ b/hw/ide.h
@@ -4,6 +4,8 @@
 #include "isa.h"
 #include "pci.h"
 
+#define MAX_IDE_DEVS	2
+
 /* ide-isa.c */
 ISADevice *isa_ide_init(int iobase, int iobase2, int isairq,
                         DriveInfo *hd0, DriveInfo *hd1);
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 968fdce..abf40f2 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -70,7 +70,6 @@
 #include "monitor.h"
 #include "dma.h"
 #include "cpu-common.h"
-#include "blockdev.h"
 #include "internal.h"
 #include <hw/ide/pci.h>
 
diff --git a/hw/qdev.c b/hw/qdev.c
index 0c94fb2..c7fec44 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -29,7 +29,6 @@
 #include "qdev.h"
 #include "sysemu.h"
 #include "monitor.h"
-#include "blockdev.h"
 
 static int qdev_hotplug = 0;
 static bool qdev_hot_added = false;
diff --git a/hw/scsi.h b/hw/scsi.h
index 846fbba..d3b5d56 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -3,9 +3,10 @@
 
 #include "qdev.h"
 #include "block.h"
-#include "blockdev.h"
 #include "block_int.h"
 
+#define MAX_SCSI_DEVS	255
+
 #define SCSI_CMD_BUF_SIZE     16
 
 /* scsi-disk.c */
diff --git a/savevm.c b/savevm.c
index fcd8db4..4453217 100644
--- a/savevm.c
+++ b/savevm.c
@@ -78,7 +78,6 @@
 #include "sysemu.h"
 #include "qemu-timer.h"
 #include "qemu-char.h"
-#include "blockdev.h"
 #include "audio/audio.h"
 #include "migration.h"
 #include "qemu_socket.h"
-- 
1.7.2.3

[Qemu-devel] [PATCH v2 06/11] blockdev: Make drive_add() take explicit type, index parameters

[Markus Armbruster]

Before, type & index were hidden in printf-like fmt, ... parameters,
which get expanded into an option string.  Rather inconvenient for
uses later in this series.

New IF_DEFAULT to ask for the machine's default interface.  Before,
that was done by having no option "if" in the option string.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c          |   20 +++++++++++++++++---
 blockdev.h          |    8 +++++++-
 hw/device-hotplug.c |    2 +-
 vl.c                |   43 +++++++++++++++++++++++--------------------
 4 files changed, 48 insertions(+), 25 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 5c64549..60dba1a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -75,20 +75,34 @@ void blockdev_auto_del(BlockDriverState *bs)
     }
 }
 
-QemuOpts *drive_add(const char *file, const char *fmt, ...)
+QemuOpts *drive_def(const char *optstr)
+{
+    return qemu_opts_parse(qemu_find_opts("drive"), optstr, 0);
+}
+
+QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
+                    const char *fmt, ...)
 {
     va_list ap;
     char optstr[1024];
     QemuOpts *opts;
+    char buf[32];
 
     va_start(ap, fmt);
     vsnprintf(optstr, sizeof(optstr), fmt, ap);
     va_end(ap);
 
-    opts = qemu_opts_parse(qemu_find_opts("drive"), optstr, 0);
+    opts = drive_def(optstr);
     if (!opts) {
         return NULL;
     }
+    if (type != IF_DEFAULT) {
+        qemu_opt_set(opts, "if", if_name[type]);
+    }
+    if (index >= 0) {
+        snprintf(buf, sizeof(buf), "%d", index);
+        qemu_opt_set(opts, "index", buf);
+    }
     if (file)
         qemu_opt_set(opts, "file", file);
     return opts;
@@ -473,7 +487,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
         if (devaddr)
             qemu_opt_set(opts, "addr", devaddr);
         break;
-    case IF_COUNT:
+    default:
         abort();
     }
     if (!file || !*file) {
diff --git a/blockdev.h b/blockdev.h
index 6e09a0b..eb76573 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -19,6 +19,7 @@ void blockdev_auto_del(BlockDriverState *bs);
 #define BLOCK_SERIAL_STRLEN 20
 
 typedef enum {
+    IF_DEFAULT = -1,            /* for use with drive_add() only */
     IF_NONE,
     IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
     IF_COUNT
@@ -43,7 +44,12 @@ DriveInfo *drive_get_next(BlockInterfaceType type);
 void drive_uninit(DriveInfo *dinfo);
 DriveInfo *drive_get_by_blockdev(BlockDriverState *bs);
 
-QemuOpts *drive_add(const char *file, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
+QemuOpts *drive_def(const char *optstr);
+QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
+                    const char *fmt, ...) /*GCC_FMT_ATTR(4, 5)*/;
+    /* GCC_FMT_ATTR() commented out to avoid the (pretty useless)
+     * "zero-length gnu_printf format string" warning we insist to
+     * enable */
 DriveInfo *drive_init(QemuOpts *arg, int default_to_scsi, int *fatal_error);
 
 /* device-hotplug */
diff --git a/hw/device-hotplug.c b/hw/device-hotplug.c
index 9704e2f..95a6372 100644
--- a/hw/device-hotplug.c
+++ b/hw/device-hotplug.c
@@ -33,7 +33,7 @@ DriveInfo *add_init_drive(const char *optstr)
     DriveInfo *dinfo;
     QemuOpts *opts;
 
-    opts = drive_add(NULL, "%s", optstr);
+    opts = drive_def(optstr);
     if (!opts)
         return NULL;
 
diff --git a/vl.c b/vl.c
index 14255c4..5399e33 100644
--- a/vl.c
+++ b/vl.c
@@ -621,12 +621,13 @@ static int bt_parse(const char *opt)
 /***********************************************************/
 /* QEMU Block devices */
 
-#define HD_ALIAS "index=%d,media=disk"
-#define CDROM_ALIAS "index=2,media=cdrom"
-#define FD_ALIAS "index=%d,if=floppy"
-#define PFLASH_ALIAS "if=pflash"
-#define MTD_ALIAS "if=mtd"
-#define SD_ALIAS "index=0,if=sd"
+/* Any % in the following strings must be escaped as %% */
+#define HD_OPTS "media=disk"
+#define CDROM_OPTS "media=cdrom"
+#define FD_OPTS ""
+#define PFLASH_OPTS ""
+#define MTD_OPTS ""
+#define SD_OPTS ""
 
 static int drive_init_func(QemuOpts *opts, void *opaque)
 {
@@ -1987,7 +1988,7 @@ int main(int argc, char **argv, char **envp)
         if (optind >= argc)
             break;
         if (argv[optind][0] != '-') {
-	    hda_opts = drive_add(argv[optind++], HD_ALIAS, 0);
+	    hda_opts = drive_add(IF_DEFAULT, 0, argv[optind++], HD_OPTS);
         } else {
             const QEMUOption *popt;
 
@@ -2027,11 +2028,11 @@ int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_hda:
                 if (cyls == 0)
-                    hda_opts = drive_add(optarg, HD_ALIAS, 0);
+                    hda_opts = drive_add(IF_DEFAULT, 0, optarg, HD_OPTS);
                 else
-                    hda_opts = drive_add(optarg, HD_ALIAS
+                    hda_opts = drive_add(IF_DEFAULT, 0, optarg, HD_OPTS
 			     ",cyls=%d,heads=%d,secs=%d%s",
-                             0, cyls, heads, secs,
+                             cyls, heads, secs,
                              translation == BIOS_ATA_TRANSLATION_LBA ?
                                  ",trans=lba" :
                              translation == BIOS_ATA_TRANSLATION_NONE ?
@@ -2040,10 +2041,11 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_hdb:
             case QEMU_OPTION_hdc:
             case QEMU_OPTION_hdd:
-                drive_add(optarg, HD_ALIAS, popt->index - QEMU_OPTION_hda);
+                drive_add(IF_DEFAULT, popt->index - QEMU_OPTION_hda, optarg,
+                          HD_OPTS);
                 break;
             case QEMU_OPTION_drive:
-                drive_add(NULL, "%s", optarg);
+                drive_def(optarg);
 	        break;
             case QEMU_OPTION_set:
                 if (qemu_set_option(optarg) != 0)
@@ -2054,13 +2056,13 @@ int main(int argc, char **argv, char **envp)
                     exit(1);
 	        break;
             case QEMU_OPTION_mtdblock:
-                drive_add(optarg, MTD_ALIAS);
+                drive_add(IF_MTD, -1, optarg, MTD_OPTS);
                 break;
             case QEMU_OPTION_sd:
-                drive_add(optarg, SD_ALIAS);
+                drive_add(IF_SD, 0, optarg, SD_OPTS);
                 break;
             case QEMU_OPTION_pflash:
-                drive_add(optarg, PFLASH_ALIAS);
+                drive_add(IF_PFLASH, -1, optarg, PFLASH_OPTS);
                 break;
             case QEMU_OPTION_snapshot:
                 snapshot = 1;
@@ -2139,7 +2141,7 @@ int main(int argc, char **argv, char **envp)
                 kernel_cmdline = optarg;
                 break;
             case QEMU_OPTION_cdrom:
-                drive_add(optarg, CDROM_ALIAS);
+                drive_add(IF_DEFAULT, 2, optarg, CDROM_OPTS);
                 break;
             case QEMU_OPTION_boot:
                 {
@@ -2192,7 +2194,8 @@ int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_fda:
             case QEMU_OPTION_fdb:
-                drive_add(optarg, FD_ALIAS, popt->index - QEMU_OPTION_fda);
+                drive_add(IF_FLOPPY, popt->index - QEMU_OPTION_fda,
+                          optarg, FD_OPTS);
                 break;
             case QEMU_OPTION_no_fd_bootchk:
                 fd_bootchk = 0;
@@ -2892,17 +2895,17 @@ int main(int argc, char **argv, char **envp)
 
     if (default_cdrom) {
         /* we always create the cdrom drive, even if no disk is there */
-        drive_add(NULL, CDROM_ALIAS);
+        drive_add(IF_DEFAULT, 2, NULL, CDROM_OPTS);
     }
 
     if (default_floppy) {
         /* we always create at least one floppy */
-        drive_add(NULL, FD_ALIAS, 0);
+        drive_add(IF_FLOPPY, 0, NULL, FD_OPTS);
     }
 
     if (default_sdcard) {
         /* we always create one sd slot, even if no card is in it */
-        drive_add(NULL, SD_ALIAS);
+        drive_add(IF_SD, 0, NULL, SD_OPTS);
     }
 
     /* open the virtual block devices */
-- 
1.7.2.3

[Qemu-devel] [PATCH v2 07/11] blockdev: Replace drive_add()'s fmt, ... by optstr parameter

[Markus Armbruster]

Let the callers build the optstr.  Only one wants to.  All the others
become simpler, because they don't have to worry about escaping '%'.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c |    8 +-------
 blockdev.h |    5 +----
 vl.c       |   13 +++++++------
 3 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 60dba1a..a5e56ee 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -81,17 +81,11 @@ QemuOpts *drive_def(const char *optstr)
 }
 
 QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
-                    const char *fmt, ...)
+                    const char *optstr)
 {
-    va_list ap;
-    char optstr[1024];
     QemuOpts *opts;
     char buf[32];
 
-    va_start(ap, fmt);
-    vsnprintf(optstr, sizeof(optstr), fmt, ap);
-    va_end(ap);
-
     opts = drive_def(optstr);
     if (!opts) {
         return NULL;
diff --git a/blockdev.h b/blockdev.h
index eb76573..3b9c5fd 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -46,10 +46,7 @@ DriveInfo *drive_get_by_blockdev(BlockDriverState *bs);
 
 QemuOpts *drive_def(const char *optstr);
 QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
-                    const char *fmt, ...) /*GCC_FMT_ATTR(4, 5)*/;
-    /* GCC_FMT_ATTR() commented out to avoid the (pretty useless)
-     * "zero-length gnu_printf format string" warning we insist to
-     * enable */
+                    const char *optstr);
 DriveInfo *drive_init(QemuOpts *arg, int default_to_scsi, int *fatal_error);
 
 /* device-hotplug */
diff --git a/vl.c b/vl.c
index 5399e33..2fa1ec0 100644
--- a/vl.c
+++ b/vl.c
@@ -621,7 +621,6 @@ static int bt_parse(const char *opt)
 /***********************************************************/
 /* QEMU Block devices */
 
-/* Any % in the following strings must be escaped as %% */
 #define HD_OPTS "media=disk"
 #define CDROM_OPTS "media=cdrom"
 #define FD_OPTS ""
@@ -1912,6 +1911,7 @@ int main(int argc, char **argv, char **envp)
     const char *incoming = NULL;
     int show_vnc_port = 0;
     int defconfig = 1;
+    char buf[256];
 
 #ifdef CONFIG_SIMPLE_TRACE
     const char *trace_file = NULL;
@@ -2028,16 +2028,17 @@ int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_hda:
                 if (cyls == 0)
-                    hda_opts = drive_add(IF_DEFAULT, 0, optarg, HD_OPTS);
+                    snprintf(buf, sizeof(buf), "%s", HD_OPTS);
                 else
-                    hda_opts = drive_add(IF_DEFAULT, 0, optarg, HD_OPTS
-			     ",cyls=%d,heads=%d,secs=%d%s",
-                             cyls, heads, secs,
+                    snprintf(buf, sizeof(buf),
+                             "%s,cyls=%d,heads=%d,secs=%d%s",
+                             HD_OPTS , cyls, heads, secs,
                              translation == BIOS_ATA_TRANSLATION_LBA ?
                                  ",trans=lba" :
                              translation == BIOS_ATA_TRANSLATION_NONE ?
                                  ",trans=none" : "");
-                 break;
+                drive_add(IF_DEFAULT, 0, optarg, buf);
+                break;
             case QEMU_OPTION_hdb:
             case QEMU_OPTION_hdc:
             case QEMU_OPTION_hdd:
-- 
1.7.2.3

[Qemu-devel] [PATCH v2 08/11] blockdev: Factor drive_index_to_{bus, unit}_id out of drive_init()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c |   22 ++++++++++++++--------
 1 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index a5e56ee..f0bed2e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -75,6 +75,18 @@ void blockdev_auto_del(BlockDriverState *bs)
     }
 }
 
+static int drive_index_to_bus_id(BlockInterfaceType type, int index)
+{
+    int max_devs = if_max_devs[type];
+    return max_devs ? index / max_devs : 0;
+}
+
+static int drive_index_to_unit_id(BlockInterfaceType type, int index)
+{
+    int max_devs = if_max_devs[type];
+    return max_devs ? index % max_devs : index;
+}
+
 QemuOpts *drive_def(const char *optstr)
 {
     return qemu_opts_parse(qemu_find_opts("drive"), optstr, 0);
@@ -376,14 +388,8 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
             error_report("index cannot be used with bus and unit");
             return NULL;
         }
-        if (max_devs == 0)
-        {
-            unit_id = index;
-            bus_id = 0;
-        } else {
-            unit_id = index % max_devs;
-            bus_id = index / max_devs;
-        }
+        bus_id = drive_index_to_bus_id(type, index);
+        unit_id = drive_index_to_unit_id(type, index);
     }
 
     /* if user doesn't specify a unit_id,
-- 
1.7.2.3

[Qemu-devel] [PATCH v2 09/11] blockdev: New drive_get_by_index()

[Markus Armbruster]

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

diff --git a/blockdev.c b/blockdev.c
index f0bed2e..2aea768 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -130,6 +130,13 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit)
     return NULL;
 }
 
+DriveInfo *drive_get_by_index(BlockInterfaceType type, int index)
+{
+    return drive_get(type,
+                     drive_index_to_bus_id(type, index),
+                     drive_index_to_unit_id(type, index));
+}
+
 int drive_get_max_bus(BlockInterfaceType type)
 {
     int max_bus;
diff --git a/blockdev.h b/blockdev.h
index 3b9c5fd..4009add 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -39,6 +39,7 @@ struct DriveInfo {
 };
 
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
+DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
 int drive_get_max_bus(BlockInterfaceType type);
 DriveInfo *drive_get_next(BlockInterfaceType type);
 void drive_uninit(DriveInfo *dinfo);
-- 
1.7.2.3

[Qemu-devel] [PATCH v2 10/11] blockdev: Reject multiple definitions for the same drive

[Markus Armbruster]

We silently ignore multiple definitions for the same drive:

    $ qemu-system-x86_64 -nodefaults -vnc :1 -S -monitor stdio -drive if=ide,index=1,file=tmp.qcow2 -drive if=ide,index=1,file=nonexistant
    QEMU 0.13.50 monitor - type 'help' for more information
    (qemu) info block
    ide0-hd1: type=hd removable=0 file=tmp.qcow2 backing_file=tmp.img ro=0 drv=qcow2 encrypted=0

With if=none, this can become quite confusing:

    $ qemu-system-x86_64 -nodefaults -vnc :1 -S -monitor stdio -drive if=none,index=1,file=tmp.qcow2,id=eins -drive if=none,index=1,file=nonexistant,id=zwei -device ide-drive,drive=eins -device ide-drive,drive=zwei
    qemu-system-x86_64: -device ide-drive,drive=zwei: Property 'ide-drive.drive' can't find value 'zwei'

The second -device fails, because it refers to drive zwei, which got
silently ignored.

Make multiple drive definitions fail cleanly.

Unfortunately, there's code that relies on multiple drive definitions
being silently ignored: main() merrily adds default drives even when
the user already defined these drives.  Fix that up.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c |    5 +++--
 vl.c       |   45 ++++++++++++++++++++++++++++++---------------
 2 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 2aea768..3daa0b2 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -423,11 +423,12 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
     }
 
     /*
-     * ignore multiple definitions
+     * catch multiple definitions
      */
 
     if (drive_get(type, bus_id, unit_id) != NULL) {
-        *fatal_error = 0;
+        error_report("drive with bus=%d, unit=%d (index=%d) exists",
+                     bus_id, unit_id, index);
         return NULL;
     }
 
diff --git a/vl.c b/vl.c
index 2fa1ec0..3637095 100644
--- a/vl.c
+++ b/vl.c
@@ -648,6 +648,29 @@ static int drive_enable_snapshot(QemuOpts *opts, void *opaque)
     return 0;
 }
 
+static void default_drive(int enable, int snapshot, int use_scsi,
+                          BlockInterfaceType type, int index,
+                          const char *optstr)
+{
+    QemuOpts *opts;
+
+    if (type == IF_DEFAULT) {
+        type = use_scsi ? IF_SCSI : IF_IDE;
+    }
+
+    if (!enable || drive_get_by_index(type, index)) {
+        return;
+    }
+
+    opts = drive_add(type, index, NULL, optstr);
+    if (snapshot) {
+        drive_enable_snapshot(opts, NULL);
+    }
+    if (drive_init_func(opts, &use_scsi)) {
+        exit(1);
+    }
+}
+
 void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque)
 {
     boot_set_handler = func;
@@ -2894,27 +2917,19 @@ int main(int argc, char **argv, char **envp)
 
     blk_mig_init();
 
-    if (default_cdrom) {
-        /* we always create the cdrom drive, even if no disk is there */
-        drive_add(IF_DEFAULT, 2, NULL, CDROM_OPTS);
-    }
-
-    if (default_floppy) {
-        /* we always create at least one floppy */
-        drive_add(IF_FLOPPY, 0, NULL, FD_OPTS);
-    }
-
-    if (default_sdcard) {
-        /* we always create one sd slot, even if no card is in it */
-        drive_add(IF_SD, 0, NULL, SD_OPTS);
-    }
-
     /* open the virtual block devices */
     if (snapshot)
         qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot, NULL, 0);
     if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func, &machine->use_scsi, 1) != 0)
         exit(1);
 
+    default_drive(default_cdrom, snapshot, machine->use_scsi,
+                  IF_DEFAULT, 2, CDROM_OPTS);
+    default_drive(default_floppy, snapshot, machine->use_scsi,
+                  IF_FLOPPY, 0, FD_OPTS);
+    default_drive(default_sdcard, snapshot, machine->use_scsi,
+                  IF_SD, 0, SD_OPTS);
+
     register_savevm_live(NULL, "ram", 0, 4, NULL, ram_save_live, NULL,
                          ram_load, NULL);
 
-- 
1.7.2.3

[Qemu-devel] [PATCH v2 11/11] blockdev: Fix drive_add for drives without media

[Markus Armbruster]

Watch this:

    (qemu) drive_add 0 if=none
    (qemu) info block
    none0: type=hd removable=0 [not inserted]
    (qemu) drive_del none0
    Segmentation fault (core dumped)

add_init_drive() is confused about drive_init()'s failure modes, and
cleans up when it shouldn't.  This leaves the DriveInfo with member
opts dangling.  drive_del attempts to free it, and dies.

drive_init() behaves as follows:

* If it created a drive with media, it returns its DriveInfo.

* If it created a drive without media, it clears *fatal_error and
  returns NULL.

* If it couldn't create a drive, it sets *fatal_error and returns
  NULL.

Of its three callers:

* drive_init_func() is correct.

* usb_msd_init() assumes drive_init() failed when it returns NULL.
  This is correct only because it always passes option "file", and
  "drive without media" can't happen then.

* add_init_drive() assumes drive_init() failed when it returns NULL.
  This is incorrect.

Clean up drive_init() to return NULL on failure and only on failure.
Drop its parameter fatal_error.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c          |    8 ++------
 blockdev.h          |    2 +-
 hw/device-hotplug.c |    3 +--
 hw/usb-msd.c        |    3 +--
 vl.c                |    9 ++-------
 5 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 3daa0b2..ecf3098 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -203,7 +203,7 @@ static int parse_block_error_action(const char *buf, int is_read)
     }
 }
 
-DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
+DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
 {
     const char *buf;
     const char *file = NULL;
@@ -225,8 +225,6 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
     int snapshot = 0;
     int ret;
 
-    *fatal_error = 1;
-
     translation = BIOS_ATA_TRANSLATION_AUTO;
 
     if (default_to_scsi) {
@@ -499,8 +497,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
         abort();
     }
     if (!file || !*file) {
-        *fatal_error = 0;
-        return NULL;
+        return dinfo;
     }
     if (snapshot) {
         /* always use cache=unsafe with snapshot */
@@ -529,7 +526,6 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
 
     if (bdrv_key_required(dinfo->bdrv))
         autostart = 0;
-    *fatal_error = 0;
     return dinfo;
 }
 
diff --git a/blockdev.h b/blockdev.h
index 4009add..50c586f 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -48,7 +48,7 @@ DriveInfo *drive_get_by_blockdev(BlockDriverState *bs);
 QemuOpts *drive_def(const char *optstr);
 QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
                     const char *optstr);
-DriveInfo *drive_init(QemuOpts *arg, int default_to_scsi, int *fatal_error);
+DriveInfo *drive_init(QemuOpts *arg, int default_to_scsi);
 
 /* device-hotplug */
 
diff --git a/hw/device-hotplug.c b/hw/device-hotplug.c
index 95a6372..8b2ed7a 100644
--- a/hw/device-hotplug.c
+++ b/hw/device-hotplug.c
@@ -29,7 +29,6 @@
 
 DriveInfo *add_init_drive(const char *optstr)
 {
-    int fatal_error;
     DriveInfo *dinfo;
     QemuOpts *opts;
 
@@ -37,7 +36,7 @@ DriveInfo *add_init_drive(const char *optstr)
     if (!opts)
         return NULL;
 
-    dinfo = drive_init(opts, current_machine->use_scsi, &fatal_error);
+    dinfo = drive_init(opts, current_machine->use_scsi);
     if (!dinfo) {
         qemu_opts_del(opts);
         return NULL;
diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index 11722c7..97d1e4a 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -542,7 +542,6 @@ static USBDevice *usb_msd_init(const char *filename)
     QemuOpts *opts;
     DriveInfo *dinfo;
     USBDevice *dev;
-    int fatal_error;
     const char *p1;
     char fmt[32];
 
@@ -572,7 +571,7 @@ static USBDevice *usb_msd_init(const char *filename)
     qemu_opt_set(opts, "if", "none");
 
     /* create host drive */
-    dinfo = drive_init(opts, 0, &fatal_error);
+    dinfo = drive_init(opts, 0);
     if (!dinfo) {
         qemu_opts_del(opts);
         return NULL;
diff --git a/vl.c b/vl.c
index 3637095..21d4929 100644
--- a/vl.c
+++ b/vl.c
@@ -631,13 +631,8 @@ static int bt_parse(const char *opt)
 static int drive_init_func(QemuOpts *opts, void *opaque)
 {
     int *use_scsi = opaque;
-    int fatal_error = 0;
 
-    if (drive_init(opts, *use_scsi, &fatal_error) == NULL) {
-        if (fatal_error)
-            return 1;
-    }
-    return 0;
+    return drive_init(opts, *use_scsi) == NULL;
 }
 
 static int drive_enable_snapshot(QemuOpts *opts, void *opaque)
@@ -666,7 +661,7 @@ static void default_drive(int enable, int snapshot, int use_scsi,
     if (snapshot) {
         drive_enable_snapshot(opts, NULL);
     }
-    if (drive_init_func(opts, &use_scsi)) {
+    if (!drive_init(opts, use_scsi)) {
         exit(1);
     }
 }
-- 
1.7.2.3

[Qemu-devel] [PATCH v3 07/11] blockdev: Replace drive_add()'s fmt, ... by optstr parameter

[Markus Armbruster]

Let the callers build the optstr.  Only one wants to.  All the others
become simpler, because they don't have to worry about escaping '%'.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
v2: Clean up shadowing of local variable buf[] in main().  Pointed out
by Kevin.

 blockdev.c |    8 +-------
 blockdev.h |    5 +----
 vl.c       |   23 +++++++++++++----------
 3 files changed, 15 insertions(+), 21 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 60dba1a..a5e56ee 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -81,17 +81,11 @@ QemuOpts *drive_def(const char *optstr)
 }
 
 QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
-                    const char *fmt, ...)
+                    const char *optstr)
 {
-    va_list ap;
-    char optstr[1024];
     QemuOpts *opts;
     char buf[32];
 
-    va_start(ap, fmt);
-    vsnprintf(optstr, sizeof(optstr), fmt, ap);
-    va_end(ap);
-
     opts = drive_def(optstr);
     if (!opts) {
         return NULL;
diff --git a/blockdev.h b/blockdev.h
index eb76573..3b9c5fd 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -46,10 +46,7 @@ DriveInfo *drive_get_by_blockdev(BlockDriverState *bs);
 
 QemuOpts *drive_def(const char *optstr);
 QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
-                    const char *fmt, ...) /*GCC_FMT_ATTR(4, 5)*/;
-    /* GCC_FMT_ATTR() commented out to avoid the (pretty useless)
-     * "zero-length gnu_printf format string" warning we insist to
-     * enable */
+                    const char *optstr);
 DriveInfo *drive_init(QemuOpts *arg, int default_to_scsi, int *fatal_error);
 
 /* device-hotplug */
diff --git a/vl.c b/vl.c
index 5399e33..979852b 100644
--- a/vl.c
+++ b/vl.c
@@ -621,7 +621,6 @@ static int bt_parse(const char *opt)
 /***********************************************************/
 /* QEMU Block devices */
 
-/* Any % in the following strings must be escaped as %% */
 #define HD_OPTS "media=disk"
 #define CDROM_OPTS "media=cdrom"
 #define FD_OPTS ""
@@ -2027,17 +2026,21 @@ int main(int argc, char **argv, char **envp)
                 initrd_filename = optarg;
                 break;
             case QEMU_OPTION_hda:
-                if (cyls == 0)
-                    hda_opts = drive_add(IF_DEFAULT, 0, optarg, HD_OPTS);
-                else
-                    hda_opts = drive_add(IF_DEFAULT, 0, optarg, HD_OPTS
-			     ",cyls=%d,heads=%d,secs=%d%s",
-                             cyls, heads, secs,
-                             translation == BIOS_ATA_TRANSLATION_LBA ?
+                {
+                    char buf[256];
+                    if (cyls == 0)
+                        snprintf(buf, sizeof(buf), "%s", HD_OPTS);
+                    else
+                        snprintf(buf, sizeof(buf),
+                                 "%s,cyls=%d,heads=%d,secs=%d%s",
+                                 HD_OPTS , cyls, heads, secs,
+                                 translation == BIOS_ATA_TRANSLATION_LBA ?
                                  ",trans=lba" :
-                             translation == BIOS_ATA_TRANSLATION_NONE ?
+                                 translation == BIOS_ATA_TRANSLATION_NONE ?
                                  ",trans=none" : "");
-                 break;
+                    drive_add(IF_DEFAULT, 0, optarg, buf);
+                    break;
+                }
             case QEMU_OPTION_hdb:
             case QEMU_OPTION_hdc:
             case QEMU_OPTION_hdd:
-- 
1.7.2.3

[Qemu-devel] Re: [PATCH v2 00/11] -drive/drive_add fixes and cleanups

[Kevin Wolf]

Am 28.01.2011 11:21, schrieb Markus Armbruster:
> Kevin found a bug in my recent "[PATCH 3+5/5] -drive/drive_add fixes".
> This is a rework of those two patches, plus a fix for the -drive
> if=scsi,index=N regression, plus the odd bonus fix found on the way.
> 
> v2: fix for the -drive if=scsi,index=N regression.  PATCH 4 fixed up
> slightly, PATCH 5 inserted, rest trivially rediffed.
> 
> Markus Armbruster (11):
>   scsi hotplug: Set DriveInfo member bus correctly
>   blockdev: New drive_get_next(), replacing qdev_init_bdrv()
>   blockdev: Move BlockInterfaceType from qemu-common.h to blockdev.h
>   blockdev: Put BlockInterfaceType names and max_devs in tables
>   blockdev: Fix regression in -drive if=scsi,index=N
>   blockdev: Make drive_add() take explicit type, index parameters
>   blockdev: Replace drive_add()'s fmt, ... by optstr parameter
>   blockdev: Factor drive_index_to_{bus,unit}_id out of drive_init()
>   blockdev: New drive_get_by_index()
>   blockdev: Reject multiple definitions for the same drive
>   blockdev: Fix drive_add for drives without media
> 
>  blockdev.c          |  143 +++++++++++++++++++++++++++++++--------------------
>  blockdev.h          |   18 +++++--
>  hw/device-hotplug.c |    5 +-
>  hw/ide.h            |    2 +
>  hw/ide/ahci.c       |    1 -
>  hw/pci-hotplug.c    |    1 +
>  hw/pl181.c          |    7 ++-
>  hw/qdev.c           |   15 -----
>  hw/qdev.h           |    2 -
>  hw/scsi.h           |    3 +-
>  hw/ssi-sd.c         |    7 ++-
>  hw/usb-msd.c        |    3 +-
>  qemu-common.h       |    6 --
>  savevm.c            |    1 -
>  vl.c                |   94 +++++++++++++++++++--------------
>  15 files changed, 171 insertions(+), 137 deletions(-)

Thanks, applied all to the block branch.

Kevin