Re: [Qemu-devel] [PATCH] sdhci: Make device "sdhci-pci" unavailable with -device again

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 04/12/2015 15:07, Markus Armbruster wrote:
>> We made it unavailable in commit 1910913 because its use of
>> drive_get_next() instead of a property.  Commit 5ec911c replaced
>> drive_get_next() and made the device available, but the property isn't
>> quite right, and the code dangerously ignores blk_attach_dev()
>> failure.  Disable it again before the property becomes ABI, and mark
>> the dangerous spot FIXME.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/sd/sd.c    | 1 +
>>  hw/sd/sdhci.c | 6 ++++++
>>  2 files changed, 7 insertions(+)
>> 
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index ce4d44b..d0be5ea 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -494,6 +494,7 @@ SDState *sd_init(BlockBackend *blk, bool is_spi)
>>      if (sd->blk) {
>>          /* Attach dev if not already attached.  (This call ignores an
>>           * error return code if sd->blk is already attached.) */
>> +        /* FIXME ignoring blk_attach_dev() failure is wrong and dangerous */
>
> No, it's not (it is tricky though) because blk_attach_dev actually will
> always  fail here when using the drive= property, and never when using
> drive_get_next.
>
> In the drive= case, the successful call (and also the one that will
> catch possible mistakes) is from parse_drive in
> hw/core/qdev-properties-system.c:
>
> $ x86_64-softmmu/qemu-system-x86_64 -drive
> if=none,driver=null-aio,id=foo -device virtio-blk-pci,drive=foo
> -device sdhci-pci,drive=foo
> qemu-system-x86_64: -device sdhci-pci,drive=foo: Drive 'foo' is
> already in use by another device
>
> Did you have something else in mind?

My comment makes two claims: "wrong" and "dangerous".

First "dangerous".  You're making a non-local argument why it's not
actually broken, and you might be right.  If you are, it's just fragile,
not broken.  We could debate whether to call it dangerous or fragile,
but I don't really care.  If you'd prefer to call it fragile, let's
update the comment and the commit message.

Now "wrong".  The qdev property belongs to the SD card (the thing we're
initializing here), not the SD controller sdhci-pci.  Unfortunately, the
SD card still hasn't been qdevified.  But if we permit tacking the
property to the controller now, we're stuck with having it there
forever.  No harm if the SD card never becomes an object in its own
right.  But if it does, it'll end up in the same unhappy place as
usb-storage, where we hackishly jump through hoops to somehow transfer
the backend from the controller to the SCSI device.  This has caused so
much trouble that we replaced the whole thing by usb-bot.  I'm not keen
on repeating the experience.