From patchwork Tue Jun 29 14:58:30 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Markus Armbruster X-Patchwork-Id: 57280 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [199.232.76.165]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id D54C2B7096 for ; Wed, 30 Jun 2010 01:03:38 +1000 (EST) Received: from localhost ([127.0.0.1]:52410 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OTcLY-0008Rd-0X for incoming@patchwork.ozlabs.org; Tue, 29 Jun 2010 11:03:36 -0400 Received: from [140.186.70.92] (port=45468 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OTcHP-0005y2-6b for qemu-devel@nongnu.org; Tue, 29 Jun 2010 10:59:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OTcHE-0003BC-H1 for qemu-devel@nongnu.org; Tue, 29 Jun 2010 10:59:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:20189) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OTcHE-0003Aq-6Q for qemu-devel@nongnu.org; Tue, 29 Jun 2010 10:59:08 -0400 Received: from int-mx03.intmail.prod.int.phx2.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.16]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o5TEwxSv020825 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 29 Jun 2010 10:58:59 -0400 Received: from blackfin.pond.sub.org (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx03.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o5TEwwi8029523; Tue, 29 Jun 2010 10:58:58 -0400 Received: by blackfin.pond.sub.org (Postfix, from userid 500) id 1152A2BF; Tue, 29 Jun 2010 16:58:31 +0200 (CEST) From: Markus Armbruster To: qemu-devel@nongnu.org References: <1277484812-22012-1-git-send-email-armbru@redhat.com> <1277484812-22012-9-git-send-email-armbru@redhat.com> Date: Tue, 29 Jun 2010 16:58:30 +0200 In-Reply-To: <1277484812-22012-9-git-send-email-armbru@redhat.com> (Markus Armbruster's message of "Fri, 25 Jun 2010 18:53:28 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.67 on 10.5.11.16 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. Cc: kwolf@redhat.com, kraxel@redhat.com, hch@lst.de Subject: [Qemu-devel] [PATCH v2 08/12] block: Catch attempt to attach multiple devices to a blockdev X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 --- 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(-) diff --git a/block.c b/block.c index e71a771..5e0ffa0 100644 --- a/block.c +++ b/block.c @@ -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 * diff --git a/block.h b/block.h index 6a157f4..88ac06e 100644 --- a/block.h +++ b/block.h @@ -71,6 +71,9 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags); int bdrv_open(BlockDriverState *bs, const char *filename, int flags, BlockDriver *drv); void bdrv_close(BlockDriverState *bs); +int bdrv_attach(BlockDriverState *bs, DeviceState *qdev); +void bdrv_detach(BlockDriverState *bs, DeviceState *qdev); +DeviceState *bdrv_get_attached(BlockDriverState *bs); int bdrv_check(BlockDriverState *bs); int bdrv_read(BlockDriverState *bs, int64_t sector_num, uint8_t *buf, int nb_sectors); diff --git a/block_int.h b/block_int.h index e60aed4..a94b801 100644 --- a/block_int.h +++ b/block_int.h @@ -148,6 +148,8 @@ struct BlockDriverState { BlockDriver *drv; /* NULL means no media */ void *opaque; + DeviceState *peer; + char filename[1024]; char backing_file[1024]; /* if non zero, the image is a diff of this file image */ diff --git a/hw/fdc.c b/hw/fdc.c index 08712bc..1496cfa 100644 --- a/hw/fdc.c +++ b/hw/fdc.c @@ -1860,10 +1860,10 @@ FDCtrl *fdctrl_init_isa(DriveInfo **fds) dev = isa_create("isa-fdc"); if (fds[0]) { - qdev_prop_set_drive(&dev->qdev, "driveA", fds[0]->bdrv); + qdev_prop_set_drive_nofail(&dev->qdev, "driveA", fds[0]->bdrv); } if (fds[1]) { - qdev_prop_set_drive(&dev->qdev, "driveB", fds[1]->bdrv); + qdev_prop_set_drive_nofail(&dev->qdev, "driveB", fds[1]->bdrv); } if (qdev_init(&dev->qdev) < 0) return NULL; @@ -1882,10 +1882,10 @@ FDCtrl *fdctrl_init_sysbus(qemu_irq irq, int dma_chann, fdctrl = &sys->state; fdctrl->dma_chann = dma_chann; /* FIXME */ if (fds[0]) { - qdev_prop_set_drive(dev, "driveA", fds[0]->bdrv); + qdev_prop_set_drive_nofail(dev, "driveA", fds[0]->bdrv); } if (fds[1]) { - qdev_prop_set_drive(dev, "driveB", fds[1]->bdrv); + qdev_prop_set_drive_nofail(dev, "driveB", fds[1]->bdrv); } qdev_init_nofail(dev); sysbus_connect_irq(&sys->busdev, 0, irq); @@ -1903,7 +1903,7 @@ FDCtrl *sun4m_fdctrl_init(qemu_irq irq, target_phys_addr_t io_base, dev = qdev_create(NULL, "SUNW,fdtwo"); if (fds[0]) { - qdev_prop_set_drive(dev, "drive", fds[0]->bdrv); + qdev_prop_set_drive_nofail(dev, "drive", fds[0]->bdrv); } qdev_init_nofail(dev); sys = DO_UPCAST(FDCtrlSysBus, busdev.qdev, dev); diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c index a5fdab0..b34c473 100644 --- a/hw/ide/qdev.c +++ b/hw/ide/qdev.c @@ -83,7 +83,7 @@ IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive) dev = qdev_create(&bus->qbus, "ide-drive"); qdev_prop_set_uint32(dev, "unit", unit); - qdev_prop_set_drive(dev, "drive", drive->bdrv); + qdev_prop_set_drive_nofail(dev, "drive", drive->bdrv); qdev_init_nofail(dev); return DO_UPCAST(IDEDevice, qdev, dev); } diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c index d743192..fe468d6 100644 --- a/hw/pci-hotplug.c +++ b/hw/pci-hotplug.c @@ -214,7 +214,11 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon, return NULL; } dev = pci_create(bus, devfn, "virtio-blk-pci"); - qdev_prop_set_drive(&dev->qdev, "drive", dinfo->bdrv); + if (qdev_prop_set_drive(&dev->qdev, "drive", dinfo->bdrv) < 0) { + qdev_free(&dev->qdev); + dev = NULL; + break; + } if (qdev_init(&dev->qdev) < 0) dev = NULL; break; diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c index 257233e..ce6e002 100644 --- a/hw/qdev-properties.c +++ b/hw/qdev-properties.c @@ -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); diff --git a/hw/qdev.h b/hw/qdev.h index 7a01a81..cbc89f2 100644 --- a/hw/qdev.h +++ b/hw/qdev.h @@ -275,7 +275,8 @@ void qdev_prop_set_string(DeviceState *dev, const char *name, char *value); void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *value); void qdev_prop_set_netdev(DeviceState *dev, const char *name, VLANClientState *value); void qdev_prop_set_vlan(DeviceState *dev, const char *name, VLANState *value); -void qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState *value); +int qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState *value) QEMU_WARN_UNUSED_RESULT; +void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name, BlockDriverState *value); void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value); /* FIXME: Remove opaque pointer properties. */ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value); diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c index c7c3fc9..6af58e2 100644 --- a/hw/s390-virtio.c +++ b/hw/s390-virtio.c @@ -262,7 +262,7 @@ static void s390_init(ram_addr_t ram_size, } dev = qdev_create((BusState *)s390_bus, "virtio-blk-s390"); - qdev_prop_set_drive(dev, "drive", dinfo->bdrv); + qdev_prop_set_drive_nofail(dev, "drive", dinfo->bdrv); qdev_init_nofail(dev); } } diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c index 2c58aca..b84b9b9 100644 --- a/hw/scsi-bus.c +++ b/hw/scsi-bus.c @@ -91,7 +91,10 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockDriverState *bdrv, int driver = bdrv_is_sg(bdrv) ? "scsi-generic" : "scsi-disk"; dev = qdev_create(&bus->qbus, driver); qdev_prop_set_uint32(dev, "scsi-id", unit); - qdev_prop_set_drive(dev, "drive", bdrv); + if (qdev_prop_set_drive(dev, "drive", bdrv) < 0) { + qdev_free(dev); + return NULL; + } if (qdev_init(dev) < 0) return NULL; return DO_UPCAST(SCSIDevice, qdev, dev); diff --git a/hw/usb-msd.c b/hw/usb-msd.c index 19a14b4..65e9624 100644 --- a/hw/usb-msd.c +++ b/hw/usb-msd.c @@ -532,12 +532,13 @@ static int usb_msd_initfn(USBDevice *dev) /* * Hack alert: this pretends to be a block device, but it's really * a SCSI bus that can serve only a single device, which it - * creates automatically. Two drive properties pointing to the - * same drive is not good: free_drive() dies for the second one. - * Zap the one we're not going to use. + * creates automatically. But first it needs to detach from its + * blockdev, or else scsi_bus_legacy_add_drive() dies when it + * attaches again. * * The hack is probably a bad idea. */ + bdrv_detach(bs, &s->dev.qdev); s->conf.bs = NULL; s->dev.speed = USB_SPEED_FULL; @@ -609,7 +610,10 @@ static USBDevice *usb_msd_init(const char *filename) if (!dev) { return NULL; } - qdev_prop_set_drive(&dev->qdev, "drive", dinfo->bdrv); + if (qdev_prop_set_drive(&dev->qdev, "drive", dinfo->bdrv) < 0) { + qdev_free(&dev->qdev); + return NULL; + } if (qdev_init(&dev->qdev) < 0) return NULL;