From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56632) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFMcz-0005Wn-60 for qemu-devel@nongnu.org; Mon, 17 Feb 2014 06:44:56 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WFMcr-00053g-OK for qemu-devel@nongnu.org; Mon, 17 Feb 2014 06:44:49 -0500 Received: from cantor2.suse.de ([195.135.220.15]:56562 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFMcr-000538-I4 for qemu-devel@nongnu.org; Mon, 17 Feb 2014 06:44:41 -0500 Message-ID: <5301F622.4000609@suse.de> Date: Mon, 17 Feb 2014 12:44:34 +0100 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1388624531-13439-1-git-send-email-akong@redhat.com> <20140217102324.GA1925@amosk.info> In-Reply-To: <20140217102324.GA1925@amosk.info> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2] qdev: add the device to the QOM tree before using it to set a link List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amos Kong , qemu-devel@nongnu.org Cc: pbonzini@redhat.com, aliguori@amazon.com, hutao@cn.fujitsu.com Am 17.02.2014 11:23, schrieb Amos Kong: > On Thu, Jan 02, 2014 at 09:02:11AM +0800, Amos Kong wrote: >> Test steps: >> (qemu) device_add e1000,addr=3Dadsf >> Property 'e1000.addr' doesn't take value 'adsf' >> (qemu) info qtree >> Then qemu crashed. >> >> Currently we set a link to the new device for qdev parent bus, but the >> device hasn't been added to QOM tree. When it fails to set properties, >> object_unparent() can't cleanup the device. >> >> This patch moves qdev_set_parent_bus() back to object_property_add_chi= ld(), >> we only needs to unref the object if setting properties fails. >> >> Signed-off-by: Amos Kong >> --- >> V2: fix bz by adjust the initialization order (Paolo) >=20 > Hi Anthony, other maintainer >=20 > The V2 already reviewed and tested by Markus. Can you help to review & = apply it? Amos, I had pointed out to Paolo (IRC?) that this differs from how all legacy devices are being created, so I consider it a bad idea. qdev_set_parent_bus() is called from qdev_try_create(), which is called by qdev_create(). Devices may thus assume that the bus is set early, e.g. in their property setters invoked by qemu_opt_foreach(), and some functions have special behavior for a NULL bus (thinking of ISA here), so the change may lead to silent functional changes. Long-term we will have to move the code adding the device out of realize because we want to make realize work recursively on the composition tree. So what about rather moving the code adding the device to periph-anon / periph between dev->id and qemu_opt_foreach() so that the original unparenting works as expected? Regards, Andreas >=20 > Thanks, Amos >=20 >> --- >> qdev-monitor.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/qdev-monitor.c b/qdev-monitor.c >> index dc37a43..4070b0a 100644 >> --- a/qdev-monitor.c >> +++ b/qdev-monitor.c >> @@ -518,16 +518,11 @@ DeviceState *qdev_device_add(QemuOpts *opts) >> /* create device, set properties */ >> dev =3D DEVICE(object_new(driver)); >> =20 >> - if (bus) { >> - qdev_set_parent_bus(dev, bus); >> - } >> - >> id =3D qemu_opts_id(opts); >> if (id) { >> dev->id =3D id; >> } >> if (qemu_opt_foreach(opts, set_property, dev, 1) !=3D 0) { >> - object_unparent(OBJECT(dev)); >> object_unref(OBJECT(dev)); >> return NULL; >> } >> @@ -541,6 +536,11 @@ DeviceState *qdev_device_add(QemuOpts *opts) >> OBJECT(dev), NULL); >> g_free(name); >> } >> + >> + if (bus) { >> + qdev_set_parent_bus(dev, bus); >> + } >> + >> object_property_set_bool(OBJECT(dev), true, "realized", &err); >> if (err !=3D NULL) { >> qerror_report_err(err); >> --=20 >> 1.8.4.2 >> --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg