From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36931) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WNPr1-0006hj-FE for qemu-devel@nongnu.org; Tue, 11 Mar 2014 12:48:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WNPqu-0006ke-4x for qemu-devel@nongnu.org; Tue, 11 Mar 2014 12:48:35 -0400 Received: from cantor2.suse.de ([195.135.220.15]:50100 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WNPqt-0006gl-Rc for qemu-devel@nongnu.org; Tue, 11 Mar 2014 12:48:28 -0400 Message-ID: <531F3E59.7070904@suse.de> Date: Tue, 11 Mar 2014 17:48:25 +0100 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 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> In-Reply-To: <1394209338.2663.51.camel@localhost.localdomain> Content-Type: text/plain; charset=UTF-8 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: Marcel Apfelbaum 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 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 risking >>>> 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 by >> simply squashing into your 3/3. > There is a problem with it: 'make check fails' on test-qdev-global-prop= s. > - 'qdev_get_machine()' is called by 'device_set_realized()' because sta= tic_prop_type > has TYPE_DEVICE as parent. > - The machine is added to the QOM tree *only in vl's main* and this tes= t does not > reach it, but assumes that always will be a machine in the QOM tree. > This is no longer true. My simple solution here is to revert my own patch addition. If /machine exists, container.c:container_get() will use object_resolve_path(), just 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. Regards, Andreas >=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 manuall= y 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_O= BJECT" 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 rev= iew. ("dangerous"?) >=20 > Any thoughts? > Thanks,=20 > Marcel --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg