[Qemu-devel] virtio_cleanup() missing in virtio-{blk, balloon, 9p}-pci

[Qemu-devel] virtio_cleanup() missing in virtio-{blk, balloon, 9p}-pci

[Markus Armbruster]

Pattern:

FOO's PCIDeviceInfo method exit is virtio_FOO_exit_pci(), which calls
virtio_FOO_exit() and virtio_exit_pci().

virtio_{net,serial}_exit() call virtio_cleanup().

virtio_blk_exit() doesn't.  Why?

virtio-balloon-pci uses virtio_exit_pci() as exit method.  No
virtio_cleanup()?

virtio-9p-pci doesn't have an exit method.  Doesn't feel right.

I suspect these three leak memory or worse on hot unplug.

[Qemu-devel] Re: virtio_cleanup() missing in virtio-{blk, balloon, 9p}-pci

[Kevin Wolf]

Am 11.10.2010 12:00, schrieb Markus Armbruster:
> Pattern:
> 
> FOO's PCIDeviceInfo method exit is virtio_FOO_exit_pci(), which calls
> virtio_FOO_exit() and virtio_exit_pci().
> 
> virtio_{net,serial}_exit() call virtio_cleanup().
> 
> virtio_blk_exit() doesn't.  Why?
> 
> virtio-balloon-pci uses virtio_exit_pci() as exit method.  No
> virtio_cleanup()?
> 
> virtio-9p-pci doesn't have an exit method.  Doesn't feel right.
> 
> I suspect these three leak memory or worse on hot unplug.

Yes, virtio-blk seems to leak memory there.

We should probably also cancel all running requests before removing the
disk. I suspect not doing this could cause some segfaults when the AIO
callback wants to access a disk that doesn't exist any more.

Kevin

Re: [Qemu-devel] virtio_cleanup() missing in virtio-{blk, balloon, 9p}-pci

[M. Mohan Kumar]

> Pattern:
> 
> FOO's PCIDeviceInfo method exit is virtio_FOO_exit_pci(), which calls
> virtio_FOO_exit() and virtio_exit_pci().
> 
> virtio_{net,serial}_exit() call virtio_cleanup().
> 
> virtio_blk_exit() doesn't.  Why?
> 
> virtio-balloon-pci uses virtio_exit_pci() as exit method.  No
> virtio_cleanup()?
> 
> virtio-9p-pci doesn't have an exit method.  Doesn't feel right.

9p does not support hot-plug now. We will implement virtio_9p_exit when we are 
adding support for hot-plug

Re: [Qemu-devel] virtio_cleanup() missing in virtio-{blk, balloon, 9p}-pci

[Markus Armbruster]

"M. Mohan Kumar" <mohan@in.ibm.com> writes:

>> Pattern:
>> 
>> FOO's PCIDeviceInfo method exit is virtio_FOO_exit_pci(), which calls
>> virtio_FOO_exit() and virtio_exit_pci().
>> 
>> virtio_{net,serial}_exit() call virtio_cleanup().
>> 
>> virtio_blk_exit() doesn't.  Why?
>> 
>> virtio-balloon-pci uses virtio_exit_pci() as exit method.  No
>> virtio_cleanup()?
>> 
>> virtio-9p-pci doesn't have an exit method.  Doesn't feel right.
>
> 9p does not support hot-plug now. We will implement virtio_9p_exit when we are 
> adding support for hot-plug

If hot plug is not expected to work, perhaps the driver should refuse
it.  Untested sketch:

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 729917d..b09d85d 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -683,6 +683,11 @@ static int virtio_9p_init_pci(PCIDevice *pci_dev)
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
     VirtIODevice *vdev;
 
+    if (pci_dev->qdev.hotplugged) {
+        error_report("Device doesn't support hot plug");
+        return -1;
+    }
+
     vdev = virtio_9p_init(&pci_dev->qdev, &proxy->fsconf);
     virtio_init_pci(proxy, vdev,
                     PCI_VENDOR_ID_REDHAT_QUMRANET,