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 <arm...@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?
Paolo
> blk_attach_dev(sd->blk, sd);
> blk_set_dev_ops(sd->blk, &sd_block_ops, sd);
> }
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index d70d1a6..0a338ed 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1261,6 +1261,12 @@ static void sdhci_pci_class_init(ObjectClass *klass,
> void *data)
> set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
> dc->vmsd = &sdhci_vmstate;
> dc->props = sdhci_pci_properties;
> + /*
> + * Reason: realize() method uses sd_init(), which ignores
> + * blk_attach_dev() failure (potentially dangerous), and the block
> + * properties really belong to the card, not the controller.
> + */
> + dc->cannot_instantiate_with_device_add_yet = true;
> }
>
> static const TypeInfo sdhci_pci_info = {
>
