Kevin Wolf <kw...@redhat.com> writes:
> Am 25.06.2010 18:53, schrieb 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.
>>
>> 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.
[...]
>> diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
>> index d743192..b47e01e 100644
>> --- a/hw/pci-hotplug.c
>> +++ b/hw/pci-hotplug.c
>> @@ -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.
Correct.
>> if (qdev_init(&dev->qdev) < 0)
>> dev = NULL;
>> break;
[...]
>> diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
>> index 2c58aca..9678328 100644
>> --- a/hw/scsi-bus.c
>> +++ b/hw/scsi-bus.c
>> @@ -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.
Yes.
>> 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..008cc0f 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,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!
