From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46610) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vigj4-0006Ad-Vs for qemu-devel@nongnu.org; Tue, 19 Nov 2013 03:32:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vigiy-0000ax-4K for qemu-devel@nongnu.org; Tue, 19 Nov 2013 03:32:02 -0500 Date: Tue, 19 Nov 2013 16:31:50 +0800 From: Amos Kong Message-ID: <20131119083150.GA16173@amosk.info> References: <1381254296-3203-1-git-send-email-afaerber@suse.de> <1381254296-3203-48-git-send-email-afaerber@suse.de> <20131118122928.GA23465@amosk.info> <528A25A8.3000301@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <528A25A8.3000301@suse.de> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PULL 47/58] qdev-monitor: Unref device when device_add fails List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?iso-8859-1?Q?F=E4rber?= Cc: Paolo Bonzini , qemu-devel@nongnu.org, Stefan Hajnoczi , qemu-stable@nongnu.org On Mon, Nov 18, 2013 at 03:35:20PM +0100, Andreas F=E4rber wrote: > Am 18.11.2013 13:29, schrieb Amos Kong: > > On Tue, Oct 08, 2013 at 07:44:45PM +0200, Andreas F=E4rber wrote: > >> From: Stefan Hajnoczi > >> > >> qdev_device_add() leaks the created device upon failure. I suspect = this > >> problem crept in because qdev_free() unparents the device but does n= ot > >> drop a reference - confusing name. > >> > >> Cc: qemu-stable@nongnu.org > >> Signed-off-by: Stefan Hajnoczi > >> Signed-off-by: Andreas F=E4rber > >=20 > > Hi Stefan, > >=20 > > This commit caused a regression bug: > >=20 > > hotplug more than 32 disks to vm, qemu crash > >=20 > > --- > >=20 > > [amos@amosk qemu]$ cat radd.sh=20 > > 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 > >=20 > > echo drive_add $i.$j id=3Ddrv$i$j,file=3D/tmp/resize$i$j.qcow2,if=3Dn= one > > echo drive_add $i.$j id=3Ddrv$i$j,file=3D/tmp/resize$i$j.qcow2,if=3Dn= one | nc -U /tmp/m > >=20 > > echo device_add virtio-blk-pci,id=3Ddev$i$j,drive=3Ddrv$i$j,addr=3D0x= $i.$j,multifunction=3Don > > echo device_add virtio-blk-pci,id=3Ddev$i$j,drive=3Ddrv$i$j,addr=3D0x= $i.$j,multifunction=3Don | nc -U /tmp/m > > done > > done >=20 > Hi, thanks for catching this. >=20 > Is this only with virtio-blk-pci or with any PCI card or only when > drives are involved? I can reproduce by hotplugging virtio-net-pci NIC 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 echo "netdev_add tap,id=3Ddev$i$j" | nc -U /tmp/m echo "device_add virtio-net-pci,netdev=3Ddev$i$j,id=3Dnic$i$j,addr=3D0x$i= .$j,multifunction=3Don" | nc -U /tmp/m done done > 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 ru= n > by everyone. I will do it. =20 > 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? >=20 > I had reviewed the call paths and believe the patch to be 100% good, so > the fault will very likely be elsewhere. >=20 > Regards, > Andreas >=20 > >=20 > > ---- > >=20 > > #0 0x00005555558b7f95 in flatview_ref (view=3D0x0) at /home/devel/qe= mu/memory.c:300 > > #1 0x00005555558b9689 in address_space_get_flatview (as=3D0x55555645= d660) at /home/devel/qemu/memory.c:656 > > #2 0x00005555558ba416 in address_space_update_topology (as=3D0x55555= 645d660) at /home/devel/qemu/memory.c:760 > > #3 0x00005555558ba5cf in memory_region_transaction_commit () at /hom= e/devel/qemu/memory.c:799 > > #4 0x00005555558bcfcc in memory_region_set_enabled (mr=3D0x55555647a= f08, enabled=3Dfalse) at /home/devel/qemu/memory.c:1503 > > #5 0x000055555571a0af in do_pci_register_device (pci_dev=3D0x5555564= 7ac10, bus=3D0x5555564132b0, name=3D0x555556261100 "virtio-blk-pci", devf= n=3D26) at hw/pci/pci.c:846 > > #6 0x000055555571c6cc in pci_qdev_init (qdev=3D0x55555647ac10) at hw= /pci/pci.c:1751 > > #7 0x0000555555694d70 in device_realize (dev=3D0x55555647ac10, err=3D= 0x7fffffffc8e8) at hw/core/qdev.c:178 > > #8 0x00005555556966fc in device_set_realized (obj=3D0x55555647ac10, = value=3Dtrue, err=3D0x7fffffffca60) at hw/core/qdev.c:699 > > #9 0x00005555557e7b57 in property_set_bool (obj=3D0x55555647ac10, v=3D= 0x55555679a830, opaque=3D0x555556461b10, name=3D0x5555559922ae "realized"= , errp=3D0x7fffffffca60) > > at qom/object.c:1315 > > #10 0x00005555557e665b in object_property_set (obj=3D0x55555647ac10, = v=3D0x55555679a830, name=3D0x5555559922ae "realized", errp=3D0x7fffffffca= 60) at qom/object.c:803 > > #11 0x00005555557e816e in object_property_set_qobject (obj=3D0x555556= 47ac10, value=3D0x555556678880, name=3D0x5555559922ae "realized", errp=3D= 0x7fffffffca60) at qom/qom-qobject.c:24 > > #12 0x00005555557e6950 in object_property_set_bool (obj=3D0x55555647a= c10, value=3Dtrue, name=3D0x5555559922ae "realized", errp=3D0x7fffffffca6= 0) at qom/object.c:866 > > #13 0x0000555555694ca7 in qdev_init (dev=3D0x55555647ac10) at hw/core= /qdev.c:163 > > #14 0x00005555557c60ee in qdev_device_add (opts=3D0x555556525370) at = qdev-monitor.c:543 > > #15 0x00005555557c6730 in do_device_add (mon=3D0x5555562fb760, qdict=3D= 0x55555645d440, ret_data=3D0x7fffffffcb80) at qdev-monitor.c:656 > > #16 0x00005555558c8892 in handle_user_command (mon=3D0x5555562fb760, = cmdline=3D0x5555563f0f60 "device_add virtio-blk-pci,id=3Ddev32,drive=3Ddr= v32,addr=3D0x3.2,multifunction=3Don") > > at /home/devel/qemu/monitor.c:4137 > > #17 0x00005555558ca10f in monitor_command_cb (mon=3D0x5555562fb760, c= mdline=3D0x5555563f0f60 "device_add virtio-blk-pci,id=3Ddev32,drive=3Ddrv= 32,addr=3D0x3.2,multifunction=3Don",=20 > > opaque=3D0x0) at /home/devel/qemu/monitor.c:4757 > > #18 0x00005555557e9491 in readline_handle_byte (rs=3D0x5555563f0f60, = ch=3D10) at readline.c:373 > > #19 0x00005555558ca045 in monitor_read (opaque=3D0x5555562fb760, buf=3D= 0x7fffffffccf0 "\n\315\377\377\377\177", size=3D1) at /home/devel/qemu/mo= nitor.c:4743 > > #20 0x00005555557c6cc8 in qemu_chr_be_write (s=3D0x555556269040, buf=3D= 0x7fffffffccf0 "\n\315\377\377\377\177", len=3D1) at qemu-char.c:165 > > #21 0x00005555557cb026 in tcp_chr_read (chan=3D0x55555645fe40, cond=3D= G_IO_IN, opaque=3D0x555556269040) at qemu-char.c:2487 > > #22 0x00007ffff76ede06 in g_main_context_dispatch () from /lib64/libg= lib-2.0.so.0 > > #23 0x000055555578ef33 in glib_pollfds_poll () at main-loop.c:189 > > #24 0x000055555578f028 in os_host_main_loop_wait (timeout=3D77312299)= at main-loop.c:234 > > #25 0x000055555578f100 in main_loop_wait (nonblocking=3D0) at main-lo= op.c:483 > > #26 0x000055555582e234 in main_loop () at vl.c:2014 > > #27 0x0000555555835697 in main (argc=3D14, argv=3D0x7fffffffe298, env= p=3D0x7fffffffe310) at vl.c:4362 > >=20 > >> --- > >> 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) !=3D 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); > >> } =20 > >> if (qdev_init(qdev) < 0) { > >> + object_unref(OBJECT(qdev)); > >> qerror_report(QERR_DEVICE_INIT_FAILED, driver); > >> return NULL; > >> } > >> --=20 > >> 1.8.1.4 >=20 > --=20 > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrn= berg --=20 Amos.