From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46516) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WNOsF-0001hK-SM for qemu-devel@nongnu.org; Tue, 11 Mar 2014 11:45:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WNOs4-0007kP-OU for qemu-devel@nongnu.org; Tue, 11 Mar 2014 11:45:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55073) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WNOs4-0007kA-FT for qemu-devel@nongnu.org; Tue, 11 Mar 2014 11:45:36 -0400 Message-ID: <1394552693.3981.51.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Tue, 11 Mar 2014 17:44:53 +0200 In-Reply-To: <531A0220.7030405@suse.de> References: <1394040647-20083-1-git-send-email-marcel.a@redhat.com> <1394040647-20083-4-git-send-email-marcel.a@redhat.com> <5319087A.3090403@suse.de> <1394170376.2663.24.camel@localhost.localdomain> <5319AD0C.9060800@suse.de> <1394209338.2663.51.camel@localhost.localdomain> <531A0220.7030405@suse.de> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 3/3] hw/boards: converted current_machine to be an instance of MachineCLass List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?ISO-8859-1?Q?F=E4rber?= Cc: qemu-devel@nongnu.org, ehabkost@redhat.com, mst@redhat.com, Alexey Kardashevskiy , armbru@redhat.com, lcapitulino@redhat.com, blauwirbel@gmail.com, aliguori@amazon.com, imammedo@redhat.com, pbonzini@redhat.com On Fri, 2014-03-07 at 18:30 +0100, Andreas F=C3=A4rber wrote: > Am 07.03.2014 17:22, schrieb Marcel Apfelbaum: > > On Fri, 2014-03-07 at 12:27 +0100, Andreas F=C3=A4rber wrote: > >> Am 07.03.2014 06:32, schrieb Marcel Apfelbaum: > >>> On Fri, 2014-03-07 at 00:44 +0100, Andreas F=C3=A4rber wrote: > >>>> Am 05.03.2014 18:30, schrieb Marcel Apfelbaum: > >>>>> In order to allow attaching machine options to a machine instance= , > >>>>> current_machine is converted into MachineState. > >>>>> As a first step of deprecating QEMUMachine, some of the functions > >>>>> were modified to return MachineCLass. > >>>>> > >>>>> Signed-off-by: Marcel Apfelbaum > >>>> > >>>> Looks mostly good, but same issue as Alexey's patch: We are riskin= g > >>>> qdev_get_machine() creating a Container-typed /machine node. > >>>> > >>>> What about the following on top? > >>> Hi Andreas, > >>> > >>> I checked with the debugger and qdev_get_machine is called > >>> long after we add the machine to the QOM tree. > >>> However, the race still exists as someone can call qdev_get_machine > >>> before the machine is added to the tree, not being aware of that. > >>> > >>> Your change solves the problem, thank you! > >>> Do you want me to add this diff and resend, > >>> or I will send yours separately? > >> > >> My preference would be to avoid another round of review on my part b= y > >> simply squashing into your 3/3. > > There is a problem with it: 'make check fails' on test-qdev-global-pr= ops. > > - 'qdev_get_machine()' is called by 'device_set_realized()' because s= tatic_prop_type > > has TYPE_DEVICE as parent. > > - The machine is added to the QOM tree *only in vl's main* and this t= est does not > > reach it, but assumes that always will be a machine in the QOM tree. > > This is no longer true. > >=20 > > Possible solution would be making existing 'null machine' a subclass = of MachineClass > > and add it manually to QOM on this test(and other places as necessary= ). >=20 > The following hack fixes this particular failure for me (ran into it > while trying to generate the HTML report): >=20 > diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-pr= ops.c > index e4ad173..31bac15 100644 > --- a/tests/test-qdev-global-props.c > +++ b/tests/test-qdev-global-props.c > @@ -167,6 +167,9 @@ int main(int argc, char **argv) > g_test_init(&argc, &argv, NULL); >=20 > module_call_init(MODULE_INIT_QOM); > + > + object_property_add_child(object_get_root(), "machine", > object_new("container"), NULL); > + > type_register_static(&static_prop_type); > type_register_static(&dynamic_prop_type); >=20 >=20 > Not yet suitable for squashing obviously. Hi Andreas, I found the reason why the 'make check' is getting stuck, it was related to a bug in qtest library. I posted a patch that fixes this: [PATCH V4] tests/libqtest: Fix possible deadlock in qtest initializat= ion I saw that you already squashed "object_resolve_path" into qdev_get_machi= ne. I just sent [PATCH 0/3] tests/qdev-global-props: fixes due to machine conversion = to QOM which takes care of the 'make check' failures. I think is OK now. Thanks, Marcel =20 >=20 > Andreas >=20 > > The risk here is > > that there are other places where the machine needs to be added manua= lly to the QOM tree. > > (I am trying this option, but make check gets stuck !!!, debugging) > >=20 > > Other possible solution is to add some kind of "CONFIG_MACHINE_IS_QOM= _OBJECT" define > > and use this in qdev_get_machine() implementation. (ugly?) > >=20 > > Finally, is possible to be aware that may be a race when doing code r= eview. ("dangerous"?) > >=20 > > Any thoughts? > > Thanks,=20 > > Marcel > >=20 > > =20 > >=20 > >> > >> Cheers, > >> Andreas > >> > >=20 > >=20 > >=20 >=20 >=20