More block-related fixes and cleanups
cover More block-related fixes and cleanups
I'm working on cleanly separating block device host and guest parts. I'd like to route all this work through Kevin's block tree. This is still just preliminaries.
There will be at least one more round of cleanup & fixes before blockdev_add proper. I intend to start with a minimal QMP-only version, then add features.
Markus Armbruster (12):
scsi: scsi_bus_legacy_handle_cmdline() can fail, fix callers
ide: Make it explicit that ide_create_drive() can't fail
blockdev: Remove drive_get_serial()
blockdev: New drive_of_blockdev()
blockdev: Clean up automatic drive deletion
qdev: Decouple qdev_prop_drive from DriveInfo
blockdev: drive_get_by_id() is no longer used, remove
block: Catch attempt to attach multiple devices to a blockdev
savevm: Survive hot-unplug of snapshot device
block: Fix virtual media change for if=none
ide: Make PIIX and ISA IDE init functions return the qdev
pc: Fix CMOS info for drives defined with -device
block.c | 55 +++++++++++++++++++++++++++++ block.h | 5 +++ block_int.h | 8 ++-- blockdev.c | 45 +++++++++++++++--------- blockdev.h | 7 +++- hw/esp.c | 3 +- hw/fdc.c | 32 +++++++++------- hw/ide.h | 13 ++++--- hw/ide/core.c | 18 +++++---- hw/ide/internal.h | 2 +- hw/ide/isa.c | 8 ++-- hw/ide/piix.c | 6 ++- hw/ide/qdev.c | 22 ++++++++--- hw/lsi53c895a.c | 2 +- hw/pc.c | 94 +++++++++++++++++++++++++++++++------------------ hw/pc.h | 3 +- hw/pc_piix.c | 16 ++++++--- hw/pci-hotplug.c | 10 ++++- hw/qdev-properties.c | 46 +++++++++++++++++++++---- hw/qdev.h | 7 ++-- hw/s390-virtio.c | 2 +- hw/scsi-bus.c | 19 ++++++---- hw/scsi-disk.c | 21 ++++++----- hw/scsi-generic.c | 7 ++-- hw/scsi.h | 4 +- hw/usb-msd.c | 29 ++++++++++++--- hw/virtio-blk.c | 3 +- hw/virtio-pci.c | 4 +- savevm.c | 31 ++-------------- 29 files changed, 344 insertions(+), 178 deletions(-)
PATCH 1/12 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().
--- 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(-)
hw/esp.c +1 -2
@@ -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 = {
hw/lsi53c895a.c +1 -1
@@ -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 ; }
hw/pci-hotplug.c +3 -0
@@ -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)
hw/scsi-bus.c +7 -4
@@ -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)
hw/scsi.h +1 -1
@@ -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);
hw/usb-msd.c +3 -0
@@ -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);
Signature · 1 line
1.6.6.1
[Qemu-devel] Re: [PATCH 01/12] scsi: scsi_bus_legacy_handle_cmdline() can fail, fix callers
Quoted 8 lines · Markus Armbruster
None of its callers checks for failure.
scsi_hot_add()can crash because of that:(qemu)
drive_add4 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().
Looks good,
PATCH 2/12 ide: Make it explicit that ide_create_drive() can't fail
In reply to Markus ArmbrusterAll 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.
--- hw/ide/qdev.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)
hw/ide/qdev.c +1 -2
@@ -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); }
Signature · 1 line
1.6.6.1
[Qemu-devel] Re: [PATCH 02/12] ide: Make it explicit that ide_create_drive() can't fail
Quoted 7 lines · Markus Armbruster
All callers of
ide_create_drive()ignore its value. Currently harmless, because it fails only whenqdev_init()fails, which fails only whenide_drive_initfn()fails, which never fails.Brittle. Change it to die instead of silently ignoring failure.
Signed-off-by Markus Armbruster <[email protected]>
Looks good,
PATCH 3/12 blockdev: Remove drive_get_serial()
In reply to Markus ArmbrusterUnused since commit 6ced55a5.
--- blockdev.c | 12 ------------ blockdev.h | 1 - 2 files changed, 0 insertions(+), 13 deletions(-)
blockdev.c +0 -12
@@ -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);
blockdev.h +0 -1
@@ -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,
Signature · 1 line
1.6.6.1
[Qemu-devel] Re: [PATCH 03/12] blockdev: Remove drive_get_serial()
Quoted 3 lines · Markus Armbruster
Unused since commit
6ced55a5.Signed-off-by Markus Armbruster <[email protected]>
Looks good,
PATCH 4/12 blockdev: New drive_of_blockdev()
In reply to Markus Armbruster--- blockdev.c | 12 ++++++++++++ blockdev.h | 1 + 2 files changed, 13 insertions(+), 0 deletions(-)
blockdev.c +12 -0
@@ -78,6 +78,18 @@ int drive_get_max_bus(BlockInterfaceType type)return max_bus; } +DriveInfo *drive_of_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);
blockdev.h +1 -0
@@ -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_of_blockdev (BlockDriverState *bs);extern QemuOpts *drive_add (const char *file,const char *fmt, ...); extern DriveInfo *drive_init(QemuOpts *arg, int default_to_scsi,
Signature · 1 line
1.6.6.1
[Qemu-devel] Re: [PATCH 04/12] blockdev: New drive_of_blockdev()
Quoted 1 line
+DriveInfo *drive_of_blockdev(BlockDriverState *bs)
I'd call this find_drive_by_blockdev.
[Qemu-devel] Re: [PATCH 04/12] blockdev: New drive_of_blockdev()
Quoted 3 lines · Christoph Hellwig
Quoted 1 line
+DriveInfo *drive_of_blockdev(BlockDriverState *bs)
I'd call this
find_drive_by_blockdev.
For what it's worth, all externally visible functions dealing with drives start with "drive_".
[Qemu-devel] Re: [PATCH 04/12] blockdev: New drive_of_blockdev()
Quoted 8 lines · Markus Armbruster
Quoted 3 lines · Christoph Hellwig
Quoted 1 line
+DriveInfo *drive_of_blockdev(BlockDriverState *bs)
I'd call this
find_drive_by_blockdev.For what it's worth, all externally visible functions dealing with drives start with "
drive_".
Ok. How about drive_get_by_blockdev, matching the drive_get_by_id we have around?
Re: [Qemu-devel] Re: [PATCH 04/12] blockdev: New drive_of_blockdev()
Quoted 5 lines · Christoph Hellwig
Quoted 4 lines · Markus Armbruster
Quoted 2 lines · Christoph Hellwig
Quoted 1 line
+DriveInfo *drive_of_blockdev(BlockDriverState *bs)
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
I'd call this
find_drive_by_blockdev.For what it's worth, all externally visible functions dealing with drives start with "
drive_".Ok. How about
drive_get_by_blockdev, matching thedrive_get_by_idwe have around?
drive_get_by_id() gets removed by 07/12. But there's still drive_get().
drive_get_by_blockdev()? blockdev_to_drive()?
Re: [Qemu-devel] Re: [PATCH 04/12] blockdev: New drive_of_blockdev()
Quoted 3 lines · Markus Armbruster
drive_get_by_id()gets removed by 07/12. But there's stilldrive_get().
drive_get_by_blockdev()?blockdev_to_drive()?
Oh well, just keep any name - it's not that important aftet all.
[Qemu-devel] Re: [PATCH 04/12] blockdev: New drive_of_blockdev()
In reply to Markus ArmbrusterQuoted 1 line · Markus Armbruster
drive_get_by_blockdev()?blockdev_to_drive()?
I like drive_find_by_blockdev or drive_get_by_blockdev, in any case I'll make sure my own cpu_get_by_id is named in the same style that you choose.
Paolo
[Qemu-devel] Re: [PATCH 04/12] blockdev: New drive_of_blockdev()
Quoted 6 lines · Paolo Bonzini
Quoted 1 line · Markus Armbruster
drive_get_by_blockdev()?blockdev_to_drive()?I like
drive_find_by_blockdevordrive_get_by_blockdev, in any case I'll make sure my owncpu_get_by_idis named in the same style that you choose.
drive_get_by_blockdev() it is.
As long as this is the only change, I don't think another post to the list is necessary. Instead, I'll push to my tree and let Kevin pick it up from there.
PATCH 5/12 blockdev: Clean up automatic drive deletion
In reply to Markus ArmbrusterWe 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.
--- 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(-)
blockdev.c +23 -0
@@ -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_of_blockdev (bs); + + dinfo->auto_del =1 ; +} + +void blockdev_auto_del (BlockDriverState *bs) +{ +DriveInfo *dinfo =drive_of_blockdev (bs); + +if (dinfo->auto_del) { +drive_uninit (dinfo); + } +} +QemuOpts *drive_add (const char *file,const char *fmt, ...) {va_list ap;
blockdev.h +4 -0
@@ -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;
hw/qdev-properties.c +10 -0
@@ -293,6 +293,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);
@@ -305,6 +314,7 @@ PropertyInfo qdev_prop_drive = { .size = sizeof(DriveInfo*), .parse = parse_drive, .print = print_drive, + .free = free_drive, };/* --- character device --- */
hw/scsi-disk.c +1 -1
@@ -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)
hw/scsi-generic.c +1 -1
@@ -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)
hw/usb-msd.c +16 -4
@@ -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 {
hw/virtio-pci.c +1 -1
@@ -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); }
Signature · 1 line
1.6.6.1
[Qemu-devel] Re: [PATCH 05/12] blockdev: Clean up automatic drive deletion
Looks correct to me,
PATCH 6/12 qdev: Decouple qdev_prop_drive from DriveInfo
In reply to Markus ArmbrusterMake 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_of_blockdev() there.
Device model code should now use DriveInfo only when explicitly dealing with drives defined the old way, i.e. without -device.
--- 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(-)
block_int.h +2 -4
@@ -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, \
hw/fdc.c +10 -12
@@ -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 (), }, };
hw/ide/core.c +9 -8
@@ -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]); }
hw/ide/internal.h +1 -1
@@ -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,
hw/ide/qdev.c +8 -4
@@ -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_of_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);
hw/pci-hotplug.c +2 -2
@@ -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 ;
hw/qdev-properties.c +12 -9
@@ -285,33 +285,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 :"<null>" ); +BlockDriverState **ptr =qdev_get_prop_ptr (dev, prop); +return snprintf (dest, len,"%s" , + *ptr ?bdrv_get_device_name (*ptr) :"<null>" ); }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,
@@ -637,7 +640,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 ); }
hw/qdev.h +3 -3
@@ -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);
hw/s390-virtio.c +1 -1
@@ -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); } }
hw/scsi-bus.c +4 -4
@@ -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 ; }
hw/scsi-disk.c +7 -9
@@ -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_of_blockdev (s->bs); + s->serial =qemu_strdup (*dinfo->serial ? dinfo->serial :"0" ); }if (!s->version) {
hw/scsi-generic.c +3 -3
@@ -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)) {
hw/scsi.h +1 -1
@@ -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);
hw/usb-msd.c +7 -8
@@ -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 ;
hw/virtio-blk.c +1 -1
@@ -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 ;
hw/virtio-pci.c +2 -2
@@ -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); }
Signature · 1 line
1.6.6.1
[Qemu-devel] Re: [PATCH 06/12] qdev: Decouple qdev_prop_drive from DriveInfo
Quoted 12 lines · Markus Armbruster
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.
Quoted 7 lines
Fortunately, the device models are perfectly happy with BlockDriverState, except for two places:
ide_drive_initfn()andscsi_disk_initfn()need to check the DriveInfo for a serial number set with legacy -drive serial=... Usedrive_of_blockdev()there.Device model code should now use DriveInfo only when explicitly dealing with drives defined the old way, i.e. without -device.
This looks good to me. Assuming the size change of the property can't cause any problems:
PATCH 7/12 blockdev: drive_get_by_id() is no longer used, remove
In reply to Markus Armbruster--- blockdev.c | 12 ------------ blockdev.h | 1 - 2 files changed, 0 insertions(+), 13 deletions(-)
blockdev.c +0 -12
@@ -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;
blockdev.h +0 -1
@@ -41,7 +41,6 @@ typedef struct DriveInfo {#define MAX_SCSI_DEVS 7extern 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_of_blockdev (BlockDriverState *bs);
Signature · 1 line
1.6.6.1
[Qemu-devel] Re: [PATCH 07/12] blockdev: drive_get_by_id() is no longer used, remove
Looks good,
PATCH 8/12 block: Catch attempt to attach multiple devices to a blockdev
In reply to Markus ArmbrusterFor 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.
--- block.c | 22 ++++++++++++++++++++++ block.h | 3 +++ block_int.h | 2 ++ hw/fdc.c | 10 +++++----- hw/ide/qdev.c | 2 +- hw/pci-hotplug.c | 5 ++++- hw/qdev-properties.c | 21 ++++++++++++++++++++- hw/qdev.h | 3 ++- hw/s390-virtio.c | 2 +- hw/scsi-bus.c | 4 +++- hw/usb-msd.c | 11 +++++++---- 11 files changed, 70 insertions(+), 15 deletions(-)
block.c +22 -0
@@ -659,6 +659,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);
@@ -672,6 +674,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 *
block.h +3 -0
@@ -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);
block_int.h +2 -0
@@ -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 */
hw/fdc.c +5 -5
@@ -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);
hw/ide/qdev.c +1 -1
@@ -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); }
hw/pci-hotplug.c +4 -1
@@ -214,7 +214,10 @@ 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 ) { + dev =NULL ; +break ; + }if (qdev_init (&dev->qdev) <0 ) dev =NULL ;break ;
hw/qdev-properties.c +20 -1
@@ -291,6 +291,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 ; }
@@ -300,6 +302,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); } }
@@ -640,11 +643,27 @@ 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; +qdev_prop_set (dev, name, &value,PROP_TYPE_DRIVE ); + 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 res; } +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 );
hw/qdev.h +2 -1
@@ -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);
hw/s390-virtio.c +1 -1
@@ -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); } }
hw/scsi-bus.c +3 -1
@@ -91,7 +91,9 @@ 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 ) { +return NULL ; + }if (qdev_init (dev) <0 )return NULL ;return DO_UPCAST (SCSIDevice, qdev, dev);
hw/usb-msd.c +7 -4
@@ -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,9 @@ 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 ) { +return NULL ; + }if (qdev_init (&dev->qdev) <0 )return NULL ;
Signature · 1 line
1.6.6.1
[Qemu-devel] Re: [PATCH 08/12] block: Catch attempt to attach multiple devices to a blockdev
Quoted 3 lines · Markus Armbruster
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.
And from some quick testing a while ago the thing seems to actually work. Not that I think that it is a good idea, but do we want to change behaviour in that respect?
Quoted 3 lines
Device usb-storage deliberately attaches twice to the same blockdev, which fails with the fix in place. Detach before the second attach there.
Can anyone explain what the hell usb storage is actually trying to do with the two drives?
Re: [Qemu-devel] Re: [PATCH 08/12] block: Catch attempt to attach multiple devices to a blockdev
Quoted 8 lines · Christoph Hellwig
Quoted 3 lines · Markus Armbruster
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.
And from some quick testing a while ago the thing seems to actually work. Not that I think that it is a good idea, but do we want to change behaviour in that respect?
Valid question. I'd answer yes. It's an easy error to make, and likely to end in massive file system corruption in the guest.
Quoted 6 lines
Quoted 3 lines
Device usb-storage deliberately attaches twice to the same blockdev, which fails with the fix in place. Detach before the second attach there.
Can anyone explain what the hell usb storage is actually trying to do with the two drives?
It's actually a SCSI controller with a single drive on its single bus.
-device usb-storage,drive=foo creates *two* devices: usb-storage itself, which serves as SCSI controller, and scsi-disk for the drive. usb-storage copies its drive property to scsi-disk.
I don't like this. Each -device should create just one device.
Re: [Qemu-devel] Re: [PATCH 08/12] block: Catch attempt to attach multiple devices to a blockdev
Quoted 2 lines · Markus Armbruster
Valid question. I'd answer yes. It's an easy error to make, and likely to end in massive file system corruption in the guest.
I suspect a modern distro in the guest will detect it as a multi-path setup.
Quoted 10 lines
Quoted 2 lines
Can anyone explain what the hell usb storage is actually trying to do with the two drives?
It's actually a SCSI controller with a single drive on its single bus.
-device usb-storage,drive=foo creates *two* devices: usb-storage itself, which serves as SCSI controller, and scsi-disk for the drive. usb-storage copies its drive property to scsi-disk.
I don't like this. Each -device should create just one device.
Indeed. I'd also prefer to get rid of this. Anthony, how hard are the rules on backwards compatiblity for things like this?
Note that currently the usb storage emulation is extremly broken anyway, just writing to it produces I/O errors after a short while. This means it can't be used very much at all.
Re: [Qemu-devel] Re: [PATCH 08/12] block: Catch attempt to attach multiple devices to a blockdev
Am 27.06.2010 11:36, schrieb Christoph Hellwig:
Quoted 11 lines · Markus Armbruster
Quoted 2 lines
Valid question. I'd answer yes. It's an easy error to make, and likely to end in massive file system corruption in the guest.
I suspect a modern distro in the guest will detect it as a multi-path setup.
Quoted 5 lines
Quoted 2 lines
Can anyone explain what the hell usb storage is actually trying to do with the two drives?
It's actually a SCSI controller with a single drive on its single bus.
Quoted 8 lines
Quoted 5 lines
-device usb-storage,drive=foo creates *two* devices: usb-storage itself, which serves as SCSI controller, and scsi-disk for the drive. usb-storage copies its drive property to scsi-disk.
I don't like this. Each -device should create just one device.
Indeed. I'd also prefer to get rid of this. Anthony, how hard are the rules on backwards compatiblity for things like this?
How would breaking compatibility help us? For the user a USB MSD is only one device, so requiring two -device parameters sounds wrong.
If anything, scsi-disk must change to be able to share code without requiring a device - and this is a change that would be kept internal with no change on the user interface.
Or are you just talking about things like migration between versions which would very likely break? That would probably be okay for USB MSD.
Quoted 3 lines
Note that currently the usb storage emulation is extremly broken anyway, just writing to it produces I/O errors after a short while. This means it can't be used very much at all.
When I tried last time, it did produce lots of kernel error messages in the guest, but in the end the data was written correctly. So it doesn't seem to be completely unusable.
Kevin
Re: [Qemu-devel] Re: [PATCH 08/12] block: Catch attempt to attach multiple devices to a blockdev
Quoted 2 lines · Kevin Wolf
How would breaking compatibility help us? For the user a USB MSD is only one device, so requiring two -device parameters sounds wrong.
But it is separate devices. At least the standards compliant usb storage devices just are a bride of scsi commands over usb and fit into the SAM device model, which makes a difference between initiator, target and LUN. So having a different device for the specific target vs the initiator port makes a difference. (and yes, we're still totally missing support for multiple luns, which would require another level of devices). Trying to hide this is not all that useful - not anymore useful than hiding it on a "normal" scsi host controller anyway.
Re: [Qemu-devel] Re: [PATCH 08/12] block: Catch attempt to attach multiple devices to a blockdev
Am 28.06.2010 12:16, schrieb Christoph Hellwig:
Quoted 12 lines · Kevin Wolf
Quoted 2 lines
How would breaking compatibility help us? For the user a USB MSD is only one device, so requiring two -device parameters sounds wrong.
But it is separate devices. At least the standards compliant usb storage devices just are a bride of scsi commands over usb and fit into the SAM device model, which makes a difference between initiator, target and LUN. So having a different device for the specific target vs the initiator port makes a difference. (and yes, we're still totally missing support for multiple luns, which would require another level of devices). Trying to hide this is not all that useful - not anymore useful than hiding it on a "normal" scsi host controller anyway.
Maybe we need something like composed devices? So when the user asks for a USB stick, he actually gets all devices that this stick internally uses? Otherwise it becomes really hard to use -device directly.
I guess the same applies for mainboards, CPUs and probably some more things, though I don't really know how these are (planned to be) done in qdev.
Kevin
Re: [Qemu-devel] Re: [PATCH 08/12] block: Catch attempt to attach multiple devices to a blockdev
Quoted 5 lines · Kevin Wolf
Am 28.06.2010 12:16, schrieb Christoph Hellwig:
Quoted 4 lines · Kevin Wolf
Quoted 3 lines
Am 27.06.2010 11:36, schrieb Christoph Hellwig:
Quoted 1 line · Markus Armbruster
[...]
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 8 lines
Quoted 5 lines
-device usb-storage,drive=foo creates *two* devices: usb-storage itself, which serves as SCSI controller, and scsi-disk for the drive. usb-storage copies its drive property to scsi-disk.
I don't like this. Each -device should create just one device.
Indeed. I'd also prefer to get rid of this. Anthony, how hard are the rules on backwards compatiblity for things like this?
How would breaking compatibility help us? For the user a USB MSD is only one device, so requiring two -device parameters sounds wrong.
-device designed to be simple, stupid and straightforward: you get exactly what you asked for, no more, no less. usb-storage breaks this design maxim.
Quoted 9 lines
Quoted 8 lines
But it is separate devices. At least the standards compliant usb storage devices just are a bride of scsi commands over usb and fit into the SAM device model, which makes a difference between initiator, target and LUN. So having a different device for the specific target vs the initiator port makes a difference. (and yes, we're still totally missing support for multiple luns, which would require another level of devices). Trying to hide this is not all that useful - not anymore useful than hiding it on a "normal" scsi host controller anyway.
Quoted 3 lines
Maybe we need something like composed devices? So when the user asks for a USB stick, he actually gets all devices that this stick internally uses? Otherwise it becomes really hard to use -device directly.
Could be useful.
Quoted 3 lines
I guess the same applies for mainboards, CPUs and probably some more things, though I don't really know how these are (planned to be) done in qdev.
I'd like to keep -device stupid. If we need "smarter" controls, let's layer them on top.
Re: [Qemu-devel] Re: [PATCH 08/12] block: Catch attempt to attach multiple devices to a blockdev
Am 30.06.2010 13:52, schrieb Markus Armbruster:
Quoted 17 lines
Quoted 5 lines · Kevin Wolf
Am 28.06.2010 12:16, schrieb Christoph Hellwig:
Quoted 4 lines · Kevin Wolf
Quoted 3 lines
Am 27.06.2010 11:36, schrieb Christoph Hellwig:
Quoted 1 line · Markus Armbruster
[...]
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 5 lines
-device usb-storage,drive=foo creates *two* devices: usb-storage itself, which serves as SCSI controller, and scsi-disk for the drive. usb-storage copies its drive property to scsi-disk.
I don't like this. Each -device should create just one device.
Indeed. I'd also prefer to get rid of this. Anthony, how hard are the rules on backwards compatiblity for things like this?
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
How would breaking compatibility help us? For the user a USB MSD is only one device, so requiring two -device parameters sounds wrong.
-device designed to be simple, stupid and straightforward: you get exactly what you asked for, no more, no less. usb-storage breaks this design maxim.
I suppose the real question is: What is a device? qemu's internal view (dozens of devices that communicate with each other) and the user's view (it's one USB stick/mainboard/...) may differ.
Quoted 4 lines
Quoted 3 lines
Maybe we need something like composed devices? So when the user asks for a USB stick, he actually gets all devices that this stick internally uses? Otherwise it becomes really hard to use -device directly.
Quoted 8 lines
Could be useful.
Quoted 3 lines
I guess the same applies for mainboards, CPUs and probably some more things, though I don't really know how these are (planned to be) done in qdev.
I'd like to keep -device stupid. If we need "smarter" controls, let's layer them on top.
Makes sense. I'm not opposed to having something that deals only with "atomic devices" or whatever you want to call them.
What users will usually want to have is something that treats an USB stick as one device, though. So maybe the stupid version should become -qdev or something and the extended version that is meant for users should get the easy-to-remember name -device.
Kevin
Re: [Qemu-devel] Re: [PATCH 08/12] block: Catch attempt to attach multiple devices to a blockdev
In reply to Kevin WolfHi,
Quoted 7 lines
Quoted 3 lines
Note that currently the usb storage emulation is extremly broken anyway, just writing to it produces I/O errors after a short while. This means it can't be used very much at all.
When I tried last time, it did produce lots of kernel error messages in the guest, but in the end the data was written correctly. So it doesn't seem to be completely unusable.
I think it is actually the linux kernel being at fault here. I've seen I/O errors on real hardware too, on my pretty old laptop which has usb 1.1 only. Probably something like timeouts being *way* too low for usb 1.1 xfer speeds.
cheers,
Gerd
Re: [Qemu-devel] Re: [PATCH 08/12] block: Catch attempt to attach multiple devices to a blockdev
In reply to Christoph HellwigQuoted 9 lines · Christoph Hellwig
Quoted 7 lines · Markus Armbruster
Quoted 5 lines · Christoph Hellwig
Quoted 3 lines · Markus Armbruster
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.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
And from some quick testing a while ago the thing seems to actually work. Not that I think that it is a good idea, but do we want to change behaviour in that respect?
Valid question. I'd answer yes. It's an easy error to make, and likely to end in massive file system corruption in the guest.
I suspect a modern distro in the guest will detect it as a multi-path setup.
Really? The guest sees two disks, different serial numbers, possibly on different buses (one could be SCSI, the other iDE).
[...]
[Qemu-devel] Re: [PATCH 08/12] block: Catch attempt to attach multiple devices to a blockdev
In reply to Markus ArmbrusterAm 25.06.2010 18:53, schrieb Markus Armbruster:
Quoted 149 lines
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 <[email protected]>--- block.c | 22 ++++++++++++++++++++++ block.h | 3 +++ block_int.h | 2 ++ hw/fdc.c | 10 +++++----- hw/ide/qdev.c | 2 +- hw/pci-hotplug.c | 5 ++++- hw/qdev-properties.c | 21 ++++++++++++++++++++- hw/qdev.h | 3 ++- hw/s390-virtio.c | 2 +- hw/scsi-bus.c | 4 +++- hw/usb-msd.c | 11 +++++++---- 11 files changed, 70 insertions(+), 15 deletions(-)block.c +22
-0@@ -659,6 +659,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);@@ -672,6 +674,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 * block.h +3
-0@@ -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);block_int.h +2
-0@@ -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 */ hw/fdc.c +5
-5@@ -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);hw/ide/qdev.c +1
-1@@ -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); }hw/pci-hotplug.c +0
-0
hw/pci-hotplug.c static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon, · line 220
Quoted 8 lines
hw/pci-hotplug.c +4
-1@@ -214,7 +214,10 @@ 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 ) { + dev =NULL ; +break ; + }
I think in error cases we'll leak dev.
Quoted 85 lines
hw/pci-hotplug.c +0
-0if (qdev_init (&dev->qdev) <0 ) dev =NULL ;break ;hw/qdev-properties.c +20
-1@@ -291,6 +291,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 ; }@@ -300,6 +302,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); } }@@ -640,11 +643,27 @@ 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; +qdev_prop_set (dev, name, &value,PROP_TYPE_DRIVE ); + 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 res; } +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 );hw/qdev.h +2
-1@@ -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);hw/s390-virtio.c +1
-1@@ -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); } }hw/scsi-bus.c +0
-0
hw/scsi-bus.c SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, int · line 96
Quoted 7 lines
hw/scsi-bus.c +3
-1@@ -91,7 +91,9 @@ 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 ) { +return NULL ; + }
As we do here.
Quoted 25 lines
hw/scsi-bus.c +0
-0if (qdev_init (dev) <0 )return NULL ;return DO_UPCAST (SCSIDevice, qdev, dev);hw/usb-msd.c +4
-3@@ -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 ;
hw/usb-msd.c static USBDevice *usb_msd_init(const char *filename) · line 617
Quoted 9 lines
hw/usb-msd.c +3
-1@@ -609,7 +610,9 @@ 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 ) { +return NULL ; + }if (qdev_init (&dev->qdev) <0 )return NULL ;
And here.
Kevin
[Qemu-devel] Re: [PATCH 08/12] block: Catch attempt to attach multiple devices to a blockdev
Quoted 10 lines · Kevin Wolf
Am 25.06.2010 18:53, schrieb Markus Armbruster:
Quoted 9 lines
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.
[...]
Quoted 7 lines
Quoted 7 lines
hw/pci-hotplug.c +0
-0@@ -214,7 +214,10 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon,return NULL ; }
Quoted 8 lines
hw/pci-hotplug.c static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon, · line 220
Quoted 6 lines
hw/pci-hotplug.c +4
-1dev =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 ) { + dev =NULL ; +break ; + }I think in error cases we'll leak dev.
Correct.
Quoted 3 lines
Quoted 3 lines
hw/pci-hotplug.c +0
-0if (qdev_init (&dev->qdev) <0 ) dev =NULL ;break ;
[...]
Quoted 6 lines
Quoted 6 lines
hw/scsi-bus.c +0
-0@@ -91,7 +91,9 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, int driver =bdrv_is_sg (bdrv) ?"scsi-generic" :"scsi-disk" ;
Quoted 8 lines
hw/scsi-bus.c SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, int · line 96
Quoted 6 lines
hw/scsi-bus.c +3
-1dev =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 ) { +return NULL ; + }As we do here.
Yes.
Quoted 23 lines
Quoted 23 lines
hw/scsi-bus.c +0
-0if (qdev_init (dev) <0 )return NULL ;return DO_UPCAST (SCSIDevice, qdev, dev);hw/usb-msd.c +4
-3@@ -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 ;
Quoted 13 lines
hw/usb-msd.c static USBDevice *usb_msd_init(const char *filename) · line 617
Quoted 11 lines
hw/usb-msd.c +3
-1s->dev.speed =USB_SPEED_FULL ;@@ -609,7 +610,9 @@ 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 ) { +return NULL ; + }if (qdev_init (&dev->qdev) <0 )return NULL ;And here.
Yes.
I double-checked the entire patch series, and could not find further leaks.
Thanks a lot!
PATCH v2 8/12 block: Catch attempt to attach multiple devices to a blockdev
In reply to Markus ArmbrusterFor 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.
---
v2: plug leaks pointed out by Kevin Wolf
fix qdev_prop_set_drive() to set the property only when attach succeeded (free_drive() will die without this)
Also available at git://repo.or.cz/qemu/armbru.git
Tag block-fixes-v2
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(-)
block.c +22 -0
@@ -659,6 +659,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);
@@ -672,6 +674,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 *
block.h +3 -0
@@ -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);
block_int.h +2 -0
@@ -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 */
hw/fdc.c +5 -5
@@ -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);
hw/ide/qdev.c +1 -1
@@ -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); }
hw/pci-hotplug.c +5 -1
@@ -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 ;
hw/qdev-properties.c +21 -1
@@ -291,6 +291,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 ; }
@@ -300,6 +302,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); } }
@@ -640,11 +643,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 );
hw/qdev.h +2 -1
@@ -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);
hw/s390-virtio.c +1 -1
@@ -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); } }
hw/scsi-bus.c +4 -1
@@ -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);
hw/usb-msd.c +8 -4
@@ -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 ;
Signature · 1 line
1.6.6.1
PATCH 9/12 savevm: Survive hot-unplug of snapshot device
In reply to Markus Armbrustersavevm.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 to goes away.
--- block.c | 25 +++++++++++++++++++++++++ block.h | 1 + savevm.c | 31 ++++--------------------------- 3 files changed, 30 insertions(+), 27 deletions(-)
block.c +25 -0
@@ -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;
@@ -660,6 +663,9 @@ void bdrv_close_all(void)void bdrv_delete (BlockDriverState *bs) {assert (!bs->peer); +if (bs == bs_snapshots) { + bs_snapshots =NULL ; + }/* remove from list, if necessary */ if (bs->device_name[0 ] !='\0' ) {
@@ -1772,6 +1778,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) {
block.h +1 -0
@@ -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,
savevm.c +4 -27
@@ -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 ;
Signature · 1 line
1.6.6.1
[Qemu-devel] Re: [PATCH 09/12] savevm: Survive hot-unplug of snapshot device
Looks good,
Of course specifying an explicit medium for snapshot, be that the snapshot section of a qcow2 image or just a separate flat file and managing that one explicitly would be even better.
[Qemu-devel] Re: [PATCH 09/12] savevm: Survive hot-unplug of snapshot device
Quoted 7 lines · Christoph Hellwig
Looks good,
Reviewed-by Christoph Hellwig <[email protected]>Of course specifying an explicit medium for snapshot, be that the snapshot section of a qcow2 image or just a separate flat file and managing that one explicitly would be even better.
Indeed.
[Qemu-devel] Re: [PATCH 09/12] savevm: Survive hot-unplug of snapshot device
In reply to Markus ArmbrusterAm 25.06.2010 18:53, schrieb Markus Armbruster:
Quoted 46 lines
savevm.ckeeps 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 to goes away.Signed-off-by Markus Armbruster <[email protected]>--- block.c | 25 +++++++++++++++++++++++++ block.h | 1 + savevm.c | 31 ++++--------------------------- 3 files changed, 30 insertions(+), 27 deletions(-)block.c +3
-0@@ -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;
block.c void bdrv_close_all(void) · line 668
Quoted 6 lines
block.c +3
-0@@ -660,6 +663,9 @@ void bdrv_close_all(void)void bdrv_delete (BlockDriverState *bs) {assert (!bs->peer); +if (bs == bs_snapshots) { + bs_snapshots =NULL ; + }
This should probably be in bdrv_close() instead. A BlockDriverState can be closed, but not deleted yet; it can't handle snapshots in this state, though.
block.c int bdrv_can_snapshot(BlockDriverState *bs) · line 1786
Quoted 12 lines
block.c +6
-0/* remove from list, if necessary */ if (bs->device_name[0 ] !='\0' ) {@@ -1772,6 +1778,25 @@ int bdrv_can_snapshot(BlockDriverState *bs)return 1 ; } +BlockDriverState *bdrv_snapshots (void ) +{ +BlockDriverState *bs; + +if (bs_snapshots) +return bs_snapshots;
I know that this function is just moved with no changes, but while we're at it and you need to respin anyway, can we add braces here?
block.c int bdrv_can_snapshot(BlockDriverState *bs) · line 1798
Quoted 12 lines
block.c +12
-0+ + bs =NULL ; +while ((bs =bdrv_next (bs))) { +if (bdrv_can_snapshot (bs)) { + goto ok; + } + } +return NULL ; + ok: + bs_snapshots = bs; +return bs; +}
And instead of a goto we could do the right thing directly in the if block.
Kevin
[Qemu-devel] Re: [PATCH 09/12] savevm: Survive hot-unplug of snapshot device
Quoted 46 lines · Kevin Wolf
Am 25.06.2010 18:53, schrieb Markus Armbruster:
Quoted 45 lines
savevm.ckeeps 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 to goes away.Signed-off-by Markus Armbruster <[email protected]>--- block.c | 25 +++++++++++++++++++++++++ block.h | 1 + savevm.c | 31 ++++--------------------------- 3 files changed, 30 insertions(+), 27 deletions(-)block.c +3
-0@@ -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;
Quoted 11 lines
block.c void bdrv_close_all(void) · line 668
Quoted 7 lines
block.c +3
-0@@ -660,6 +663,9 @@ void bdrv_close_all(void)void bdrv_delete (BlockDriverState *bs) {assert (!bs->peer); +if (bs == bs_snapshots) { + bs_snapshots =NULL ; + }This should probably be in
bdrv_close()instead. A BlockDriverState can be closed, but not deleted yet; it can't handle snapshots in this state, though.
Right. I was thinking about the dangling pointer only.
My patch works as advertized: it fixes the crash. But zapping bs_snapshots in bdrv_close() is even better. I'll respin.
Quoted 16 lines
block.c int bdrv_can_snapshot(BlockDriverState *bs) · line 1786
Quoted 12 lines
block.c +6
-0/* remove from list, if necessary */ if (bs->device_name[0 ] !='\0' ) {@@ -1772,6 +1778,25 @@ int bdrv_can_snapshot(BlockDriverState *bs)return 1 ; } +BlockDriverState *bdrv_snapshots (void ) +{ +BlockDriverState *bs; + +if (bs_snapshots) +return bs_snapshots;I know that this function is just moved with no changes, but while we're at it and you need to respin anyway, can we add braces here?
Quoted 14 lines
block.c int bdrv_can_snapshot(BlockDriverState *bs) · line 1798
Quoted 12 lines
block.c +12
-0+ + bs =NULL ; +while ((bs =bdrv_next (bs))) { +if (bdrv_can_snapshot (bs)) { + goto ok; + } + } +return NULL ; + ok: + bs_snapshots = bs; +return bs; +}And instead of a goto we could do the right thing directly in the if block.
Separate patch. I hate it when people squash code motion and change together.
PATCH 10/12 block: Fix virtual media change for if=none
In reply to Markus ArmbrusterBlockDriverState 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 removableunless 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.
--- 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(-)
block.c +8 -0
@@ -1292,6 +1292,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;
block.h +1 -0
@@ -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);
hw/fdc.c +8 -2
@@ -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 ); + } } }
hw/ide/core.c +1 -0
@@ -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)
hw/scsi-disk.c +4 -1
@@ -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 ; }
hw/scsi-generic.c +1 -0
@@ -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 ; }
hw/virtio-blk.c +1 -0
@@ -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; }
Signature · 1 line
1.6.6.1
[Qemu-devel] Re: [PATCH 10/12] block: Fix virtual media change for if=none
Looks good,
PATCH 11/12 ide: Make PIIX and ISA IDE init functions return the qdev
In reply to Markus Armbruster--- hw/ide.h | 11 ++++++----- hw/ide/isa.c | 8 ++++---- hw/ide/piix.c | 6 ++++-- 3 files changed, 14 insertions(+), 11 deletions(-)
hw/ide.h +6 -5
@@ -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);/* ide-macio.c */ int pmac_ide_init (DriveInfo **hd_table, qemu_irq irq,
hw/ide/isa.c +4 -4
@@ -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 = {
hw/ide/piix.c +4 -2
@@ -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[] = {
Signature · 1 line
1.6.6.1
[Qemu-devel] Re: [PATCH 11/12] ide: Make PIIX and ISA IDE init functions return the qdev
Looks good,
PATCH 12/12 pc: Fix CMOS info for drives defined with -device
In reply to Markus ArmbrusterDrives 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.
--- 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(-)
hw/ide.h +2 -0
@@ -23,4 +23,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 */
hw/ide/qdev.c +7 -0
@@ -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 {
hw/pc.c +59 -35
117 lines not on this page · raw message
hw/pc.h +2 -1
@@ -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);
hw/pc_piix.c +11 -5
@@ -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,11 +133,16 @@ 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" ); } }
@@ -144,8 +150,8 @@ static void pc_init1(ram_addr_t ram_size,pc_audio_init (pci_enabled ? pci_bus :NULL , isa_irq);#endif -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 );
Signature · 1 line
1.6.6.1