From 19dbcbf7cc1892f5130b4edd5a4bb4ca800ef7d8 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 22 Jun 2010 16:59:46 +0200 Subject: [PATCH 01/23] qcow2: Fix error handling during metadata preallocation People were wondering why qemu-img check failed after they tried to preallocate a large qcow2 file and ran out of disk space. Signed-off-by: Kevin Wolf --- block/qcow2.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index d29e6b6c05..9ee34b6dd0 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -805,14 +805,14 @@ static int preallocate(BlockDriverState *bs) while (nb_sectors) { num = MIN(nb_sectors, INT_MAX >> 9); ret = qcow2_alloc_cluster_offset(bs, offset, 0, num, &num, &meta); - if (ret < 0) { - return -1; + return ret; } - if (qcow2_alloc_cluster_link_l2(bs, &meta) < 0) { + ret = qcow2_alloc_cluster_link_l2(bs, &meta); + if (ret < 0) { qcow2_free_any_clusters(bs, meta.cluster_offset, meta.nb_clusters); - return -1; + return ret; } /* There are no dependent requests, but we need to remove our request @@ -833,7 +833,10 @@ static int preallocate(BlockDriverState *bs) if (meta.cluster_offset != 0) { uint8_t buf[512]; memset(buf, 0, 512); - bdrv_write(bs->file, (meta.cluster_offset >> 9) + num - 1, buf, 1); + ret = bdrv_write(bs->file, (meta.cluster_offset >> 9) + num - 1, buf, 1); + if (ret < 0) { + return ret; + } } return 0; @@ -1030,7 +1033,7 @@ exit: BlockDriver *drv = bdrv_find_format("qcow2"); bs = bdrv_new(""); bdrv_open(bs, filename, BDRV_O_CACHE_WB | BDRV_O_RDWR, drv); - preallocate(bs); + ret = preallocate(bs); bdrv_close(bs); } From 39508e7adb0de3ef69caa1b494d823d8ac11d3f3 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Wed, 23 Jun 2010 12:25:17 +0200 Subject: [PATCH 02/23] block: allow filenames with colons again for host devices Before the raw/file split we used to allow filenames with colons for host device only. While this was more by accident than by design people rely on it, so we need to bring it back. So move the host device probing to be before the protocol detection again. Signed-off-by: Christoph Hellwig Signed-off-by: Kevin Wolf --- block.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/block.c b/block.c index e71a771e06..0aaec3bf35 100644 --- a/block.c +++ b/block.c @@ -288,24 +288,31 @@ BlockDriver *bdrv_find_protocol(const char *filename) char protocol[128]; int len; const char *p; - int is_drive; /* TODO Drivers without bdrv_file_open must be specified explicitly */ -#ifdef _WIN32 - is_drive = is_windows_drive(filename) || - is_windows_drive_prefix(filename); -#else - is_drive = 0; -#endif - p = strchr(filename, ':'); - if (!p || is_drive) { - drv1 = find_hdev_driver(filename); - if (!drv1) { - drv1 = bdrv_find_format("file"); - } + /* + * XXX(hch): we really should not let host device detection + * override an explicit protocol specification, but moving this + * later breaks access to device names with colons in them. + * Thanks to the brain-dead persistent naming schemes on udev- + * based Linux systems those actually are quite common. + */ + drv1 = find_hdev_driver(filename); + if (drv1) { return drv1; } + +#ifdef _WIN32 + if (is_windows_drive(filename) || + is_windows_drive_prefix(filename)) + return bdrv_find_format("file"); +#endif + + p = strchr(filename, ':'); + if (!p) { + return bdrv_find_format("file"); + } len = p - filename; if (len > sizeof(protocol) - 1) len = sizeof(protocol) - 1; From fa66b909f382619da15f8c7e323145adfa94fdac Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 25 Jun 2010 18:53:21 +0200 Subject: [PATCH 03/23] scsi: scsi_bus_legacy_handle_cmdline() can fail, fix callers None of its callers checks for failure. scsi_hot_add() can crash because of that: (qemu) drive_add 4 if=scsi,format=host_device,file=/dev/sg1 scsi-generic: scsi generic interface too old Segmentation fault (core dumped) Fix all callers, not just scsi_hot_add(). Signed-off-by: Markus Armbruster Signed-off-by: Kevin Wolf --- hw/esp.c | 3 +-- hw/lsi53c895a.c | 2 +- hw/pci-hotplug.c | 3 +++ hw/scsi-bus.c | 11 +++++++---- hw/scsi.h | 2 +- hw/usb-msd.c | 3 +++ 6 files changed, 16 insertions(+), 8 deletions(-) diff --git a/hw/esp.c b/hw/esp.c index 7740879102..349052a024 100644 --- a/hw/esp.c +++ b/hw/esp.c @@ -679,8 +679,7 @@ static int esp_init1(SysBusDevice *dev) qdev_init_gpio_in(&dev->qdev, parent_esp_reset, 1); scsi_bus_new(&s->bus, &dev->qdev, 0, ESP_MAX_DEVS, esp_command_complete); - scsi_bus_legacy_handle_cmdline(&s->bus); - return 0; + return scsi_bus_legacy_handle_cmdline(&s->bus); } static SysBusDeviceInfo esp_info = { diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c index 9a37fed3b3..1bb1caf478 100644 --- a/hw/lsi53c895a.c +++ b/hw/lsi53c895a.c @@ -2176,7 +2176,7 @@ static int lsi_scsi_init(PCIDevice *dev) scsi_bus_new(&s->bus, &dev->qdev, 1, LSI_MAX_DEVS, lsi_command_complete); if (!dev->qdev.hotplugged) { - scsi_bus_legacy_handle_cmdline(&s->bus); + return scsi_bus_legacy_handle_cmdline(&s->bus); } return 0; } diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c index c39e640089..55c9fe3a79 100644 --- a/hw/pci-hotplug.c +++ b/hw/pci-hotplug.c @@ -90,6 +90,9 @@ static int scsi_hot_add(Monitor *mon, DeviceState *adapter, */ dinfo->unit = qemu_opt_get_number(dinfo->opts, "unit", -1); scsidev = scsi_bus_legacy_add_drive(scsibus, dinfo, dinfo->unit); + if (!scsidev) { + return -1; + } dinfo->unit = scsidev->id; if (printinfo) diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c index 24bd0602ef..d5b66c14dd 100644 --- a/hw/scsi-bus.c +++ b/hw/scsi-bus.c @@ -83,7 +83,6 @@ void scsi_qdev_register(SCSIDeviceInfo *info) } /* handle legacy '-drive if=scsi,...' cmd line args */ -/* FIXME callers should check for failure, but don't */ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, DriveInfo *dinfo, int unit) { const char *driver; @@ -98,18 +97,22 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, DriveInfo *dinfo, int unit) return DO_UPCAST(SCSIDevice, qdev, dev); } -void scsi_bus_legacy_handle_cmdline(SCSIBus *bus) +int scsi_bus_legacy_handle_cmdline(SCSIBus *bus) { DriveInfo *dinfo; - int unit; + int res = 0, unit; for (unit = 0; unit < MAX_SCSI_DEVS; unit++) { dinfo = drive_get(IF_SCSI, bus->busnr, unit); if (dinfo == NULL) { continue; } - scsi_bus_legacy_add_drive(bus, dinfo, unit); + if (!scsi_bus_legacy_add_drive(bus, dinfo, unit)) { + res = -1; + break; + } } + return res; } void scsi_dev_clear_sense(SCSIDevice *dev) diff --git a/hw/scsi.h b/hw/scsi.h index b668e277ae..b1b5f730d5 100644 --- a/hw/scsi.h +++ b/hw/scsi.h @@ -98,7 +98,7 @@ static inline SCSIBus *scsi_bus_from_device(SCSIDevice *d) } SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, DriveInfo *dinfo, int unit); -void scsi_bus_legacy_handle_cmdline(SCSIBus *bus); +int scsi_bus_legacy_handle_cmdline(SCSIBus *bus); void scsi_dev_clear_sense(SCSIDevice *dev); void scsi_dev_set_sense(SCSIDevice *dev, uint8_t key); diff --git a/hw/usb-msd.c b/hw/usb-msd.c index 003bd8a4cd..8e9718c562 100644 --- a/hw/usb-msd.c +++ b/hw/usb-msd.c @@ -531,6 +531,9 @@ static int usb_msd_initfn(USBDevice *dev) s->dev.speed = USB_SPEED_FULL; scsi_bus_new(&s->bus, &s->dev.qdev, 0, 1, usb_msd_command_complete); s->scsi_dev = scsi_bus_legacy_add_drive(&s->bus, s->conf.dinfo, 0); + if (!s->scsi_dev) { + return -1; + } s->bus.qbus.allow_hotplug = 0; usb_msd_handle_reset(dev); From fa12fbbecefc6f9bf20374daa275a598096d113c Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 25 Jun 2010 18:53:22 +0200 Subject: [PATCH 04/23] ide: Make it explicit that ide_create_drive() can't fail All callers of ide_create_drive() ignore its value. Currently harmless, because it fails only when qdev_init() fails, which fails only when ide_drive_initfn() fails, which never fails. Brittle. Change it to die instead of silently ignoring failure. Signed-off-by: Markus Armbruster Signed-off-by: Kevin Wolf --- hw/ide/qdev.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c index 0f9f22e30f..127478ba65 100644 --- a/hw/ide/qdev.c +++ b/hw/ide/qdev.c @@ -84,8 +84,7 @@ IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive) dev = qdev_create(&bus->qbus, "ide-drive"); qdev_prop_set_uint32(dev, "unit", unit); qdev_prop_set_drive(dev, "drive", drive); - if (qdev_init(dev) < 0) - return NULL; + qdev_init_nofail(dev); return DO_UPCAST(IDEDevice, qdev, dev); } From 0c6f9c17f301f713b88daba6baadf9d7474ece72 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 25 Jun 2010 18:53:23 +0200 Subject: [PATCH 05/23] blockdev: Remove drive_get_serial() Unused since commit 6ced55a5. Signed-off-by: Markus Armbruster Signed-off-by: Kevin Wolf --- blockdev.c | 12 ------------ blockdev.h | 1 - 2 files changed, 13 deletions(-) diff --git a/blockdev.c b/blockdev.c index 3b8c6067c7..e0495e5c61 100644 --- a/blockdev.c +++ b/blockdev.c @@ -78,18 +78,6 @@ int drive_get_max_bus(BlockInterfaceType type) return max_bus; } -const char *drive_get_serial(BlockDriverState *bdrv) -{ - DriveInfo *dinfo; - - QTAILQ_FOREACH(dinfo, &drives, next) { - if (dinfo->bdrv == bdrv) - return dinfo->serial; - } - - return "\0"; -} - static void bdrv_format_print(void *opaque, const char *name) { fprintf(stderr, " %s", name); diff --git a/blockdev.h b/blockdev.h index 23ea5764a7..a9367857d8 100644 --- a/blockdev.h +++ b/blockdev.h @@ -40,7 +40,6 @@ extern DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit); extern DriveInfo *drive_get_by_id(const char *id); extern int drive_get_max_bus(BlockInterfaceType type); extern void drive_uninit(DriveInfo *dinfo); -extern const char *drive_get_serial(BlockDriverState *bdrv); extern QemuOpts *drive_add(const char *file, const char *fmt, ...); extern DriveInfo *drive_init(QemuOpts *arg, int default_to_scsi, From 15c7733bb231090e5ebd6d10060dccdb98bb4941 Mon Sep 17 00:00:00 2001 From: Ryan Harper Date: Mon, 28 Jun 2010 09:38:33 -0500 Subject: [PATCH 06/23] Don't reset bs->is_temporary in bdrv_open_common To fix https://bugs.launchpad.net/qemu/+bug/597402 where qemu fails to call unlink() on temporary snapshots due to bs->is_temporary getting clobbered in bdrv_open_common() after being set in bdrv_open() which calls the former. We don't need to initialize bs->is_temporary in bdrv_open_common(). Signed-off-by: Ryan Harper Signed-off-by: Kevin Wolf --- block.c | 1 - 1 file changed, 1 deletion(-) diff --git a/block.c b/block.c index 0aaec3bf35..31ca4c5a43 100644 --- a/block.c +++ b/block.c @@ -400,7 +400,6 @@ static int bdrv_open_common(BlockDriverState *bs, const char *filename, bs->file = NULL; bs->total_sectors = 0; - bs->is_temporary = 0; bs->encrypted = 0; bs->valid_key = 0; bs->open_flags = flags; From e4700e595ea0b24d5291dbd68deba26d7a955703 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 24 Jun 2010 17:25:32 +0200 Subject: [PATCH 07/23] blockdev: New drive_get_by_blockdev() Signed-off-by: Markus Armbruster Signed-off-by: Kevin Wolf --- blockdev.c | 12 ++++++++++++ blockdev.h | 1 + 2 files changed, 13 insertions(+) diff --git a/blockdev.c b/blockdev.c index e0495e5c61..ba4f66f976 100644 --- a/blockdev.c +++ b/blockdev.c @@ -78,6 +78,18 @@ int drive_get_max_bus(BlockInterfaceType type) return max_bus; } +DriveInfo *drive_get_by_blockdev(BlockDriverState *bs) +{ + DriveInfo *dinfo; + + QTAILQ_FOREACH(dinfo, &drives, next) { + if (dinfo->bdrv == bs) { + return dinfo; + } + } + return NULL; +} + static void bdrv_format_print(void *opaque, const char *name) { fprintf(stderr, " %s", name); diff --git a/blockdev.h b/blockdev.h index a9367857d8..6ab592f0fd 100644 --- a/blockdev.h +++ b/blockdev.h @@ -40,6 +40,7 @@ extern DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit); extern DriveInfo *drive_get_by_id(const char *id); extern int drive_get_max_bus(BlockInterfaceType type); extern void drive_uninit(DriveInfo *dinfo); +extern DriveInfo *drive_get_by_blockdev(BlockDriverState *bs); extern QemuOpts *drive_add(const char *file, const char *fmt, ...); extern DriveInfo *drive_init(QemuOpts *arg, int default_to_scsi, From 14bafc540774baf316e9ce2474e97d5df6cb8e6c Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 25 Jun 2010 08:09:10 +0200 Subject: [PATCH 08/23] blockdev: Clean up automatic drive deletion We automatically delete blockdev host parts on unplug of the guest device. Too much magic, but we can't change that now. The delete happens early in the guest device teardown, before the connection to the host part is severed. Thus, the guest part's pointer to the host part dangles for a brief time. No actual harm comes from this, but we'll catch such dangling pointers a few commits down the road. Clean up the dangling pointers by delaying the automatic deletion until the guest part's pointer is gone. Device usb-storage deliberately makes two qdev properties refer to the same drive, because it automatically creates a second device. Again, too much magic we can't change now. Multiple references worked okay before, but now free_drive() dies for the second one. Zap the extra reference. Signed-off-by: Markus Armbruster Signed-off-by: Kevin Wolf --- blockdev.c | 23 +++++++++++++++++++++++ blockdev.h | 4 ++++ hw/qdev-properties.c | 10 ++++++++++ hw/scsi-disk.c | 2 +- hw/scsi-generic.c | 2 +- hw/usb-msd.c | 20 ++++++++++++++++---- hw/virtio-pci.c | 2 +- 7 files changed, 56 insertions(+), 7 deletions(-) diff --git a/blockdev.c b/blockdev.c index ba4f66f976..48481120c2 100644 --- a/blockdev.c +++ b/blockdev.c @@ -17,6 +17,29 @@ static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives); +/* + * We automatically delete the drive when a device using it gets + * unplugged. Questionable feature, but we can't just drop it. + * Device models call blockdev_mark_auto_del() to schedule the + * automatic deletion, and generic qdev code calls blockdev_auto_del() + * when deletion is actually safe. + */ +void blockdev_mark_auto_del(BlockDriverState *bs) +{ + DriveInfo *dinfo = drive_get_by_blockdev(bs); + + dinfo->auto_del = 1; +} + +void blockdev_auto_del(BlockDriverState *bs) +{ + DriveInfo *dinfo = drive_get_by_blockdev(bs); + + if (dinfo->auto_del) { + drive_uninit(dinfo); + } +} + QemuOpts *drive_add(const char *file, const char *fmt, ...) { va_list ap; diff --git a/blockdev.h b/blockdev.h index 6ab592f0fd..32e6979456 100644 --- a/blockdev.h +++ b/blockdev.h @@ -13,6 +13,9 @@ #include "block.h" #include "qemu-queue.h" +void blockdev_mark_auto_del(BlockDriverState *bs); +void blockdev_auto_del(BlockDriverState *bs); + typedef enum { IF_NONE, IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN, @@ -28,6 +31,7 @@ typedef struct DriveInfo { BlockInterfaceType type; int bus; int unit; + int auto_del; /* see blockdev_mark_auto_del() */ QemuOpts *opts; char serial[BLOCK_SERIAL_STRLEN + 1]; QTAILQ_ENTRY(DriveInfo) next; diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c index 5b7fd77d44..d7dc234e25 100644 --- a/hw/qdev-properties.c +++ b/hw/qdev-properties.c @@ -313,6 +313,15 @@ static int parse_drive(DeviceState *dev, Property *prop, const char *str) return 0; } +static void free_drive(DeviceState *dev, Property *prop) +{ + DriveInfo **ptr = qdev_get_prop_ptr(dev, prop); + + if (*ptr) { + blockdev_auto_del((*ptr)->bdrv); + } +} + static int print_drive(DeviceState *dev, Property *prop, char *dest, size_t len) { DriveInfo **ptr = qdev_get_prop_ptr(dev, prop); @@ -325,6 +334,7 @@ PropertyInfo qdev_prop_drive = { .size = sizeof(DriveInfo*), .parse = parse_drive, .print = print_drive, + .free = free_drive, }; /* --- character device --- */ diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index 2b3898435c..d76e640412 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -1043,7 +1043,7 @@ static void scsi_destroy(SCSIDevice *dev) SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev); scsi_disk_purge_requests(s); - drive_uninit(s->qdev.conf.dinfo); + blockdev_mark_auto_del(s->qdev.conf.dinfo->bdrv); } static int scsi_disk_initfn(SCSIDevice *dev) diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c index e31060e944..1859c94b7c 100644 --- a/hw/scsi-generic.c +++ b/hw/scsi-generic.c @@ -453,7 +453,7 @@ static void scsi_destroy(SCSIDevice *d) r = DO_UPCAST(SCSIGenericReq, req, QTAILQ_FIRST(&s->qdev.requests)); scsi_remove_request(r); } - drive_uninit(s->qdev.conf.dinfo); + blockdev_mark_auto_del(s->qdev.conf.dinfo->bdrv); } static int scsi_generic_initfn(SCSIDevice *dev) diff --git a/hw/usb-msd.c b/hw/usb-msd.c index 8e9718c562..3dbfcabeb1 100644 --- a/hw/usb-msd.c +++ b/hw/usb-msd.c @@ -522,24 +522,36 @@ static void usb_msd_password_cb(void *opaque, int err) static int usb_msd_initfn(USBDevice *dev) { MSDState *s = DO_UPCAST(MSDState, dev, dev); + DriveInfo *dinfo = s->conf.dinfo; - if (!s->conf.dinfo || !s->conf.dinfo->bdrv) { + if (!dinfo || !dinfo->bdrv) { error_report("usb-msd: drive property not set"); return -1; } + /* + * Hack alert: this pretends to be a block device, but it's really + * a SCSI bus that can serve only a single device, which it + * creates automatically. Two drive properties pointing to the + * same drive is not good: free_drive() dies for the second one. + * Zap the one we're not going to use. + * + * The hack is probably a bad idea. + */ + s->conf.dinfo = NULL; + s->dev.speed = USB_SPEED_FULL; scsi_bus_new(&s->bus, &s->dev.qdev, 0, 1, usb_msd_command_complete); - s->scsi_dev = scsi_bus_legacy_add_drive(&s->bus, s->conf.dinfo, 0); + s->scsi_dev = scsi_bus_legacy_add_drive(&s->bus, dinfo, 0); if (!s->scsi_dev) { return -1; } s->bus.qbus.allow_hotplug = 0; usb_msd_handle_reset(dev); - if (bdrv_key_required(s->conf.dinfo->bdrv)) { + if (bdrv_key_required(dinfo->bdrv)) { if (cur_mon) { - monitor_read_bdrv_key_start(cur_mon, s->conf.dinfo->bdrv, + monitor_read_bdrv_key_start(cur_mon, dinfo->bdrv, usb_msd_password_cb, s); s->dev.auto_attach = 0; } else { diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index d1303b1a38..31a68fec40 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -571,7 +571,7 @@ static int virtio_blk_exit_pci(PCIDevice *pci_dev) { VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); - drive_uninit(proxy->block.dinfo); + blockdev_mark_auto_del(proxy->block.dinfo->bdrv); return virtio_exit_pci(pci_dev); } From f8b6cc0070aab8b75bd082582c829be1353f395f Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 5 May 2010 16:36:52 +0200 Subject: [PATCH 09/23] qdev: Decouple qdev_prop_drive from DriveInfo Make the property point to BlockDriverState, cutting out the DriveInfo middleman. This prepares the ground for block devices that don't have a DriveInfo. Currently all user-defined ones have a DriveInfo, because the only way to define one is -drive & friends (they go through drive_init()). DriveInfo is closely tied to -drive, and like -drive, it mixes information about host and guest part of the block device. I'm working towards a new way to define block devices, with clean host/guest separation, and I need to get DriveInfo out of the way for that. Fortunately, the device models are perfectly happy with BlockDriverState, except for two places: ide_drive_initfn() and scsi_disk_initfn() need to check the DriveInfo for a serial number set with legacy -drive serial=... Use drive_get_by_blockdev() there. Device model code should now use DriveInfo only when explicitly dealing with drives defined the old way, i.e. without -device. Signed-off-by: Markus Armbruster Reviewed-by: Christoph Hellwig Signed-off-by: Kevin Wolf --- block_int.h | 6 ++---- hw/fdc.c | 22 ++++++++++------------ hw/ide/core.c | 17 +++++++++-------- hw/ide/internal.h | 2 +- hw/ide/qdev.c | 12 ++++++++---- hw/pci-hotplug.c | 4 ++-- hw/qdev-properties.c | 21 ++++++++++++--------- hw/qdev.h | 6 +++--- hw/s390-virtio.c | 2 +- hw/scsi-bus.c | 8 ++++---- hw/scsi-disk.c | 16 +++++++--------- hw/scsi-generic.c | 6 +++--- hw/scsi.h | 2 +- hw/usb-msd.c | 15 +++++++-------- hw/virtio-blk.c | 2 +- hw/virtio-pci.c | 4 ++-- 16 files changed, 73 insertions(+), 72 deletions(-) diff --git a/block_int.h b/block_int.h index b64a0095f5..e60aed4a04 100644 --- a/block_int.h +++ b/block_int.h @@ -210,10 +210,8 @@ void *qemu_blockalign(BlockDriverState *bs, size_t size); int is_windows_drive(const char *filename); #endif -struct DriveInfo; - typedef struct BlockConf { - struct DriveInfo *dinfo; + BlockDriverState *bs; uint16_t physical_block_size; uint16_t logical_block_size; uint16_t min_io_size; @@ -234,7 +232,7 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf) } #define DEFINE_BLOCK_PROPERTIES(_state, _conf) \ - DEFINE_PROP_DRIVE("drive", _state, _conf.dinfo), \ + DEFINE_PROP_DRIVE("drive", _state, _conf.bs), \ DEFINE_PROP_UINT16("logical_block_size", _state, \ _conf.logical_block_size, 512), \ DEFINE_PROP_UINT16("physical_block_size", _state, \ diff --git a/hw/fdc.c b/hw/fdc.c index 45a876d2c1..08712bc7d2 100644 --- a/hw/fdc.c +++ b/hw/fdc.c @@ -80,7 +80,6 @@ typedef enum FDiskFlags { } FDiskFlags; typedef struct FDrive { - DriveInfo *dinfo; BlockDriverState *bs; /* Drive status */ FDriveType drive; @@ -100,7 +99,6 @@ typedef struct FDrive { static void fd_init(FDrive *drv) { /* Drive */ - drv->bs = drv->dinfo ? drv->dinfo->bdrv : NULL; drv->drive = FDRIVE_DRV_NONE; drv->perpendicular = 0; /* Disk */ @@ -1862,10 +1860,10 @@ FDCtrl *fdctrl_init_isa(DriveInfo **fds) dev = isa_create("isa-fdc"); if (fds[0]) { - qdev_prop_set_drive(&dev->qdev, "driveA", fds[0]); + qdev_prop_set_drive(&dev->qdev, "driveA", fds[0]->bdrv); } if (fds[1]) { - qdev_prop_set_drive(&dev->qdev, "driveB", fds[1]); + qdev_prop_set_drive(&dev->qdev, "driveB", fds[1]->bdrv); } if (qdev_init(&dev->qdev) < 0) return NULL; @@ -1884,10 +1882,10 @@ FDCtrl *fdctrl_init_sysbus(qemu_irq irq, int dma_chann, fdctrl = &sys->state; fdctrl->dma_chann = dma_chann; /* FIXME */ if (fds[0]) { - qdev_prop_set_drive(dev, "driveA", fds[0]); + qdev_prop_set_drive(dev, "driveA", fds[0]->bdrv); } if (fds[1]) { - qdev_prop_set_drive(dev, "driveB", fds[1]); + qdev_prop_set_drive(dev, "driveB", fds[1]->bdrv); } qdev_init_nofail(dev); sysbus_connect_irq(&sys->busdev, 0, irq); @@ -1905,7 +1903,7 @@ FDCtrl *sun4m_fdctrl_init(qemu_irq irq, target_phys_addr_t io_base, dev = qdev_create(NULL, "SUNW,fdtwo"); if (fds[0]) { - qdev_prop_set_drive(dev, "drive", fds[0]); + qdev_prop_set_drive(dev, "drive", fds[0]->bdrv); } qdev_init_nofail(dev); sys = DO_UPCAST(FDCtrlSysBus, busdev.qdev, dev); @@ -2030,8 +2028,8 @@ static ISADeviceInfo isa_fdc_info = { .qdev.vmsd = &vmstate_isa_fdc, .qdev.reset = fdctrl_external_reset_isa, .qdev.props = (Property[]) { - DEFINE_PROP_DRIVE("driveA", FDCtrlISABus, state.drives[0].dinfo), - DEFINE_PROP_DRIVE("driveB", FDCtrlISABus, state.drives[1].dinfo), + DEFINE_PROP_DRIVE("driveA", FDCtrlISABus, state.drives[0].bs), + DEFINE_PROP_DRIVE("driveB", FDCtrlISABus, state.drives[1].bs), DEFINE_PROP_END_OF_LIST(), }, }; @@ -2053,8 +2051,8 @@ static SysBusDeviceInfo sysbus_fdc_info = { .qdev.vmsd = &vmstate_sysbus_fdc, .qdev.reset = fdctrl_external_reset_sysbus, .qdev.props = (Property[]) { - DEFINE_PROP_DRIVE("driveA", FDCtrlSysBus, state.drives[0].dinfo), - DEFINE_PROP_DRIVE("driveB", FDCtrlSysBus, state.drives[1].dinfo), + DEFINE_PROP_DRIVE("driveA", FDCtrlSysBus, state.drives[0].bs), + DEFINE_PROP_DRIVE("driveB", FDCtrlSysBus, state.drives[1].bs), DEFINE_PROP_END_OF_LIST(), }, }; @@ -2066,7 +2064,7 @@ static SysBusDeviceInfo sun4m_fdc_info = { .qdev.vmsd = &vmstate_sysbus_fdc, .qdev.reset = fdctrl_external_reset_sysbus, .qdev.props = (Property[]) { - DEFINE_PROP_DRIVE("drive", FDCtrlSysBus, state.drives[0].dinfo), + DEFINE_PROP_DRIVE("drive", FDCtrlSysBus, state.drives[0].bs), DEFINE_PROP_END_OF_LIST(), }, }; diff --git a/hw/ide/core.c b/hw/ide/core.c index 0b3b7c2e69..cc4591baa0 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -2594,15 +2594,15 @@ void ide_bus_reset(IDEBus *bus) ide_clear_hob(bus); } -void ide_init_drive(IDEState *s, DriveInfo *dinfo, +void ide_init_drive(IDEState *s, BlockDriverState *bs, const char *version, const char *serial) { int cylinders, heads, secs; uint64_t nb_sectors; - s->bs = dinfo->bdrv; - bdrv_get_geometry(s->bs, &nb_sectors); - bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs); + s->bs = bs; + bdrv_get_geometry(bs, &nb_sectors); + bdrv_guess_geometry(bs, &cylinders, &heads, &secs); s->cylinders = cylinders; s->heads = heads; s->sectors = secs; @@ -2613,11 +2613,11 @@ void ide_init_drive(IDEState *s, DriveInfo *dinfo, s->smart_autosave = 1; s->smart_errors = 0; s->smart_selftest_count = 0; - if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) { + if (bdrv_get_type_hint(bs) == BDRV_TYPE_CDROM) { s->is_cdrom = 1; - bdrv_set_change_cb(s->bs, cdrom_change_cb, s); + bdrv_set_change_cb(bs, cdrom_change_cb, s); } - if (serial && *serial) { + if (serial) { strncpy(s->drive_serial_str, serial, sizeof(s->drive_serial_str)); } else { snprintf(s->drive_serial_str, sizeof(s->drive_serial_str), @@ -2668,7 +2668,8 @@ void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0, dinfo = i == 0 ? hd0 : hd1; ide_init1(bus, i); if (dinfo) { - ide_init_drive(&bus->ifs[i], dinfo, NULL, dinfo->serial); + ide_init_drive(&bus->ifs[i], dinfo->bdrv, NULL, + *dinfo->serial ? dinfo->serial : NULL); } else { ide_reset(&bus->ifs[i]); } diff --git a/hw/ide/internal.h b/hw/ide/internal.h index eef1ee141d..0125a9f0b9 100644 --- a/hw/ide/internal.h +++ b/hw/ide/internal.h @@ -555,7 +555,7 @@ uint32_t ide_data_readw(void *opaque, uint32_t addr); void ide_data_writel(void *opaque, uint32_t addr, uint32_t val); uint32_t ide_data_readl(void *opaque, uint32_t addr); -void ide_init_drive(IDEState *s, DriveInfo *dinfo, +void ide_init_drive(IDEState *s, BlockDriverState *bs, const char *version, const char *serial); void ide_init2(IDEBus *bus, qemu_irq irq); void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0, diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c index 127478ba65..a5fdab04ba 100644 --- a/hw/ide/qdev.c +++ b/hw/ide/qdev.c @@ -39,7 +39,7 @@ static int ide_qdev_init(DeviceState *qdev, DeviceInfo *base) IDEDeviceInfo *info = DO_UPCAST(IDEDeviceInfo, qdev, base); IDEBus *bus = DO_UPCAST(IDEBus, qbus, qdev->parent_bus); - if (!dev->conf.dinfo) { + if (!dev->conf.bs) { fprintf(stderr, "%s: no drive specified\n", qdev->info->name); goto err; } @@ -83,7 +83,7 @@ IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive) dev = qdev_create(&bus->qbus, "ide-drive"); qdev_prop_set_uint32(dev, "unit", unit); - qdev_prop_set_drive(dev, "drive", drive); + qdev_prop_set_drive(dev, "drive", drive->bdrv); qdev_init_nofail(dev); return DO_UPCAST(IDEDevice, qdev, dev); } @@ -99,14 +99,18 @@ static int ide_drive_initfn(IDEDevice *dev) IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus); IDEState *s = bus->ifs + dev->unit; const char *serial; + DriveInfo *dinfo; serial = dev->serial; if (!serial) { /* try to fall back to value set with legacy -drive serial=... */ - serial = dev->conf.dinfo->serial; + dinfo = drive_get_by_blockdev(dev->conf.bs); + if (*dinfo->serial) { + serial = dinfo->serial; + } } - ide_init_drive(s, dev->conf.dinfo, dev->version, serial); + ide_init_drive(s, dev->conf.bs, dev->version, serial); if (!dev->version) { dev->version = qemu_strdup(s->version); diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c index 55c9fe3a79..d7431929ab 100644 --- a/hw/pci-hotplug.c +++ b/hw/pci-hotplug.c @@ -89,7 +89,7 @@ static int scsi_hot_add(Monitor *mon, DeviceState *adapter, * specified). */ dinfo->unit = qemu_opt_get_number(dinfo->opts, "unit", -1); - scsidev = scsi_bus_legacy_add_drive(scsibus, dinfo, dinfo->unit); + scsidev = scsi_bus_legacy_add_drive(scsibus, dinfo->bdrv, dinfo->unit); if (!scsidev) { return -1; } @@ -214,7 +214,7 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon, return NULL; } dev = pci_create(bus, devfn, "virtio-blk-pci"); - qdev_prop_set_drive(&dev->qdev, "drive", dinfo); + qdev_prop_set_drive(&dev->qdev, "drive", dinfo->bdrv); if (qdev_init(&dev->qdev) < 0) dev = NULL; break; diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c index d7dc234e25..49b33773b2 100644 --- a/hw/qdev-properties.c +++ b/hw/qdev-properties.c @@ -305,33 +305,36 @@ PropertyInfo qdev_prop_string = { static int parse_drive(DeviceState *dev, Property *prop, const char *str) { - DriveInfo **ptr = qdev_get_prop_ptr(dev, prop); + BlockDriverState **ptr = qdev_get_prop_ptr(dev, prop); + BlockDriverState *bs; - *ptr = drive_get_by_id(str); - if (*ptr == NULL) + bs = bdrv_find(str); + if (bs == NULL) return -ENOENT; + *ptr = bs; return 0; } static void free_drive(DeviceState *dev, Property *prop) { - DriveInfo **ptr = qdev_get_prop_ptr(dev, prop); + BlockDriverState **ptr = qdev_get_prop_ptr(dev, prop); if (*ptr) { - blockdev_auto_del((*ptr)->bdrv); + blockdev_auto_del(*ptr); } } static int print_drive(DeviceState *dev, Property *prop, char *dest, size_t len) { - DriveInfo **ptr = qdev_get_prop_ptr(dev, prop); - return snprintf(dest, len, "%s", (*ptr) ? (*ptr)->id : ""); + BlockDriverState **ptr = qdev_get_prop_ptr(dev, prop); + return snprintf(dest, len, "%s", + *ptr ? bdrv_get_device_name(*ptr) : ""); } PropertyInfo qdev_prop_drive = { .name = "drive", .type = PROP_TYPE_DRIVE, - .size = sizeof(DriveInfo*), + .size = sizeof(BlockDriverState *), .parse = parse_drive, .print = print_drive, .free = free_drive, @@ -657,7 +660,7 @@ void qdev_prop_set_string(DeviceState *dev, const char *name, char *value) qdev_prop_set(dev, name, &value, PROP_TYPE_STRING); } -void qdev_prop_set_drive(DeviceState *dev, const char *name, DriveInfo *value) +void qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState *value) { qdev_prop_set(dev, name, &value, PROP_TYPE_DRIVE); } diff --git a/hw/qdev.h b/hw/qdev.h index be5ad671cc..7a01a8170e 100644 --- a/hw/qdev.h +++ b/hw/qdev.h @@ -253,8 +253,8 @@ extern PropertyInfo qdev_prop_pci_devfn; DEFINE_PROP(_n, _s, _f, qdev_prop_netdev, VLANClientState*) #define DEFINE_PROP_VLAN(_n, _s, _f) \ DEFINE_PROP(_n, _s, _f, qdev_prop_vlan, VLANState*) -#define DEFINE_PROP_DRIVE(_n, _s, _f) \ - DEFINE_PROP(_n, _s, _f, qdev_prop_drive, DriveInfo*) +#define DEFINE_PROP_DRIVE(_n, _s, _f) \ + DEFINE_PROP(_n, _s, _f, qdev_prop_drive, BlockDriverState *) #define DEFINE_PROP_MACADDR(_n, _s, _f) \ DEFINE_PROP(_n, _s, _f, qdev_prop_macaddr, MACAddr) @@ -275,7 +275,7 @@ void qdev_prop_set_string(DeviceState *dev, const char *name, char *value); void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *value); void qdev_prop_set_netdev(DeviceState *dev, const char *name, VLANClientState *value); void qdev_prop_set_vlan(DeviceState *dev, const char *name, VLANState *value); -void qdev_prop_set_drive(DeviceState *dev, const char *name, DriveInfo *value); +void qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState *value); void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value); /* FIXME: Remove opaque pointer properties. */ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value); diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c index 898f442355..c7c3fc94bf 100644 --- a/hw/s390-virtio.c +++ b/hw/s390-virtio.c @@ -262,7 +262,7 @@ static void s390_init(ram_addr_t ram_size, } dev = qdev_create((BusState *)s390_bus, "virtio-blk-s390"); - qdev_prop_set_drive(dev, "drive", dinfo); + qdev_prop_set_drive(dev, "drive", dinfo->bdrv); qdev_init_nofail(dev); } } diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c index d5b66c14dd..2c58acac49 100644 --- a/hw/scsi-bus.c +++ b/hw/scsi-bus.c @@ -83,15 +83,15 @@ void scsi_qdev_register(SCSIDeviceInfo *info) } /* handle legacy '-drive if=scsi,...' cmd line args */ -SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, DriveInfo *dinfo, int unit) +SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, int unit) { const char *driver; DeviceState *dev; - driver = bdrv_is_sg(dinfo->bdrv) ? "scsi-generic" : "scsi-disk"; + driver = bdrv_is_sg(bdrv) ? "scsi-generic" : "scsi-disk"; dev = qdev_create(&bus->qbus, driver); qdev_prop_set_uint32(dev, "scsi-id", unit); - qdev_prop_set_drive(dev, "drive", dinfo); + qdev_prop_set_drive(dev, "drive", bdrv); if (qdev_init(dev) < 0) return NULL; return DO_UPCAST(SCSIDevice, qdev, dev); @@ -107,7 +107,7 @@ int scsi_bus_legacy_handle_cmdline(SCSIBus *bus) if (dinfo == NULL) { continue; } - if (!scsi_bus_legacy_add_drive(bus, dinfo, unit)) { + if (!scsi_bus_legacy_add_drive(bus, dinfo->bdrv, unit)) { res = -1; break; } diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index d76e640412..e900eff024 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -1043,26 +1043,24 @@ static void scsi_destroy(SCSIDevice *dev) SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev); scsi_disk_purge_requests(s); - blockdev_mark_auto_del(s->qdev.conf.dinfo->bdrv); + blockdev_mark_auto_del(s->qdev.conf.bs); } static int scsi_disk_initfn(SCSIDevice *dev) { SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev); + DriveInfo *dinfo; - if (!s->qdev.conf.dinfo || !s->qdev.conf.dinfo->bdrv) { + if (!s->qdev.conf.bs) { error_report("scsi-disk: drive property not set"); return -1; } - s->bs = s->qdev.conf.dinfo->bdrv; + s->bs = s->qdev.conf.bs; if (!s->serial) { - if (*dev->conf.dinfo->serial) { - /* try to fall back to value set with legacy -drive serial=... */ - s->serial = qemu_strdup(dev->conf.dinfo->serial); - } else { - s->serial = qemu_strdup("0"); - } + /* try to fall back to value set with legacy -drive serial=... */ + dinfo = drive_get_by_blockdev(s->bs); + s->serial = qemu_strdup(*dinfo->serial ? dinfo->serial : "0"); } if (!s->version) { diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c index 1859c94b7c..79347f4a67 100644 --- a/hw/scsi-generic.c +++ b/hw/scsi-generic.c @@ -453,7 +453,7 @@ static void scsi_destroy(SCSIDevice *d) r = DO_UPCAST(SCSIGenericReq, req, QTAILQ_FIRST(&s->qdev.requests)); scsi_remove_request(r); } - blockdev_mark_auto_del(s->qdev.conf.dinfo->bdrv); + blockdev_mark_auto_del(s->qdev.conf.bs); } static int scsi_generic_initfn(SCSIDevice *dev) @@ -462,11 +462,11 @@ static int scsi_generic_initfn(SCSIDevice *dev) int sg_version; struct sg_scsi_id scsiid; - if (!s->qdev.conf.dinfo || !s->qdev.conf.dinfo->bdrv) { + if (!s->qdev.conf.bs) { error_report("scsi-generic: drive property not set"); return -1; } - s->bs = s->qdev.conf.dinfo->bdrv; + s->bs = s->qdev.conf.bs; /* check we are really using a /dev/sg* file */ if (!bdrv_is_sg(s->bs)) { diff --git a/hw/scsi.h b/hw/scsi.h index b1b5f730d5..4fbf1d5dfd 100644 --- a/hw/scsi.h +++ b/hw/scsi.h @@ -97,7 +97,7 @@ static inline SCSIBus *scsi_bus_from_device(SCSIDevice *d) return DO_UPCAST(SCSIBus, qbus, d->qdev.parent_bus); } -SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, DriveInfo *dinfo, int unit); +SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, int unit); int scsi_bus_legacy_handle_cmdline(SCSIBus *bus); void scsi_dev_clear_sense(SCSIDevice *dev); diff --git a/hw/usb-msd.c b/hw/usb-msd.c index 3dbfcabeb1..19a14b4f80 100644 --- a/hw/usb-msd.c +++ b/hw/usb-msd.c @@ -522,9 +522,9 @@ static void usb_msd_password_cb(void *opaque, int err) static int usb_msd_initfn(USBDevice *dev) { MSDState *s = DO_UPCAST(MSDState, dev, dev); - DriveInfo *dinfo = s->conf.dinfo; + BlockDriverState *bs = s->conf.bs; - if (!dinfo || !dinfo->bdrv) { + if (!bs) { error_report("usb-msd: drive property not set"); return -1; } @@ -538,21 +538,20 @@ static int usb_msd_initfn(USBDevice *dev) * * The hack is probably a bad idea. */ - s->conf.dinfo = NULL; + s->conf.bs = NULL; s->dev.speed = USB_SPEED_FULL; scsi_bus_new(&s->bus, &s->dev.qdev, 0, 1, usb_msd_command_complete); - s->scsi_dev = scsi_bus_legacy_add_drive(&s->bus, dinfo, 0); + s->scsi_dev = scsi_bus_legacy_add_drive(&s->bus, bs, 0); if (!s->scsi_dev) { return -1; } s->bus.qbus.allow_hotplug = 0; usb_msd_handle_reset(dev); - if (bdrv_key_required(dinfo->bdrv)) { + if (bdrv_key_required(bs)) { if (cur_mon) { - monitor_read_bdrv_key_start(cur_mon, dinfo->bdrv, - usb_msd_password_cb, s); + monitor_read_bdrv_key_start(cur_mon, bs, usb_msd_password_cb, s); s->dev.auto_attach = 0; } else { autostart = 0; @@ -610,7 +609,7 @@ static USBDevice *usb_msd_init(const char *filename) if (!dev) { return NULL; } - qdev_prop_set_drive(&dev->qdev, "drive", dinfo); + qdev_prop_set_drive(&dev->qdev, "drive", dinfo->bdrv); if (qdev_init(&dev->qdev) < 0) return NULL; diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c index 0bf929aba9..71c5d21e61 100644 --- a/hw/virtio-blk.c +++ b/hw/virtio-blk.c @@ -489,7 +489,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf) s->vdev.get_config = virtio_blk_update_config; s->vdev.get_features = virtio_blk_get_features; s->vdev.reset = virtio_blk_reset; - s->bs = conf->dinfo->bdrv; + s->bs = conf->bs; s->conf = conf; s->rq = NULL; s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1; diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index 31a68fec40..c6ef8254e3 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -547,7 +547,7 @@ static int virtio_blk_init_pci(PCIDevice *pci_dev) proxy->class_code != PCI_CLASS_STORAGE_OTHER) proxy->class_code = PCI_CLASS_STORAGE_SCSI; - if (!proxy->block.dinfo) { + if (!proxy->block.bs) { error_report("virtio-blk-pci: drive property not set"); return -1; } @@ -571,7 +571,7 @@ static int virtio_blk_exit_pci(PCIDevice *pci_dev) { VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); - blockdev_mark_auto_del(proxy->block.dinfo->bdrv); + blockdev_mark_auto_del(proxy->block.bs); return virtio_exit_pci(pci_dev); } From dfb0acd88782573e075251ef323e23a4bffdbf93 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Wed, 5 May 2010 18:46:17 +0200 Subject: [PATCH 10/23] blockdev: drive_get_by_id() is no longer used, remove Signed-off-by: Markus Armbruster Signed-off-by: Kevin Wolf --- blockdev.c | 12 ------------ blockdev.h | 1 - 2 files changed, 13 deletions(-) diff --git a/blockdev.c b/blockdev.c index 48481120c2..cecde2b873 100644 --- a/blockdev.c +++ b/blockdev.c @@ -75,18 +75,6 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit) return NULL; } -DriveInfo *drive_get_by_id(const char *id) -{ - DriveInfo *dinfo; - - QTAILQ_FOREACH(dinfo, &drives, next) { - if (strcmp(id, dinfo->id)) - continue; - return dinfo; - } - return NULL; -} - int drive_get_max_bus(BlockInterfaceType type) { int max_bus; diff --git a/blockdev.h b/blockdev.h index 32e6979456..37f3a017ea 100644 --- a/blockdev.h +++ b/blockdev.h @@ -41,7 +41,6 @@ typedef struct DriveInfo { #define MAX_SCSI_DEVS 7 extern DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit); -extern DriveInfo *drive_get_by_id(const char *id); extern int drive_get_max_bus(BlockInterfaceType type); extern void drive_uninit(DriveInfo *dinfo); extern DriveInfo *drive_get_by_blockdev(BlockDriverState *bs); From 18846dee1a795b4345ac0bd10b70a3a46fd14287 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 29 Jun 2010 16:58:30 +0200 Subject: [PATCH 11/23] block: Catch attempt to attach multiple devices to a blockdev For instance, -device scsi-disk,drive=foo -device scsi-disk,drive=foo happily creates two SCSI disks connected to the same block device. It's all downhill from there. Device usb-storage deliberately attaches twice to the same blockdev, which fails with the fix in place. Detach before the second attach there. Also catch attempt to delete while a guest device model is attached. Signed-off-by: Markus Armbruster Signed-off-by: Kevin Wolf --- block.c | 22 ++++++++++++++++++++++ block.h | 3 +++ block_int.h | 2 ++ hw/fdc.c | 10 +++++----- hw/ide/qdev.c | 2 +- hw/pci-hotplug.c | 6 +++++- hw/qdev-properties.c | 22 +++++++++++++++++++++- hw/qdev.h | 3 ++- hw/s390-virtio.c | 2 +- hw/scsi-bus.c | 5 ++++- hw/usb-msd.c | 12 ++++++++---- 11 files changed, 74 insertions(+), 15 deletions(-) diff --git a/block.c b/block.c index 31ca4c5a43..4c65035313 100644 --- a/block.c +++ b/block.c @@ -665,6 +665,8 @@ void bdrv_close_all(void) void bdrv_delete(BlockDriverState *bs) { + assert(!bs->peer); + /* remove from list, if necessary */ if (bs->device_name[0] != '\0') { QTAILQ_REMOVE(&bdrv_states, bs, list); @@ -678,6 +680,26 @@ void bdrv_delete(BlockDriverState *bs) qemu_free(bs); } +int bdrv_attach(BlockDriverState *bs, DeviceState *qdev) +{ + if (bs->peer) { + return -EBUSY; + } + bs->peer = qdev; + return 0; +} + +void bdrv_detach(BlockDriverState *bs, DeviceState *qdev) +{ + assert(bs->peer == qdev); + bs->peer = NULL; +} + +DeviceState *bdrv_get_attached(BlockDriverState *bs) +{ + return bs->peer; +} + /* * Run consistency checks on an image * diff --git a/block.h b/block.h index 6a157f4382..88ac06eea6 100644 --- a/block.h +++ b/block.h @@ -71,6 +71,9 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags); int bdrv_open(BlockDriverState *bs, const char *filename, int flags, BlockDriver *drv); void bdrv_close(BlockDriverState *bs); +int bdrv_attach(BlockDriverState *bs, DeviceState *qdev); +void bdrv_detach(BlockDriverState *bs, DeviceState *qdev); +DeviceState *bdrv_get_attached(BlockDriverState *bs); int bdrv_check(BlockDriverState *bs); int bdrv_read(BlockDriverState *bs, int64_t sector_num, uint8_t *buf, int nb_sectors); diff --git a/block_int.h b/block_int.h index e60aed4a04..a94b80152f 100644 --- a/block_int.h +++ b/block_int.h @@ -148,6 +148,8 @@ struct BlockDriverState { BlockDriver *drv; /* NULL means no media */ void *opaque; + DeviceState *peer; + char filename[1024]; char backing_file[1024]; /* if non zero, the image is a diff of this file image */ diff --git a/hw/fdc.c b/hw/fdc.c index 08712bc7d2..1496cfa14d 100644 --- a/hw/fdc.c +++ b/hw/fdc.c @@ -1860,10 +1860,10 @@ FDCtrl *fdctrl_init_isa(DriveInfo **fds) dev = isa_create("isa-fdc"); if (fds[0]) { - qdev_prop_set_drive(&dev->qdev, "driveA", fds[0]->bdrv); + qdev_prop_set_drive_nofail(&dev->qdev, "driveA", fds[0]->bdrv); } if (fds[1]) { - qdev_prop_set_drive(&dev->qdev, "driveB", fds[1]->bdrv); + qdev_prop_set_drive_nofail(&dev->qdev, "driveB", fds[1]->bdrv); } if (qdev_init(&dev->qdev) < 0) return NULL; @@ -1882,10 +1882,10 @@ FDCtrl *fdctrl_init_sysbus(qemu_irq irq, int dma_chann, fdctrl = &sys->state; fdctrl->dma_chann = dma_chann; /* FIXME */ if (fds[0]) { - qdev_prop_set_drive(dev, "driveA", fds[0]->bdrv); + qdev_prop_set_drive_nofail(dev, "driveA", fds[0]->bdrv); } if (fds[1]) { - qdev_prop_set_drive(dev, "driveB", fds[1]->bdrv); + qdev_prop_set_drive_nofail(dev, "driveB", fds[1]->bdrv); } qdev_init_nofail(dev); sysbus_connect_irq(&sys->busdev, 0, irq); @@ -1903,7 +1903,7 @@ FDCtrl *sun4m_fdctrl_init(qemu_irq irq, target_phys_addr_t io_base, dev = qdev_create(NULL, "SUNW,fdtwo"); if (fds[0]) { - qdev_prop_set_drive(dev, "drive", fds[0]->bdrv); + qdev_prop_set_drive_nofail(dev, "drive", fds[0]->bdrv); } qdev_init_nofail(dev); sys = DO_UPCAST(FDCtrlSysBus, busdev.qdev, dev); diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c index a5fdab04ba..b34c473336 100644 --- a/hw/ide/qdev.c +++ b/hw/ide/qdev.c @@ -83,7 +83,7 @@ IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive) dev = qdev_create(&bus->qbus, "ide-drive"); qdev_prop_set_uint32(dev, "unit", unit); - qdev_prop_set_drive(dev, "drive", drive->bdrv); + qdev_prop_set_drive_nofail(dev, "drive", drive->bdrv); qdev_init_nofail(dev); return DO_UPCAST(IDEDevice, qdev, dev); } diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c index d7431929ab..fe468d646e 100644 --- a/hw/pci-hotplug.c +++ b/hw/pci-hotplug.c @@ -214,7 +214,11 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon, return NULL; } dev = pci_create(bus, devfn, "virtio-blk-pci"); - qdev_prop_set_drive(&dev->qdev, "drive", dinfo->bdrv); + if (qdev_prop_set_drive(&dev->qdev, "drive", dinfo->bdrv) < 0) { + qdev_free(&dev->qdev); + dev = NULL; + break; + } if (qdev_init(&dev->qdev) < 0) dev = NULL; break; diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c index 49b33773b2..7e3e99efcb 100644 --- a/hw/qdev-properties.c +++ b/hw/qdev-properties.c @@ -311,6 +311,8 @@ static int parse_drive(DeviceState *dev, Property *prop, const char *str) bs = bdrv_find(str); if (bs == NULL) return -ENOENT; + if (bdrv_attach(bs, dev) < 0) + return -EEXIST; *ptr = bs; return 0; } @@ -320,6 +322,7 @@ static void free_drive(DeviceState *dev, Property *prop) BlockDriverState **ptr = qdev_get_prop_ptr(dev, prop); if (*ptr) { + bdrv_detach(*ptr, dev); blockdev_auto_del(*ptr); } } @@ -660,11 +663,28 @@ void qdev_prop_set_string(DeviceState *dev, const char *name, char *value) qdev_prop_set(dev, name, &value, PROP_TYPE_STRING); } -void qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState *value) +int qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState *value) { + int res; + + res = bdrv_attach(value, dev); + if (res < 0) { + error_report("Can't attach drive %s to %s.%s: %s", + bdrv_get_device_name(value), + dev->id ? dev->id : dev->info->name, + name, strerror(-res)); + return -1; + } qdev_prop_set(dev, name, &value, PROP_TYPE_DRIVE); + return 0; } +void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name, BlockDriverState *value) +{ + if (qdev_prop_set_drive(dev, name, value) < 0) { + exit(1); + } +} void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *value) { qdev_prop_set(dev, name, &value, PROP_TYPE_CHR); diff --git a/hw/qdev.h b/hw/qdev.h index 7a01a8170e..cbc89f2c1e 100644 --- a/hw/qdev.h +++ b/hw/qdev.h @@ -275,7 +275,8 @@ void qdev_prop_set_string(DeviceState *dev, const char *name, char *value); void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *value); void qdev_prop_set_netdev(DeviceState *dev, const char *name, VLANClientState *value); void qdev_prop_set_vlan(DeviceState *dev, const char *name, VLANState *value); -void qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState *value); +int qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState *value) QEMU_WARN_UNUSED_RESULT; +void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name, BlockDriverState *value); void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value); /* FIXME: Remove opaque pointer properties. */ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value); diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c index c7c3fc94bf..6af58e23af 100644 --- a/hw/s390-virtio.c +++ b/hw/s390-virtio.c @@ -262,7 +262,7 @@ static void s390_init(ram_addr_t ram_size, } dev = qdev_create((BusState *)s390_bus, "virtio-blk-s390"); - qdev_prop_set_drive(dev, "drive", dinfo->bdrv); + qdev_prop_set_drive_nofail(dev, "drive", dinfo->bdrv); qdev_init_nofail(dev); } } diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c index 2c58acac49..b84b9b98b5 100644 --- a/hw/scsi-bus.c +++ b/hw/scsi-bus.c @@ -91,7 +91,10 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, int driver = bdrv_is_sg(bdrv) ? "scsi-generic" : "scsi-disk"; dev = qdev_create(&bus->qbus, driver); qdev_prop_set_uint32(dev, "scsi-id", unit); - qdev_prop_set_drive(dev, "drive", bdrv); + if (qdev_prop_set_drive(dev, "drive", bdrv) < 0) { + qdev_free(dev); + return NULL; + } if (qdev_init(dev) < 0) return NULL; return DO_UPCAST(SCSIDevice, qdev, dev); diff --git a/hw/usb-msd.c b/hw/usb-msd.c index 19a14b4f80..65e9624e54 100644 --- a/hw/usb-msd.c +++ b/hw/usb-msd.c @@ -532,12 +532,13 @@ static int usb_msd_initfn(USBDevice *dev) /* * Hack alert: this pretends to be a block device, but it's really * a SCSI bus that can serve only a single device, which it - * creates automatically. Two drive properties pointing to the - * same drive is not good: free_drive() dies for the second one. - * Zap the one we're not going to use. + * creates automatically. But first it needs to detach from its + * blockdev, or else scsi_bus_legacy_add_drive() dies when it + * attaches again. * * The hack is probably a bad idea. */ + bdrv_detach(bs, &s->dev.qdev); s->conf.bs = NULL; s->dev.speed = USB_SPEED_FULL; @@ -609,7 +610,10 @@ static USBDevice *usb_msd_init(const char *filename) if (!dev) { return NULL; } - qdev_prop_set_drive(&dev->qdev, "drive", dinfo->bdrv); + if (qdev_prop_set_drive(&dev->qdev, "drive", dinfo->bdrv) < 0) { + qdev_free(&dev->qdev); + return NULL; + } if (qdev_init(&dev->qdev) < 0) return NULL; From bb67ab0290b9a83d4d444e2e9a0311d3b9b54385 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 1 Jun 2010 10:47:34 +0200 Subject: [PATCH 12/23] qemu-option: New qemu_opts_reset() Signed-off-by: Markus Armbruster Signed-off-by: Kevin Wolf --- qemu-option.c | 9 +++++++++ qemu-option.h | 1 + 2 files changed, 10 insertions(+) diff --git a/qemu-option.c b/qemu-option.c index 7f70d0f5fb..30327d4804 100644 --- a/qemu-option.c +++ b/qemu-option.c @@ -719,6 +719,15 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id, int fail_if_exist return opts; } +void qemu_opts_reset(QemuOptsList *list) +{ + QemuOpts *opts, *next_opts; + + QTAILQ_FOREACH_SAFE(opts, &list->head, next, next_opts) { + qemu_opts_del(opts); + } +} + int qemu_opts_set(QemuOptsList *list, const char *id, const char *name, const char *value) { diff --git a/qemu-option.h b/qemu-option.h index 4823219a18..9e2406c562 100644 --- a/qemu-option.h +++ b/qemu-option.h @@ -115,6 +115,7 @@ int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque, QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id); QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id, int fail_if_exists); +void qemu_opts_reset(QemuOptsList *list); int qemu_opts_set(QemuOptsList *list, const char *id, const char *name, const char *value); const char *qemu_opts_id(QemuOpts *opts); From 327cdad4163dcf3928472e0cdbf6b6b97c88670b Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 30 Jun 2010 17:40:42 +0200 Subject: [PATCH 13/23] blkdebug: Fix set_state_opts definition The list head was initialized to point to the wrong list, so all actions ended up being handled as inject-error even if they were set-state in fact. Signed-off-by: Kevin Wolf --- block/blkdebug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/blkdebug.c b/block/blkdebug.c index 98fed944b1..4ec8ca6984 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -111,7 +111,7 @@ static QemuOptsList inject_error_opts = { static QemuOptsList set_state_opts = { .name = "set-state", - .head = QTAILQ_HEAD_INITIALIZER(inject_error_opts.head), + .head = QTAILQ_HEAD_INITIALIZER(set_state_opts.head), .desc = { { .name = "event", From 698f0d52cde708557801de3c583cafc4a0497a14 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 30 Jun 2010 17:42:23 +0200 Subject: [PATCH 14/23] blkdebug: Free QemuOpts after having read the config Forgetting to free them means that the next instance inherits all rules and gets its own rules only additionally. Signed-off-by: Kevin Wolf --- block/blkdebug.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/block/blkdebug.c b/block/blkdebug.c index 4ec8ca6984..78cbff4bdc 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -267,6 +267,8 @@ static int read_config(BDRVBlkdebugState *s, const char *filename) ret = 0; fail: + qemu_opts_reset(&inject_error_opts); + qemu_opts_reset(&set_state_opts); fclose(f); return ret; } From 8db520cee89bb84f21d107cea2d786894901bc54 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 30 Jun 2010 17:43:40 +0200 Subject: [PATCH 15/23] blkdebug: Initialize state as 1 state = 0 in rules means that the rule is valid for any state. Therefore it's impossible to have a rule that works only in the initial state. This changes the initial state from 0 to 1 to make this possible. Signed-off-by: Kevin Wolf --- block/blkdebug.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/block/blkdebug.c b/block/blkdebug.c index 78cbff4bdc..2a63df9323 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -301,6 +301,9 @@ static int blkdebug_open(BlockDriverState *bs, const char *filename, int flags) } filename = c + 1; + /* Set initial state */ + s->vars.state = 1; + /* Open the backing file */ ret = bdrv_file_open(&bs->file, filename, flags); if (ret < 0) { From f9092b108f7b35e463e58bd3dd348ff002ba9e63 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 25 Jun 2010 10:33:39 +0200 Subject: [PATCH 16/23] savevm: Survive hot-unplug of snapshot device savevm.c keeps a pointer to the snapshot block device. If you manage to get that device deleted, the pointer dangles, and the next snapshot operation will crash & burn. Unplugging a guest device that uses it does the trick: $ MALLOC_PERTURB_=234 qemu-system-x86_64 [...] QEMU 0.12.50 monitor - type 'help' for more information (qemu) info snapshots No available block device supports snapshots (qemu) drive_add auto if=none,file=tmp.qcow2 OK (qemu) device_add usb-storage,id=foo,drive=none1 (qemu) info snapshots Snapshot devices: none1 Snapshot list (from none1): ID TAG VM SIZE DATE VM CLOCK (qemu) device_del foo (qemu) info snapshots Snapshot devices: Segmentation fault (core dumped) Move management of that pointer to block.c, and zap it when the device it points becomes unusable. Signed-off-by: Markus Armbruster Signed-off-by: Kevin Wolf --- block.c | 26 ++++++++++++++++++++++++++ block.h | 1 + savevm.c | 31 ++++--------------------------- 3 files changed, 31 insertions(+), 27 deletions(-) diff --git a/block.c b/block.c index 4c65035313..feda755294 100644 --- a/block.c +++ b/block.c @@ -63,6 +63,9 @@ static QTAILQ_HEAD(, BlockDriverState) bdrv_states = static QLIST_HEAD(, BlockDriver) bdrv_drivers = QLIST_HEAD_INITIALIZER(bdrv_drivers); +/* The device to use for VM snapshots */ +static BlockDriverState *bs_snapshots; + /* If non-zero, use only whitelisted block drivers */ static int use_bdrv_whitelist; @@ -629,6 +632,9 @@ unlink_and_fail: void bdrv_close(BlockDriverState *bs) { if (bs->drv) { + if (bs == bs_snapshots) { + bs_snapshots = NULL; + } if (bs->backing_hd) { bdrv_delete(bs->backing_hd); bs->backing_hd = NULL; @@ -677,6 +683,7 @@ void bdrv_delete(BlockDriverState *bs) bdrv_delete(bs->file); } + assert(bs != bs_snapshots); qemu_free(bs); } @@ -1778,6 +1785,25 @@ int bdrv_can_snapshot(BlockDriverState *bs) return 1; } +BlockDriverState *bdrv_snapshots(void) +{ + BlockDriverState *bs; + + if (bs_snapshots) + return bs_snapshots; + + bs = NULL; + while ((bs = bdrv_next(bs))) { + if (bdrv_can_snapshot(bs)) { + goto ok; + } + } + return NULL; + ok: + bs_snapshots = bs; + return bs; +} + int bdrv_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info) { diff --git a/block.h b/block.h index 88ac06eea6..012c2a1156 100644 --- a/block.h +++ b/block.h @@ -193,6 +193,7 @@ const char *bdrv_get_encrypted_filename(BlockDriverState *bs); void bdrv_get_backing_filename(BlockDriverState *bs, char *filename, int filename_size); int bdrv_can_snapshot(BlockDriverState *bs); +BlockDriverState *bdrv_snapshots(void); int bdrv_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info); int bdrv_snapshot_goto(BlockDriverState *bs, diff --git a/savevm.c b/savevm.c index 20354a8714..f1f450edc4 100644 --- a/savevm.c +++ b/savevm.c @@ -83,9 +83,6 @@ #include "qemu_socket.h" #include "qemu-queue.h" -/* point to the block driver where the snapshots are managed */ -static BlockDriverState *bs_snapshots; - #define SELF_ANNOUNCE_ROUNDS 5 #ifndef ETH_P_RARP @@ -1575,26 +1572,6 @@ out: return ret; } -static BlockDriverState *get_bs_snapshots(void) -{ - BlockDriverState *bs; - - if (bs_snapshots) - return bs_snapshots; - /* FIXME what if bs_snapshots gets hot-unplugged? */ - - bs = NULL; - while ((bs = bdrv_next(bs))) { - if (bdrv_can_snapshot(bs)) { - goto ok; - } - } - return NULL; - ok: - bs_snapshots = bs; - return bs; -} - static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, const char *name) { @@ -1674,7 +1651,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) } } - bs = get_bs_snapshots(); + bs = bdrv_snapshots(); if (!bs) { monitor_printf(mon, "No block device can accept snapshots\n"); return; @@ -1769,7 +1746,7 @@ int load_vmstate(const char *name) } } - bs = get_bs_snapshots(); + bs = bdrv_snapshots(); if (!bs) { error_report("No block device supports snapshots"); return -EINVAL; @@ -1833,7 +1810,7 @@ void do_delvm(Monitor *mon, const QDict *qdict) int ret; const char *name = qdict_get_str(qdict, "name"); - bs = get_bs_snapshots(); + bs = bdrv_snapshots(); if (!bs) { monitor_printf(mon, "No block device supports snapshots\n"); return; @@ -1863,7 +1840,7 @@ void do_info_snapshots(Monitor *mon) int nb_sns, i; char buf[256]; - bs = get_bs_snapshots(); + bs = bdrv_snapshots(); if (!bs) { monitor_printf(mon, "No available block device supports snapshots\n"); return; From 3ac906f771c42363dc05e0a393defd21a57d3790 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 1 Jul 2010 09:30:38 +0200 Subject: [PATCH 17/23] block: Clean up bdrv_snapshots() Signed-off-by: Markus Armbruster Signed-off-by: Kevin Wolf --- block.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/block.c b/block.c index feda755294..003d13213e 100644 --- a/block.c +++ b/block.c @@ -1789,19 +1789,18 @@ BlockDriverState *bdrv_snapshots(void) { BlockDriverState *bs; - if (bs_snapshots) + if (bs_snapshots) { return bs_snapshots; + } bs = NULL; while ((bs = bdrv_next(bs))) { if (bdrv_can_snapshot(bs)) { - goto ok; + bs_snapshots = bs; + return bs; } } return NULL; - ok: - bs_snapshots = bs; - return bs; } int bdrv_snapshot_create(BlockDriverState *bs, From 7d0d69509a966a5328e4fed7d0a942112d431c35 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 25 Jun 2010 13:42:14 +0200 Subject: [PATCH 18/23] block: Fix virtual media change for if=none BlockDriverState member removable controls whether virtual media change (monitor commands change, eject) is allowed. It is set when the "type hint" is BDRV_TYPE_CDROM or BDRV_TYPE_FLOPPY. The type hint is only set by drive_init(). It sets BDRV_TYPE_FLOPPY for if=floppy. It sets BDRV_TYPE_CDROM for media=cdrom and if=ide, scsi, xen, or none. if=ide and if=scsi work, because the type hint makes it a CD-ROM. if=xen likewise, I think. For the same reason, if=none works when it's used by ide-drive or scsi-disk. For other guest devices, there are problems: * fdc: you can't change virtual media $ qemu [...] -drive if=none,id=foo,... -global isa-fdc.driveA=foo QEMU 0.12.50 monitor - type 'help' for more information (qemu) eject foo Device 'foo' is not removable unless you add media=cdrom, but that makes it readonly. * virtio: if you add media=cdrom, you can change virtual media. If you eject, the guest gets I/O errors. If you change, the guest sees the drive's contents suddenly change. * scsi-generic: if you add media=cdrom, you can change virtual media. I didn't test what that does to the guest or the physical device, but it can't be pretty. Signed-off-by: Markus Armbruster Signed-off-by: Kevin Wolf --- block.c | 8 ++++++++ block.h | 1 + hw/fdc.c | 10 ++++++++-- hw/ide/core.c | 1 + hw/scsi-disk.c | 5 ++++- hw/scsi-generic.c | 1 + hw/virtio-blk.c | 1 + 7 files changed, 24 insertions(+), 3 deletions(-) diff --git a/block.c b/block.c index 003d13213e..9176decf33 100644 --- a/block.c +++ b/block.c @@ -1299,6 +1299,14 @@ BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read) return is_read ? bs->on_read_error : bs->on_write_error; } +void bdrv_set_removable(BlockDriverState *bs, int removable) +{ + bs->removable = removable; + if (removable && bs == bs_snapshots) { + bs_snapshots = NULL; + } +} + int bdrv_is_removable(BlockDriverState *bs) { return bs->removable; diff --git a/block.h b/block.h index 012c2a1156..3d03b3e041 100644 --- a/block.h +++ b/block.h @@ -162,6 +162,7 @@ int bdrv_get_translation_hint(BlockDriverState *bs); void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error, BlockErrorAction on_write_error); BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read); +void bdrv_set_removable(BlockDriverState *bs, int removable); int bdrv_is_removable(BlockDriverState *bs); int bdrv_is_read_only(BlockDriverState *bs); int bdrv_is_sg(BlockDriverState *bs); diff --git a/hw/fdc.c b/hw/fdc.c index 1496cfa14d..6c748782e3 100644 --- a/hw/fdc.c +++ b/hw/fdc.c @@ -1847,10 +1847,16 @@ static void fdctrl_result_timer(void *opaque) static void fdctrl_connect_drives(FDCtrl *fdctrl) { unsigned int i; + FDrive *drive; for (i = 0; i < MAX_FD; i++) { - fd_init(&fdctrl->drives[i]); - fd_revalidate(&fdctrl->drives[i]); + drive = &fdctrl->drives[i]; + + fd_init(drive); + fd_revalidate(drive); + if (drive->bs) { + bdrv_set_removable(drive->bs, 1); + } } } diff --git a/hw/ide/core.c b/hw/ide/core.c index cc4591baa0..ebdceb5fec 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -2629,6 +2629,7 @@ void ide_init_drive(IDEState *s, BlockDriverState *bs, pstrcpy(s->version, sizeof(s->version), QEMU_VERSION); } ide_reset(s); + bdrv_set_removable(bs, s->is_cdrom); } static void ide_init1(IDEBus *bus, int unit) diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index e900eff024..3e41011ccb 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -1049,6 +1049,7 @@ static void scsi_destroy(SCSIDevice *dev) static int scsi_disk_initfn(SCSIDevice *dev) { SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev); + int is_cd; DriveInfo *dinfo; if (!s->qdev.conf.bs) { @@ -1056,6 +1057,7 @@ static int scsi_disk_initfn(SCSIDevice *dev) return -1; } s->bs = s->qdev.conf.bs; + is_cd = bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM; if (!s->serial) { /* try to fall back to value set with legacy -drive serial=... */ @@ -1072,7 +1074,7 @@ static int scsi_disk_initfn(SCSIDevice *dev) return -1; } - if (bdrv_get_type_hint(s->bs) == BDRV_TYPE_CDROM) { + if (is_cd) { s->qdev.blocksize = 2048; } else { s->qdev.blocksize = s->qdev.conf.logical_block_size; @@ -1081,6 +1083,7 @@ static int scsi_disk_initfn(SCSIDevice *dev) s->qdev.type = TYPE_DISK; qemu_add_vm_change_state_handler(scsi_dma_restart_cb, s); + bdrv_set_removable(s->bs, is_cd); return 0; } diff --git a/hw/scsi-generic.c b/hw/scsi-generic.c index 79347f4a67..3915e7844e 100644 --- a/hw/scsi-generic.c +++ b/hw/scsi-generic.c @@ -509,6 +509,7 @@ static int scsi_generic_initfn(SCSIDevice *dev) DPRINTF("block size %d\n", s->qdev.blocksize); s->driver_status = 0; memset(s->sensebuf, 0, sizeof(s->sensebuf)); + bdrv_set_removable(s->bs, 0); return 0; } diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c index 71c5d21e61..f0b3ba5cb1 100644 --- a/hw/virtio-blk.c +++ b/hw/virtio-blk.c @@ -500,6 +500,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, BlockConf *conf) qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s); register_savevm("virtio-blk", virtio_blk_id++, 2, virtio_blk_save, virtio_blk_load, s); + bdrv_set_removable(s->bs, 0); return &s->vdev; } From 57c888664b5eb7edbbce4be98cb1406aa0d85c2b Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 24 Jun 2010 19:59:29 +0200 Subject: [PATCH 19/23] ide: Make PIIX and ISA IDE init functions return the qdev Signed-off-by: Markus Armbruster Signed-off-by: Kevin Wolf --- hw/ide.h | 11 ++++++----- hw/ide/isa.c | 8 ++++---- hw/ide/piix.c | 6 ++++-- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/hw/ide.h b/hw/ide.h index bb635b6c06..82b3c11283 100644 --- a/hw/ide.h +++ b/hw/ide.h @@ -1,17 +1,18 @@ #ifndef HW_IDE_H #define HW_IDE_H -#include "qdev.h" +#include "isa.h" +#include "pci.h" /* ide-isa.c */ -int isa_ide_init(int iobase, int iobase2, int isairq, - DriveInfo *hd0, DriveInfo *hd1); +ISADevice *isa_ide_init(int iobase, int iobase2, int isairq, + DriveInfo *hd0, DriveInfo *hd1); /* ide-pci.c */ void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int secondary_ide_enabled); -void pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); -void pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); +PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); +PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn); /* ide-macio.c */ diff --git a/hw/ide/isa.c b/hw/ide/isa.c index b6c6347289..10777caec1 100644 --- a/hw/ide/isa.c +++ b/hw/ide/isa.c @@ -75,8 +75,8 @@ static int isa_ide_initfn(ISADevice *dev) return 0; }; -int isa_ide_init(int iobase, int iobase2, int isairq, - DriveInfo *hd0, DriveInfo *hd1) +ISADevice *isa_ide_init(int iobase, int iobase2, int isairq, + DriveInfo *hd0, DriveInfo *hd1) { ISADevice *dev; ISAIDEState *s; @@ -86,14 +86,14 @@ int isa_ide_init(int iobase, int iobase2, int isairq, qdev_prop_set_uint32(&dev->qdev, "iobase2", iobase2); qdev_prop_set_uint32(&dev->qdev, "irq", isairq); if (qdev_init(&dev->qdev) < 0) - return -1; + return NULL; s = DO_UPCAST(ISAIDEState, dev, dev); if (hd0) ide_create_drive(&s->bus, 0, hd0); if (hd1) ide_create_drive(&s->bus, 1, hd1); - return 0; + return dev; } static ISADeviceInfo isa_ide_info = { diff --git a/hw/ide/piix.c b/hw/ide/piix.c index dad6e86ff6..fa22226dce 100644 --- a/hw/ide/piix.c +++ b/hw/ide/piix.c @@ -160,22 +160,24 @@ static int pci_piix4_ide_initfn(PCIDevice *dev) /* hd_table must contain 4 block drivers */ /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */ -void pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn) +PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn) { PCIDevice *dev; dev = pci_create_simple(bus, devfn, "piix3-ide"); pci_ide_create_devs(dev, hd_table); + return dev; } /* hd_table must contain 4 block drivers */ /* NOTE: for the PIIX4, the IRQs and IOports are hardcoded */ -void pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn) +PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn) { PCIDevice *dev; dev = pci_create_simple(bus, devfn, "piix4-ide"); pci_ide_create_devs(dev, hd_table); + return dev; } static PCIDeviceInfo piix_ide_info[] = { From c0897e0cb94e83ec1098867b81870e4f51f225b9 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 24 Jun 2010 19:58:20 +0200 Subject: [PATCH 20/23] pc: Fix CMOS info for drives defined with -device Drives defined with -drive if=ide get get created along with the IDE controller, inside machine->init(). That's before cmos_init(). Drives defined with -device get created during generic device init. That's after cmos_init(). Because of that, CMOS has no information on them (type, geometry, translation). Older versions of Windows such as XP reportedly choke on that. Split off the part of CMOS initialization that needs to know about -device devices, and turn it into a reset handler, so it runs after device creation. Signed-off-by: Markus Armbruster Signed-off-by: Kevin Wolf --- hw/ide.h | 2 ++ hw/ide/qdev.c | 7 ++++ hw/pc.c | 94 ++++++++++++++++++++++++++++++++------------------- hw/pc.h | 3 +- hw/pc_piix.c | 16 ++++++--- 5 files changed, 81 insertions(+), 41 deletions(-) diff --git a/hw/ide.h b/hw/ide.h index 82b3c11283..2b5ae7c39e 100644 --- a/hw/ide.h +++ b/hw/ide.h @@ -24,4 +24,6 @@ void mmio_ide_init (target_phys_addr_t membase, target_phys_addr_t membase2, qemu_irq irq, int shift, DriveInfo *hd0, DriveInfo *hd1); +void ide_get_bs(BlockDriverState *bs[], BusState *qbus); + #endif /* HW_IDE_H */ diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c index b34c473336..2977a168e5 100644 --- a/hw/ide/qdev.c +++ b/hw/ide/qdev.c @@ -88,6 +88,13 @@ IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive) return DO_UPCAST(IDEDevice, qdev, dev); } +void ide_get_bs(BlockDriverState *bs[], BusState *qbus) +{ + IDEBus *bus = DO_UPCAST(IDEBus, qbus, qbus); + bs[0] = bus->master ? bus->master->conf.bs : NULL; + bs[1] = bus->slave ? bus->slave->conf.bs : NULL; +} + /* --------------------------------- */ typedef struct IDEDrive { diff --git a/hw/pc.c b/hw/pc.c index 25ebafac80..b577fb1bc6 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -25,6 +25,7 @@ #include "pc.h" #include "apic.h" #include "fdc.h" +#include "ide.h" #include "pci.h" #include "vmware_vga.h" #include "monitor.h" @@ -275,14 +276,65 @@ static int pc_boot_set(void *opaque, const char *boot_device) return set_boot_dev(opaque, boot_device, 0); } -/* hd_table must contain 4 block drivers */ +typedef struct pc_cmos_init_late_arg { + ISADevice *rtc_state; + BusState *idebus0, *idebus1; +} pc_cmos_init_late_arg; + +static void pc_cmos_init_late(void *opaque) +{ + pc_cmos_init_late_arg *arg = opaque; + ISADevice *s = arg->rtc_state; + int val; + BlockDriverState *hd_table[4]; + int i; + + ide_get_bs(hd_table, arg->idebus0); + ide_get_bs(hd_table + 2, arg->idebus1); + + rtc_set_memory(s, 0x12, (hd_table[0] ? 0xf0 : 0) | (hd_table[1] ? 0x0f : 0)); + if (hd_table[0]) + cmos_init_hd(0x19, 0x1b, hd_table[0], s); + if (hd_table[1]) + cmos_init_hd(0x1a, 0x24, hd_table[1], s); + + val = 0; + for (i = 0; i < 4; i++) { + if (hd_table[i]) { + int cylinders, heads, sectors, translation; + /* NOTE: bdrv_get_geometry_hint() returns the physical + geometry. It is always such that: 1 <= sects <= 63, 1 + <= heads <= 16, 1 <= cylinders <= 16383. The BIOS + geometry can be different if a translation is done. */ + translation = bdrv_get_translation_hint(hd_table[i]); + if (translation == BIOS_ATA_TRANSLATION_AUTO) { + bdrv_get_geometry_hint(hd_table[i], &cylinders, &heads, §ors); + if (cylinders <= 1024 && heads <= 16 && sectors <= 63) { + /* No translation. */ + translation = 0; + } else { + /* LBA translation. */ + translation = 1; + } + } else { + translation--; + } + val |= translation << (i * 2); + } + } + rtc_set_memory(s, 0x39, val); + + qemu_unregister_reset(pc_cmos_init_late, opaque); +} + void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size, - const char *boot_device, DriveInfo **hd_table, + const char *boot_device, + BusState *idebus0, BusState *idebus1, FDCtrl *floppy_controller, ISADevice *s) { int val; int fd0, fd1, nb; - int i; + static pc_cmos_init_late_arg arg; /* various important CMOS locations needed by PC/Bochs bios */ @@ -351,38 +403,10 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size, rtc_set_memory(s, REG_EQUIPMENT_BYTE, val); /* hard drives */ - - rtc_set_memory(s, 0x12, (hd_table[0] ? 0xf0 : 0) | (hd_table[1] ? 0x0f : 0)); - if (hd_table[0]) - cmos_init_hd(0x19, 0x1b, hd_table[0]->bdrv, s); - if (hd_table[1]) - cmos_init_hd(0x1a, 0x24, hd_table[1]->bdrv, s); - - val = 0; - for (i = 0; i < 4; i++) { - if (hd_table[i]) { - int cylinders, heads, sectors, translation; - /* NOTE: bdrv_get_geometry_hint() returns the physical - geometry. It is always such that: 1 <= sects <= 63, 1 - <= heads <= 16, 1 <= cylinders <= 16383. The BIOS - geometry can be different if a translation is done. */ - translation = bdrv_get_translation_hint(hd_table[i]->bdrv); - if (translation == BIOS_ATA_TRANSLATION_AUTO) { - bdrv_get_geometry_hint(hd_table[i]->bdrv, &cylinders, &heads, §ors); - if (cylinders <= 1024 && heads <= 16 && sectors <= 63) { - /* No translation. */ - translation = 0; - } else { - /* LBA translation. */ - translation = 1; - } - } else { - translation--; - } - val |= translation << (i * 2); - } - } - rtc_set_memory(s, 0x39, val); + arg.rtc_state = s; + arg.idebus0 = idebus0; + arg.idebus1 = idebus1; + qemu_register_reset(pc_cmos_init_late, &arg); } static void handle_a20_line_change(void *opaque, int irq, int level) diff --git a/hw/pc.h b/hw/pc.h index ccfd7add99..63b0249f2f 100644 --- a/hw/pc.h +++ b/hw/pc.h @@ -104,7 +104,8 @@ void pc_init_ne2k_isa(NICInfo *nd); void pc_audio_init (PCIBus *pci_bus, qemu_irq *pic); #endif void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size, - const char *boot_device, DriveInfo **hd_table, + const char *boot_device, + BusState *ide0, BusState *ide1, FDCtrl *floppy_controller, ISADevice *s); void pc_pci_device_init(PCIBus *pci_bus); diff --git a/hw/pc_piix.c b/hw/pc_piix.c index f670fd7a78..519e8a5ccb 100644 --- a/hw/pc_piix.c +++ b/hw/pc_piix.c @@ -79,6 +79,7 @@ static void pc_init1(ram_addr_t ram_size, IsaIrqState *isa_irq_state; DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS]; FDCtrl *floppy_controller; + BusState *idebus[MAX_IDE_BUS]; ISADevice *rtc_state; pc_cpus_init(cpu_model); @@ -132,18 +133,23 @@ static void pc_init1(ram_addr_t ram_size, } if (pci_enabled) { - pci_piix3_ide_init(pci_bus, hd, piix3_devfn + 1); + PCIDevice *dev; + dev = pci_piix3_ide_init(pci_bus, hd, piix3_devfn + 1); + idebus[0] = qdev_get_child_bus(&dev->qdev, "ide.0"); + idebus[1] = qdev_get_child_bus(&dev->qdev, "ide.1"); } else { for(i = 0; i < MAX_IDE_BUS; i++) { - isa_ide_init(ide_iobase[i], ide_iobase2[i], ide_irq[i], - hd[MAX_IDE_DEVS * i], hd[MAX_IDE_DEVS * i + 1]); + ISADevice *dev; + dev = isa_ide_init(ide_iobase[i], ide_iobase2[i], ide_irq[i], + hd[MAX_IDE_DEVS * i], hd[MAX_IDE_DEVS * i + 1]); + idebus[i] = qdev_get_child_bus(&dev->qdev, "ide.0"); } } pc_audio_init(pci_enabled ? pci_bus : NULL, isa_irq); - pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device, hd, - floppy_controller, rtc_state); + pc_cmos_init(below_4g_mem_size, above_4g_mem_size, boot_device, + idebus[0], idebus[1], floppy_controller, rtc_state); if (pci_enabled && usb_enabled) { usb_uhci_piix3_init(pci_bus, piix3_devfn + 2); From c2abccecd93d5977460fdfdab19461ccfa09ae21 Mon Sep 17 00:00:00 2001 From: MORITA Kazutaka Date: Mon, 21 Jun 2010 04:26:35 +0900 Subject: [PATCH 21/23] qemu-img: avoid calling exit(1) to release resources properly This patch removes exit(1) from error(), and properly releases resources such as a block driver and an allocated memory. For testing the Sheepdog block driver with qemu-iotests, it is necessary to call bdrv_delete() before the program exits. Because the driver releases the lock of VM images in the close handler. Signed-off-by: MORITA Kazutaka Signed-off-by: Kevin Wolf --- qemu-img.c | 237 +++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 185 insertions(+), 52 deletions(-) diff --git a/qemu-img.c b/qemu-img.c index ea091f00ca..700af21841 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -39,14 +39,13 @@ typedef struct img_cmd_t { /* Default to cache=writeback as data integrity is not important for qemu-tcg. */ #define BDRV_O_FLAGS BDRV_O_CACHE_WB -static void QEMU_NORETURN error(const char *fmt, ...) +static void error(const char *fmt, ...) { va_list ap; va_start(ap, fmt); fprintf(stderr, "qemu-img: "); vfprintf(stderr, fmt, ap); fprintf(stderr, "\n"); - exit(1); va_end(ap); } @@ -197,57 +196,76 @@ static BlockDriverState *bdrv_new_open(const char *filename, char password[256]; bs = bdrv_new(""); - if (!bs) + if (!bs) { error("Not enough memory"); + goto fail; + } if (fmt) { drv = bdrv_find_format(fmt); - if (!drv) + if (!drv) { error("Unknown file format '%s'", fmt); + goto fail; + } } else { drv = NULL; } if (bdrv_open(bs, filename, flags, drv) < 0) { error("Could not open '%s'", filename); + goto fail; } if (bdrv_is_encrypted(bs)) { printf("Disk image '%s' is encrypted.\n", filename); - if (read_password(password, sizeof(password)) < 0) + if (read_password(password, sizeof(password)) < 0) { error("No password given"); - if (bdrv_set_key(bs, password) < 0) + goto fail; + } + if (bdrv_set_key(bs, password) < 0) { error("invalid password"); + goto fail; + } } return bs; +fail: + if (bs) { + bdrv_delete(bs); + } + return NULL; } -static void add_old_style_options(const char *fmt, QEMUOptionParameter *list, +static int add_old_style_options(const char *fmt, QEMUOptionParameter *list, int flags, const char *base_filename, const char *base_fmt) { if (flags & BLOCK_FLAG_ENCRYPT) { if (set_option_parameter(list, BLOCK_OPT_ENCRYPT, "on")) { error("Encryption not supported for file format '%s'", fmt); + return -1; } } if (flags & BLOCK_FLAG_COMPAT6) { if (set_option_parameter(list, BLOCK_OPT_COMPAT6, "on")) { error("VMDK version 6 not supported for file format '%s'", fmt); + return -1; } } if (base_filename) { if (set_option_parameter(list, BLOCK_OPT_BACKING_FILE, base_filename)) { error("Backing file not supported for file format '%s'", fmt); + return -1; } } if (base_fmt) { if (set_option_parameter(list, BLOCK_OPT_BACKING_FMT, base_fmt)) { error("Backing file format not supported for file format '%s'", fmt); + return -1; } } + return 0; } static int img_create(int argc, char **argv) { - int c, ret, flags; + int c, ret = 0, flags; const char *fmt = "raw"; const char *base_fmt = NULL; const char *filename; @@ -293,12 +311,16 @@ static int img_create(int argc, char **argv) /* Find driver and parse its options */ drv = bdrv_find_format(fmt); - if (!drv) + if (!drv) { error("Unknown file format '%s'", fmt); + return 1; + } proto_drv = bdrv_find_protocol(filename); - if (!proto_drv) + if (!proto_drv) { error("Unknown protocol '%s'", filename); + return 1; + } create_options = append_option_parameters(create_options, drv->create_options); @@ -307,7 +329,7 @@ static int img_create(int argc, char **argv) if (options && !strcmp(options, "?")) { print_option_help(create_options); - return 0; + goto out; } /* Create parameter list with default values */ @@ -319,6 +341,8 @@ static int img_create(int argc, char **argv) param = parse_option_parameters(options, create_options, param); if (param == NULL) { error("Invalid options for file format '%s'.", fmt); + ret = -1; + goto out; } } @@ -328,7 +352,10 @@ static int img_create(int argc, char **argv) } /* Add old-style options to parameters */ - add_old_style_options(fmt, param, flags, base_filename, base_fmt); + ret = add_old_style_options(fmt, param, flags, base_filename, base_fmt); + if (ret < 0) { + goto out; + } // The size for the image must always be specified, with one exception: // If we are using a backing file, we can obtain the size from there @@ -351,10 +378,16 @@ static int img_create(int argc, char **argv) } else { error("Unknown backing file format '%s'", backing_fmt->value.s); + ret = -1; + goto out; } } bs = bdrv_new_open(backing_file->value.s, fmt, BDRV_O_FLAGS); + if (!bs) { + ret = -1; + goto out; + } bdrv_get_geometry(bs, &size); size *= 512; bdrv_delete(bs); @@ -363,6 +396,8 @@ static int img_create(int argc, char **argv) set_option_parameter(param, BLOCK_OPT_SIZE, buf); } else { error("Image creation needs a size parameter"); + ret = -1; + goto out; } } @@ -383,6 +418,10 @@ static int img_create(int argc, char **argv) error("%s: error while creating %s: %s", filename, fmt, strerror(-ret)); } } +out: + if (ret) { + return 1; + } return 0; } @@ -411,6 +450,9 @@ static int img_check(int argc, char **argv) filename = argv[optind++]; bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS); + if (!bs) { + return 1; + } ret = bdrv_check(bs); switch(ret) { case 0: @@ -429,6 +471,9 @@ static int img_check(int argc, char **argv) } bdrv_delete(bs); + if (ret) { + return 1; + } return 0; } @@ -457,6 +502,9 @@ static int img_commit(int argc, char **argv) filename = argv[optind++]; bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR); + if (!bs) { + return 1; + } ret = bdrv_commit(bs); switch(ret) { case 0: @@ -477,6 +525,9 @@ static int img_commit(int argc, char **argv) } bdrv_delete(bs); + if (ret) { + return 1; + } return 0; } @@ -551,13 +602,13 @@ static int compare_sectors(const uint8_t *buf1, const uint8_t *buf2, int n, static int img_convert(int argc, char **argv) { - int c, ret, n, n1, bs_n, bs_i, flags, cluster_size, cluster_sectors; + int c, ret = 0, n, n1, bs_n, bs_i, flags, cluster_size, cluster_sectors; const char *fmt, *out_fmt, *out_baseimg, *out_filename; BlockDriver *drv, *proto_drv; - BlockDriverState **bs, *out_bs; + BlockDriverState **bs = NULL, *out_bs = NULL; int64_t total_sectors, nb_sectors, sector_num, bs_offset; uint64_t bs_sectors; - uint8_t * buf; + uint8_t * buf = NULL; const uint8_t *buf1; BlockDriverInfo bdi; QEMUOptionParameter *param = NULL, *create_options = NULL; @@ -604,30 +655,43 @@ static int img_convert(int argc, char **argv) out_filename = argv[argc - 1]; - if (bs_n > 1 && out_baseimg) + if (bs_n > 1 && out_baseimg) { error("-B makes no sense when concatenating multiple input images"); + return 1; + } bs = calloc(bs_n, sizeof(BlockDriverState *)); - if (!bs) + if (!bs) { error("Out of memory"); + return 1; + } total_sectors = 0; for (bs_i = 0; bs_i < bs_n; bs_i++) { bs[bs_i] = bdrv_new_open(argv[optind + bs_i], fmt, BDRV_O_FLAGS); - if (!bs[bs_i]) + if (!bs[bs_i]) { error("Could not open '%s'", argv[optind + bs_i]); + ret = -1; + goto out; + } bdrv_get_geometry(bs[bs_i], &bs_sectors); total_sectors += bs_sectors; } /* Find driver and parse its options */ drv = bdrv_find_format(out_fmt); - if (!drv) + if (!drv) { error("Unknown file format '%s'", out_fmt); + ret = -1; + goto out; + } proto_drv = bdrv_find_protocol(out_filename); - if (!proto_drv) + if (!proto_drv) { error("Unknown protocol '%s'", out_filename); + ret = -1; + goto out; + } create_options = append_option_parameters(create_options, drv->create_options); @@ -635,21 +699,25 @@ static int img_convert(int argc, char **argv) proto_drv->create_options); if (options && !strcmp(options, "?")) { print_option_help(create_options); - free(bs); - return 0; + goto out; } if (options) { param = parse_option_parameters(options, create_options, param); if (param == NULL) { error("Invalid options for file format '%s'.", out_fmt); + ret = -1; + goto out; } } else { param = parse_option_parameters("", create_options, param); } set_option_parameter_int(param, BLOCK_OPT_SIZE, total_sectors * 512); - add_old_style_options(out_fmt, param, flags, out_baseimg, NULL); + ret = add_old_style_options(out_fmt, param, flags, out_baseimg, NULL); + if (ret < 0) { + goto out; + } /* Check if compression is supported */ if (flags & BLOCK_FLAG_COMPRESS) { @@ -658,18 +726,19 @@ static int img_convert(int argc, char **argv) if (!drv->bdrv_write_compressed) { error("Compression not supported for this file format"); + ret = -1; + goto out; } if (encryption && encryption->value.n) { error("Compression and encryption not supported at the same time"); + ret = -1; + goto out; } } /* Create the new image */ ret = bdrv_create(drv, out_filename, param); - free_option_parameters(create_options); - free_option_parameters(param); - if (ret < 0) { if (ret == -ENOTSUP) { error("Formatting not supported for file format '%s'", out_fmt); @@ -678,9 +747,14 @@ static int img_convert(int argc, char **argv) } else { error("%s: error while converting %s: %s", out_filename, out_fmt, strerror(-ret)); } + goto out; } out_bs = bdrv_new_open(out_filename, out_fmt, BDRV_O_FLAGS | BDRV_O_RDWR); + if (!out_bs) { + ret = -1; + goto out; + } bs_i = 0; bs_offset = 0; @@ -688,11 +762,17 @@ static int img_convert(int argc, char **argv) buf = qemu_malloc(IO_BUF_SIZE); if (flags & BLOCK_FLAG_COMPRESS) { - if (bdrv_get_info(out_bs, &bdi) < 0) + ret = bdrv_get_info(out_bs, &bdi); + if (ret < 0) { error("could not get block driver info"); + goto out; + } cluster_size = bdi.cluster_size; - if (cluster_size <= 0 || cluster_size > IO_BUF_SIZE) + if (cluster_size <= 0 || cluster_size > IO_BUF_SIZE) { error("invalid cluster size"); + ret = -1; + goto out; + } cluster_sectors = cluster_size >> 9; sector_num = 0; for(;;) { @@ -728,8 +808,11 @@ static int img_convert(int argc, char **argv) nlow = (remainder > bs_sectors - bs_num) ? bs_sectors - bs_num : remainder; - if (bdrv_read(bs[bs_i], bs_num, buf2, nlow) < 0) + ret = bdrv_read(bs[bs_i], bs_num, buf2, nlow); + if (ret < 0) { error("error while reading"); + goto out; + } buf2 += nlow * 512; bs_num += nlow; @@ -741,10 +824,13 @@ static int img_convert(int argc, char **argv) if (n < cluster_sectors) memset(buf + n * 512, 0, cluster_size - n * 512); if (is_not_zero(buf, cluster_size)) { - if (bdrv_write_compressed(out_bs, sector_num, buf, - cluster_sectors) != 0) + ret = bdrv_write_compressed(out_bs, sector_num, buf, + cluster_sectors); + if (ret != 0) { error("error while compressing sector %" PRId64, sector_num); + goto out; + } } sector_num += n; } @@ -795,8 +881,11 @@ static int img_convert(int argc, char **argv) n1 = n; } - if (bdrv_read(bs[bs_i], sector_num - bs_offset, buf, n) < 0) + ret = bdrv_read(bs[bs_i], sector_num - bs_offset, buf, n); + if (ret < 0) { error("error while reading"); + goto out; + } /* NOTE: at the same time we convert, we do not write zero sectors to have a chance to compress the image. Ideally, we should add a specific call to have the info to go faster */ @@ -811,8 +900,11 @@ static int img_convert(int argc, char **argv) already there is garbage, not 0s. */ if (!has_zero_init || out_baseimg || is_allocated_sectors(buf1, n, &n1)) { - if (bdrv_write(out_bs, sector_num, buf1, n1) < 0) + ret = bdrv_write(out_bs, sector_num, buf1, n1); + if (ret < 0) { error("error while writing"); + goto out; + } } sector_num += n1; n -= n1; @@ -820,11 +912,22 @@ static int img_convert(int argc, char **argv) } } } +out: + free_option_parameters(create_options); + free_option_parameters(param); qemu_free(buf); - bdrv_delete(out_bs); - for (bs_i = 0; bs_i < bs_n; bs_i++) - bdrv_delete(bs[bs_i]); + if (out_bs) { + bdrv_delete(out_bs); + } + for (bs_i = 0; bs_i < bs_n; bs_i++) { + if (bs[bs_i]) { + bdrv_delete(bs[bs_i]); + } + } free(bs); + if (ret) { + return 1; + } return 0; } @@ -907,6 +1010,9 @@ static int img_info(int argc, char **argv) filename = argv[optind++]; bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING); + if (!bs) { + return 1; + } bdrv_get_format(bs, fmt_name, sizeof(fmt_name)); bdrv_get_geometry(bs, &total_sectors); get_human_readable_size(size_buf, sizeof(size_buf), total_sectors * 512); @@ -952,7 +1058,7 @@ static int img_snapshot(int argc, char **argv) BlockDriverState *bs; QEMUSnapshotInfo sn; char *filename, *snapshot_name = NULL; - int c, ret, bdrv_oflags; + int c, ret = 0, bdrv_oflags; int action = 0; qemu_timeval tv; @@ -1007,6 +1113,9 @@ static int img_snapshot(int argc, char **argv) /* Open the image */ bs = bdrv_new_open(filename, NULL, bdrv_oflags); + if (!bs) { + return 1; + } /* Perform the requested action */ switch(action) { @@ -1045,13 +1154,15 @@ static int img_snapshot(int argc, char **argv) /* Cleanup */ bdrv_delete(bs); - + if (ret) { + return 1; + } return 0; } static int img_rebase(int argc, char **argv) { - BlockDriverState *bs, *bs_old_backing, *bs_new_backing; + BlockDriverState *bs, *bs_old_backing = NULL, *bs_new_backing = NULL; BlockDriver *old_backing_drv, *new_backing_drv; char *filename; const char *fmt, *out_basefmt, *out_baseimg; @@ -1098,6 +1209,9 @@ static int img_rebase(int argc, char **argv) */ flags = BDRV_O_FLAGS | BDRV_O_RDWR | (unsafe ? BDRV_O_NO_BACKING : 0); bs = bdrv_new_open(filename, fmt, flags); + if (!bs) { + return 1; + } /* Find the right drivers for the backing files */ old_backing_drv = NULL; @@ -1107,6 +1221,8 @@ static int img_rebase(int argc, char **argv) old_backing_drv = bdrv_find_format(bs->backing_format); if (old_backing_drv == NULL) { error("Invalid format name: '%s'", bs->backing_format); + ret = -1; + goto out; } } @@ -1114,6 +1230,8 @@ static int img_rebase(int argc, char **argv) new_backing_drv = bdrv_find_format(out_basefmt); if (new_backing_drv == NULL) { error("Invalid format name: '%s'", out_basefmt); + ret = -1; + goto out; } } @@ -1127,19 +1245,19 @@ static int img_rebase(int argc, char **argv) bs_old_backing = bdrv_new("old_backing"); bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name)); - if (bdrv_open(bs_old_backing, backing_name, BDRV_O_FLAGS, - old_backing_drv)) - { + ret = bdrv_open(bs_old_backing, backing_name, BDRV_O_FLAGS, + old_backing_drv); + if (ret) { error("Could not open old backing file '%s'", backing_name); - return -1; + goto out; } bs_new_backing = bdrv_new("new_backing"); - if (bdrv_open(bs_new_backing, out_baseimg, BDRV_O_FLAGS | BDRV_O_RDWR, - new_backing_drv)) - { + ret = bdrv_open(bs_new_backing, out_baseimg, BDRV_O_FLAGS | BDRV_O_RDWR, + new_backing_drv); + if (ret) { error("Could not open new backing file '%s'", out_baseimg); - return -1; + goto out; } } @@ -1180,11 +1298,15 @@ static int img_rebase(int argc, char **argv) } /* Read old and new backing file */ - if (bdrv_read(bs_old_backing, sector, buf_old, n) < 0) { + ret = bdrv_read(bs_old_backing, sector, buf_old, n); + if (ret < 0) { error("error while reading from old backing file"); + goto out; } - if (bdrv_read(bs_new_backing, sector, buf_new, n) < 0) { + ret = bdrv_read(bs_new_backing, sector, buf_new, n); + if (ret < 0) { error("error while reading from new backing file"); + goto out; } /* If they differ, we need to write to the COW file */ @@ -1201,6 +1323,7 @@ static int img_rebase(int argc, char **argv) if (ret < 0) { error("Error while writing to COW image: %s", strerror(-ret)); + goto out; } } @@ -1232,7 +1355,7 @@ static int img_rebase(int argc, char **argv) * could be dropped from the COW file. Don't do this before switching the * backing file, in case of a crash this would lead to corruption. */ - +out: /* Cleanup */ if (!unsafe) { bdrv_delete(bs_old_backing); @@ -1240,7 +1363,9 @@ static int img_rebase(int argc, char **argv) } bdrv_delete(bs); - + if (ret) { + return 1; + } return 0; } @@ -1306,6 +1431,9 @@ static int img_resize(int argc, char **argv) free_option_parameters(param); bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_RDWR); + if (!bs) { + return 1; + } if (relative) { total_size = bdrv_getlength(bs) + n * relative; @@ -1314,6 +1442,8 @@ static int img_resize(int argc, char **argv) } if (total_size <= 0) { error("New image size must be positive"); + ret = -1; + goto out; } ret = bdrv_truncate(bs, total_size); @@ -1331,8 +1461,11 @@ static int img_resize(int argc, char **argv) error("Error resizing image (%d)", -ret); break; } - +out: bdrv_delete(bs); + if (ret) { + return 1; + } return 0; } From 453f9a1652629e5805995b165be2e634c8487139 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 2 Jul 2010 14:01:21 +0200 Subject: [PATCH 22/23] block: Fix early failure in multiwrite bdrv_aio_writev may call the callback immediately (and it will commonly do so in error cases). Current code doesn't consider this. For details see the comment added by this patch. Signed-off-by: Kevin Wolf --- block.c | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/block.c b/block.c index 9176decf33..e65971c91b 100644 --- a/block.c +++ b/block.c @@ -2183,8 +2183,29 @@ int bdrv_aio_multiwrite(BlockDriverState *bs, BlockRequest *reqs, int num_reqs) // Check for mergable requests num_reqs = multiwrite_merge(bs, reqs, num_reqs, mcb); - // Run the aio requests + /* + * Run the aio requests. As soon as one request can't be submitted + * successfully, fail all requests that are not yet submitted (we must + * return failure for all requests anyway) + * + * num_requests cannot be set to the right value immediately: If + * bdrv_aio_writev fails for some request, num_requests would be too high + * and therefore multiwrite_cb() would never recognize the multiwrite + * request as completed. We also cannot use the loop variable i to set it + * when the first request fails because the callback may already have been + * called for previously submitted requests. Thus, num_requests must be + * incremented for each request that is submitted. + * + * The problem that callbacks may be called early also means that we need + * to take care that num_requests doesn't become 0 before all requests are + * submitted - multiwrite_cb() would consider the multiwrite request + * completed. A dummy request that is "completed" by a manual call to + * multiwrite_cb() takes care of this. + */ + mcb->num_requests = 1; + for (i = 0; i < num_reqs; i++) { + mcb->num_requests++; acb = bdrv_aio_writev(bs, reqs[i].sector, reqs[i].qiov, reqs[i].nb_sectors, multiwrite_cb, mcb); @@ -2192,22 +2213,24 @@ int bdrv_aio_multiwrite(BlockDriverState *bs, BlockRequest *reqs, int num_reqs) // We can only fail the whole thing if no request has been // submitted yet. Otherwise we'll wait for the submitted AIOs to // complete and report the error in the callback. - if (mcb->num_requests == 0) { - reqs[i].error = -EIO; + if (i == 0) { goto fail; } else { - mcb->num_requests++; multiwrite_cb(mcb, -EIO); break; } - } else { - mcb->num_requests++; } } + /* Complete the dummy request */ + multiwrite_cb(mcb, 0); + return 0; fail: + for (i = 0; i < mcb->num_callbacks; i++) { + reqs[i].error = -EIO; + } qemu_free(mcb); return -1; } From de189a1b4a471d37a2909e97646654fc9751b52f Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Thu, 1 Jul 2010 16:08:51 +0200 Subject: [PATCH 23/23] block: Handle multiwrite errors only when all requests have completed Don't try to be clever by freeing all temporary data and calling all callbacks when the return value (an error) is certain. Doing so has at least two important problems: * The temporary data that is freed (qiov, possibly zero buffer) is still used by the requests that have not yet completed. * Calling the callbacks for all requests in the multiwrite means for the caller that it may free buffers etc. which are still in use. Just remember the error value and do the cleanup when all requests have completed. Signed-off-by: Kevin Wolf --- block.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/block.c b/block.c index e65971c91b..dd6dd76c6d 100644 --- a/block.c +++ b/block.c @@ -2042,14 +2042,11 @@ static void multiwrite_cb(void *opaque, int ret) if (ret < 0 && !mcb->error) { mcb->error = ret; - multiwrite_user_cb(mcb); } mcb->num_requests--; if (mcb->num_requests == 0) { - if (mcb->error == 0) { - multiwrite_user_cb(mcb); - } + multiwrite_user_cb(mcb); qemu_free(mcb); } }