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

[Qemu-devel] [PATCH 00/10] -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 the odd bonus fix found on
the way.

Markus Armbruster (10):
  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: 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          |  127 +++++++++++++++++++++++++++++---------------------
 blockdev.h          |   15 +++++-
 hw/device-hotplug.c |    5 +-
 hw/pci-hotplug.c    |    1 +
 hw/pl181.c          |    7 ++-
 hw/qdev.c           |   14 ------
 hw/qdev.h           |    2 -
 hw/ssi-sd.c         |    7 ++-
 hw/usb-msd.c        |    3 +-
 qemu-common.h       |    6 --
 vl.c                |   94 ++++++++++++++++++++++----------------
 11 files changed, 153 insertions(+), 128 deletions(-)

-- 
1.7.2.3

[Qemu-devel] [PATCH 01/10] 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 02/10] 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 03/10] 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 04/10] 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 |   49 +++++++++++++++++++++----------------------------
 1 files changed, 21 insertions(+), 28 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 883117a..e8dd353 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.
@@ -199,37 +216,13 @@ 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) {
-- 
1.7.2.3

[Qemu-devel] [PATCH 05/10] 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 e8dd353..2c4246f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -61,20 +61,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;
@@ -461,7 +475,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 f616855..cd64a70 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
@@ -46,7 +47,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 06/10] 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 2c4246f..e67f93c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -67,17 +67,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 cd64a70..e29fa5e 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -49,10 +49,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 07/10] 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 e67f93c..4d8bb3d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -61,6 +61,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);
@@ -364,14 +376,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 08/10] 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 4d8bb3d..08c9d7e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -116,6 +116,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 e29fa5e..2d518c5 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -42,6 +42,7 @@ struct DriveInfo {
 #define MAX_SCSI_DEVS	255
 
 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 09/10] 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 08c9d7e..a2fd4d5 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -411,11 +411,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