[Qemu-devel] Monitor command pci_add regressed (qdev)

[Qemu-devel] Monitor command pci_add regressed (qdev)

[Markus Armbruster]

gdb --args qemu -monitor stdio tmp.qcow2 -S
[...]
QEMU 0.10.50 monitor - type 'help' for more information
(qemu) pci_add pci_addr=auto storage if=virtio,file=foo.img
Program received signal SIGSEGV, Segmentation fault.
0x080630c5 in virtio_blk_init (dev=0x849a008)
    at /home/armbru/work/qemu/hw/virtio-blk.c:368
368         bs->private = dev;

This used to work just fine.

Culprit seems to be commit 07e3af9a.  qdev_init_bdrv() fails, and
virtio_blk_init() doesn't check the failure.

I haven't investigated why qdev_init_bdrv() fails (the old code got the
BlockDriverState just fine).  Regardless, there are scenarios where
qdev_init_bdrv() rightly fails, so virtio_blk_init() needs fixing.

Returning NULL would be easy enough, but its caller
virtio_blk_init_pci() doesn't check its value, and it is a qdev init()
callback, which can't fail.  How to handle the error?  exit(1) would be
just fine for -drive, but not for a monitor command.

Re: [Qemu-devel] Monitor command pci_add regressed (qdev)

[Gerd Hoffmann]

On 06/10/09 19:43, Markus Armbruster wrote:
> gdb --args qemu -monitor stdio tmp.qcow2 -S

> (qemu) pci_add pci_addr=auto storage if=virtio,file=foo.img
> Program received signal SIGSEGV, Segmentation fault.

> Culprit seems to be commit 07e3af9a.  qdev_init_bdrv() fails, and
> virtio_blk_init() doesn't check the failure.
>
> I haven't investigated why qdev_init_bdrv() fails (the old code got the
> BlockDriverState just fine).  Regardless, there are scenarios where
> qdev_init_bdrv() rightly fails, so virtio_blk_init() needs fixing.
>
> Returning NULL would be easy enough, but its caller
> virtio_blk_init_pci() doesn't check its value, and it is a qdev init()
> callback, which can't fail.  How to handle the error?  exit(1) would be
> just fine for -drive, but not for a monitor command.

I think the correct long-term fix is to cleanly separate the guest-side 
stuff (virtio or whatever else block device) and the host-side 
(BlockDriverState) and how the two are linked together.

The BlockDriverState setup can fail for a number of reasons.
The virtio blk setup shouldn't fail.  So setting up BlockDriverState 
first, when succeeded setup virtio-blk, then link the two should work fine.

Dunno how much work that would be with the current qemu code structure 
though.  Maybe we should allow device initialization fail for the time 
being.

cheers,
   Gerd

[Qemu-devel] [PATCH] qdev: Fix regression in "pci_add ... storage if=virtio, ..."

[Markus Armbruster]

qemu_pci_hot_add_storage() runs qdev_init() twice.  Broken in commit
07e3af9a "Virtio-blk qdev conversion".

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/pci-hotplug.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index abe5bae..4d49c29 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -122,7 +122,6 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon, PCIBus *pci_bus,
         break;
     case IF_VIRTIO:
         opaque = pci_create_simple(pci_bus, -1, "virtio-blk-pci");
-        qdev_init(opaque);
         break;
     }
 
-- 
1.6.0.6

Re: [Qemu-devel] [PATCH] qdev: Fix regression in "pci_add ... storage if=virtio, ..."

[Gerd Hoffmann]

   Hi,

>       case IF_VIRTIO:
>           opaque = pci_create_simple(pci_bus, -1, "virtio-blk-pci");
> -        qdev_init(opaque);
>           break;

Yep, pci_create_simple() calls qdev_init() already.
Patch looks good.

cheers,
   Gerd