Re: [Qemu-devel] [PULL 47/58] qdev-monitor: Unref device when device_add fails

[Andreas Färber]

Am 18.11.2013 13:29, schrieb Amos Kong:
> On Tue, Oct 08, 2013 at 07:44:45PM +0200, Andreas Färber wrote:
>> From: Stefan Hajnoczi <address@hidden>
>>
>> qdev_device_add() leaks the created device upon failure.  I suspect this
>> problem crept in because qdev_free() unparents the device but does not
>> drop a reference - confusing name.
>>
>> Cc: address@hidden
>> Signed-off-by: Stefan Hajnoczi <address@hidden>
>> Signed-off-by: Andreas Färber <address@hidden>
> 
> Hi Stefan,
> 
> This commit caused a regression bug:
> 
> hotplug more than 32 disks to vm, qemu crash
> 
> ---
> 
> address@hidden qemu]$ cat radd.sh 
> for i in `seq 3 9` a b c d e f 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f;do
> for j in `seq 1 7` 0;do
> /bin/cp /images/none.qcow2 /tmp/resize$i$j.qcow2
> 
> echo drive_add $i.$j id=drv$i$j,file=/tmp/resize$i$j.qcow2,if=none
> echo drive_add $i.$j id=drv$i$j,file=/tmp/resize$i$j.qcow2,if=none | nc -U 
> /tmp/m
> 
> echo device_add 
> virtio-blk-pci,id=dev$i$j,drive=drv$i$j,addr=0x$i.$j,multifunction=on
> echo device_add 
> virtio-blk-pci,id=dev$i$j,drive=drv$i$j,addr=0x$i.$j,multifunction=on | nc -U 
> /tmp/m
> done
> done

Hi, thanks for catching this.

Is this only with virtio-blk-pci or with any PCI card or only when
drives are involved? Either way it would be really great if you could
add such tests to Stefan's qtest using QMP, so that it can easily be run
by everyone.

The stacktrace below is not really telling to me. I wonder if we forget
to clean up some MemoryRegion in the device that still has a back
reference or whether the Memory API still references MemoryRegions that
have been destroyed by the device or forgets the reference devices it
still needs... Paolo?

I had reviewed the call paths and believe the patch to be 100% good, so
the fault will very likely be elsewhere.

Regards,
Andreas

> 
> ----
> 
> #0  0x00005555558b7f95 in flatview_ref (view=0x0) at 
> /home/devel/qemu/memory.c:300
> #1  0x00005555558b9689 in address_space_get_flatview (as=0x55555645d660) at 
> /home/devel/qemu/memory.c:656
> #2  0x00005555558ba416 in address_space_update_topology (as=0x55555645d660) 
> at /home/devel/qemu/memory.c:760
> #3  0x00005555558ba5cf in memory_region_transaction_commit () at 
> /home/devel/qemu/memory.c:799
> #4  0x00005555558bcfcc in memory_region_set_enabled (mr=0x55555647af08, 
> enabled=false) at /home/devel/qemu/memory.c:1503
> #5  0x000055555571a0af in do_pci_register_device (pci_dev=0x55555647ac10, 
> bus=0x5555564132b0, name=0x555556261100 "virtio-blk-pci", devfn=26) at 
> hw/pci/pci.c:846
> #6  0x000055555571c6cc in pci_qdev_init (qdev=0x55555647ac10) at 
> hw/pci/pci.c:1751
> #7  0x0000555555694d70 in device_realize (dev=0x55555647ac10, 
> err=0x7fffffffc8e8) at hw/core/qdev.c:178
> #8  0x00005555556966fc in device_set_realized (obj=0x55555647ac10, 
> value=true, err=0x7fffffffca60) at hw/core/qdev.c:699
> #9  0x00005555557e7b57 in property_set_bool (obj=0x55555647ac10, 
> v=0x55555679a830, opaque=0x555556461b10, name=0x5555559922ae "realized", 
> errp=0x7fffffffca60)
>     at qom/object.c:1315
> #10 0x00005555557e665b in object_property_set (obj=0x55555647ac10, 
> v=0x55555679a830, name=0x5555559922ae "realized", errp=0x7fffffffca60) at 
> qom/object.c:803
> #11 0x00005555557e816e in object_property_set_qobject (obj=0x55555647ac10, 
> value=0x555556678880, name=0x5555559922ae "realized", errp=0x7fffffffca60) at 
> qom/qom-qobject.c:24
> #12 0x00005555557e6950 in object_property_set_bool (obj=0x55555647ac10, 
> value=true, name=0x5555559922ae "realized", errp=0x7fffffffca60) at 
> qom/object.c:866
> #13 0x0000555555694ca7 in qdev_init (dev=0x55555647ac10) at hw/core/qdev.c:163
> #14 0x00005555557c60ee in qdev_device_add (opts=0x555556525370) at 
> qdev-monitor.c:543
> #15 0x00005555557c6730 in do_device_add (mon=0x5555562fb760, 
> qdict=0x55555645d440, ret_data=0x7fffffffcb80) at qdev-monitor.c:656
> #16 0x00005555558c8892 in handle_user_command (mon=0x5555562fb760, 
> cmdline=0x5555563f0f60 "device_add 
> virtio-blk-pci,id=dev32,drive=drv32,addr=0x3.2,multifunction=on")
>     at /home/devel/qemu/monitor.c:4137
> #17 0x00005555558ca10f in monitor_command_cb (mon=0x5555562fb760, 
> cmdline=0x5555563f0f60 "device_add 
> virtio-blk-pci,id=dev32,drive=drv32,addr=0x3.2,multifunction=on", 
>     opaque=0x0) at /home/devel/qemu/monitor.c:4757
> #18 0x00005555557e9491 in readline_handle_byte (rs=0x5555563f0f60, ch=10) at 
> readline.c:373
> #19 0x00005555558ca045 in monitor_read (opaque=0x5555562fb760, 
> buf=0x7fffffffccf0 "\n\315\377\377\377\177", size=1) at 
> /home/devel/qemu/monitor.c:4743
> #20 0x00005555557c6cc8 in qemu_chr_be_write (s=0x555556269040, 
> buf=0x7fffffffccf0 "\n\315\377\377\377\177", len=1) at qemu-char.c:165
> #21 0x00005555557cb026 in tcp_chr_read (chan=0x55555645fe40, cond=G_IO_IN, 
> opaque=0x555556269040) at qemu-char.c:2487
> #22 0x00007ffff76ede06 in g_main_context_dispatch () from 
> /lib64/libglib-2.0.so.0
> #23 0x000055555578ef33 in glib_pollfds_poll () at main-loop.c:189
> #24 0x000055555578f028 in os_host_main_loop_wait (timeout=77312299) at 
> main-loop.c:234
> #25 0x000055555578f100 in main_loop_wait (nonblocking=0) at main-loop.c:483
> #26 0x000055555582e234 in main_loop () at vl.c:2014
> #27 0x0000555555835697 in main (argc=14, argv=0x7fffffffe298, 
> envp=0x7fffffffe310) at vl.c:4362
> 
>> ---
>>  qdev-monitor.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index b1ce26a..531b258 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -518,6 +518,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>      }
>>      if (qemu_opt_foreach(opts, set_property, qdev, 1) != 0) {
>>          qdev_free(qdev);
>> +        object_unref(OBJECT(qdev));
>>          return NULL;
>>      }
>>      if (qdev->id) {
>> @@ -531,6 +532,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>>          g_free(name);
>>      }        
>>      if (qdev_init(qdev) < 0) {
>> +        object_unref(OBJECT(qdev));
>>          qerror_report(QERR_DEVICE_INIT_FAILED, driver);
>>          return NULL;
>>      }
>> -- 
>> 1.8.1.4

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg