* [Qemu-devel] [PATCH v2 01/11] scsi hotplug: Set DriveInfo member bus correctly
2011-01-28 10:21 [Qemu-devel] [PATCH v2 00/11] -drive/drive_add fixes and cleanups Markus Armbruster
@ 2011-01-28 10:21 ` Markus Armbruster
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 02/11] blockdev: New drive_get_next(), replacing qdev_init_bdrv() Markus Armbruster
` (10 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Markus Armbruster @ 2011-01-28 10:21 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, hare
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [PATCH v2 02/11] blockdev: New drive_get_next(), replacing qdev_init_bdrv()
2011-01-28 10:21 [Qemu-devel] [PATCH v2 00/11] -drive/drive_add fixes and cleanups Markus Armbruster
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 01/11] scsi hotplug: Set DriveInfo member bus correctly Markus Armbruster
@ 2011-01-28 10:21 ` Markus Armbruster
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 03/11] blockdev: Move BlockInterfaceType from qemu-common.h to blockdev.h Markus Armbruster
` (9 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Markus Armbruster @ 2011-01-28 10:21 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, hare
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [PATCH v2 03/11] blockdev: Move BlockInterfaceType from qemu-common.h to blockdev.h
2011-01-28 10:21 [Qemu-devel] [PATCH v2 00/11] -drive/drive_add fixes and cleanups Markus Armbruster
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 01/11] scsi hotplug: Set DriveInfo member bus correctly Markus Armbruster
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 02/11] blockdev: New drive_get_next(), replacing qdev_init_bdrv() Markus Armbruster
@ 2011-01-28 10:21 ` Markus Armbruster
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 04/11] blockdev: Put BlockInterfaceType names and max_devs in tables Markus Armbruster
` (8 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Markus Armbruster @ 2011-01-28 10:21 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, hare
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [PATCH v2 04/11] blockdev: Put BlockInterfaceType names and max_devs in tables
2011-01-28 10:21 [Qemu-devel] [PATCH v2 00/11] -drive/drive_add fixes and cleanups Markus Armbruster
` (2 preceding siblings ...)
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 03/11] blockdev: Move BlockInterfaceType from qemu-common.h to blockdev.h Markus Armbruster
@ 2011-01-28 10:21 ` Markus Armbruster
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 05/11] blockdev: Fix regression in -drive if=scsi, index=N Markus Armbruster
` (7 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Markus Armbruster @ 2011-01-28 10:21 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, hare
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [PATCH v2 05/11] blockdev: Fix regression in -drive if=scsi, index=N
2011-01-28 10:21 [Qemu-devel] [PATCH v2 00/11] -drive/drive_add fixes and cleanups Markus Armbruster
` (3 preceding siblings ...)
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 04/11] blockdev: Put BlockInterfaceType names and max_devs in tables Markus Armbruster
@ 2011-01-28 10:21 ` Markus Armbruster
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 06/11] blockdev: Make drive_add() take explicit type, index parameters Markus Armbruster
` (6 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Markus Armbruster @ 2011-01-28 10:21 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, hare
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [PATCH v2 06/11] blockdev: Make drive_add() take explicit type, index parameters
2011-01-28 10:21 [Qemu-devel] [PATCH v2 00/11] -drive/drive_add fixes and cleanups Markus Armbruster
` (4 preceding siblings ...)
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 05/11] blockdev: Fix regression in -drive if=scsi, index=N Markus Armbruster
@ 2011-01-28 10:21 ` Markus Armbruster
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 07/11] blockdev: Replace drive_add()'s fmt, ... by optstr parameter Markus Armbruster
` (5 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Markus Armbruster @ 2011-01-28 10:21 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, hare
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [PATCH v2 07/11] blockdev: Replace drive_add()'s fmt, ... by optstr parameter
2011-01-28 10:21 [Qemu-devel] [PATCH v2 00/11] -drive/drive_add fixes and cleanups Markus Armbruster
` (5 preceding siblings ...)
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 06/11] blockdev: Make drive_add() take explicit type, index parameters Markus Armbruster
@ 2011-01-28 10:21 ` Markus Armbruster
2011-01-31 10:50 ` [Qemu-devel] [PATCH v3 " Markus Armbruster
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 08/11] blockdev: Factor drive_index_to_{bus, unit}_id out of drive_init() Markus Armbruster
` (4 subsequent siblings)
11 siblings, 1 reply; 14+ messages in thread
From: Markus Armbruster @ 2011-01-28 10:21 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, hare
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [PATCH v3 07/11] blockdev: Replace drive_add()'s fmt, ... by optstr parameter
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 07/11] blockdev: Replace drive_add()'s fmt, ... by optstr parameter Markus Armbruster
@ 2011-01-31 10:50 ` Markus Armbruster
0 siblings, 0 replies; 14+ messages in thread
From: Markus Armbruster @ 2011-01-31 10:50 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, hare
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v2 08/11] blockdev: Factor drive_index_to_{bus, unit}_id out of drive_init()
2011-01-28 10:21 [Qemu-devel] [PATCH v2 00/11] -drive/drive_add fixes and cleanups Markus Armbruster
` (6 preceding siblings ...)
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 07/11] blockdev: Replace drive_add()'s fmt, ... by optstr parameter Markus Armbruster
@ 2011-01-28 10:21 ` Markus Armbruster
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 09/11] blockdev: New drive_get_by_index() Markus Armbruster
` (3 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Markus Armbruster @ 2011-01-28 10:21 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, hare
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [PATCH v2 09/11] blockdev: New drive_get_by_index()
2011-01-28 10:21 [Qemu-devel] [PATCH v2 00/11] -drive/drive_add fixes and cleanups Markus Armbruster
` (7 preceding siblings ...)
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 08/11] blockdev: Factor drive_index_to_{bus, unit}_id out of drive_init() Markus Armbruster
@ 2011-01-28 10:21 ` Markus Armbruster
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 10/11] blockdev: Reject multiple definitions for the same drive Markus Armbruster
` (2 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Markus Armbruster @ 2011-01-28 10:21 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, hare
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [PATCH v2 10/11] blockdev: Reject multiple definitions for the same drive
2011-01-28 10:21 [Qemu-devel] [PATCH v2 00/11] -drive/drive_add fixes and cleanups Markus Armbruster
` (8 preceding siblings ...)
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 09/11] blockdev: New drive_get_by_index() Markus Armbruster
@ 2011-01-28 10:21 ` Markus Armbruster
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 11/11] blockdev: Fix drive_add for drives without media Markus Armbruster
2011-01-31 11:00 ` [Qemu-devel] Re: [PATCH v2 00/11] -drive/drive_add fixes and cleanups Kevin Wolf
11 siblings, 0 replies; 14+ messages in thread
From: Markus Armbruster @ 2011-01-28 10:21 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, hare
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] [PATCH v2 11/11] blockdev: Fix drive_add for drives without media
2011-01-28 10:21 [Qemu-devel] [PATCH v2 00/11] -drive/drive_add fixes and cleanups Markus Armbruster
` (9 preceding siblings ...)
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 10/11] blockdev: Reject multiple definitions for the same drive Markus Armbruster
@ 2011-01-28 10:21 ` Markus Armbruster
2011-01-31 11:00 ` [Qemu-devel] Re: [PATCH v2 00/11] -drive/drive_add fixes and cleanups Kevin Wolf
11 siblings, 0 replies; 14+ messages in thread
From: Markus Armbruster @ 2011-01-28 10:21 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, hare
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [Qemu-devel] Re: [PATCH v2 00/11] -drive/drive_add fixes and cleanups
2011-01-28 10:21 [Qemu-devel] [PATCH v2 00/11] -drive/drive_add fixes and cleanups Markus Armbruster
` (10 preceding siblings ...)
2011-01-28 10:21 ` [Qemu-devel] [PATCH v2 11/11] blockdev: Fix drive_add for drives without media Markus Armbruster
@ 2011-01-31 11:00 ` Kevin Wolf
11 siblings, 0 replies; 14+ messages in thread
From: Kevin Wolf @ 2011-01-31 11:00 UTC (permalink / raw)
To: Markus Armbruster; +Cc: stefanha, qemu-devel, hare
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
^ permalink raw reply [flat|nested] 14+ messages in thread