[Qemu-devel] Re: [PATCH 08/12] block: Catch attempt to attach multiple devices to a blockdev

[Markus Armbruster]

Kevin Wolf <address@hidden> 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!