From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57380) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WNSLn-0006mi-0H for qemu-devel@nongnu.org; Tue, 11 Mar 2014 15:28:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WNSLg-0000wm-Ea for qemu-devel@nongnu.org; Tue, 11 Mar 2014 15:28:30 -0400 Received: from mx1.redhat.com ([209.132.183.28]:7821) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WNSLg-0000uI-6T for qemu-devel@nongnu.org; Tue, 11 Mar 2014 15:28:24 -0400 Message-ID: <1394566117.3981.68.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Tue, 11 Mar 2014 21:28:37 +0200 In-Reply-To: <531F3E59.7070904@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> <531F3E59.7070904@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, pbonzini@redhat.com, imammedo@redhat.com On Tue, 2014-03-11 at 17:48 +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 > My simple solution here is to revert my own patch addition. If /machine > exists, container.c:container_get() will use object_resolve_path(), jus= t > like my diff, to obtain the pre-existing object. And your addition of > the /machine child<> in vl.c actually uses error_abort, so would error > out if already added by qdev_get_machine(). This means that vl.c > actually works as intended and the unit test would continue to > implicitly create the /machine code without us needing to add more > workarounds. I completely agree, I liked the idea of "object_resolve_path" because the current code hides the *subtle* and *not straight forward* idea you just mentioned above. Meaning, it would be much easier to understand the code if it would just = work, but maybe it does not worth all the workarounds. So, no problem, just "un-squash" this, Thanks for the help, Marcel >=20 > Regards, > Andreas >=20 > >=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= ). 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