[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v3 3/3] hw/boards: converted current_machine to

From: Andreas Färber
Subject: Re: [Qemu-devel] [PATCH v3 3/3] hw/boards: converted current_machine to be an instance of MachineCLass
Date: Tue, 11 Mar 2014 17:48:25 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.3.0

Am 07.03.2014 17:22, schrieb Marcel Apfelbaum:
> On Fri, 2014-03-07 at 12:27 +0100, Andreas Färber wrote:
>> Am 07.03.2014 06:32, schrieb Marcel Apfelbaum:
>>> On Fri, 2014-03-07 at 00:44 +0100, Andreas Färber 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 <address@hidden>
>>>> 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-props.
> - 'qdev_get_machine()' is called by 'device_set_realized()' because 
> static_prop_type
>   has TYPE_DEVICE as parent.
> - The machine is added to the QOM tree *only in vl's main* and this test 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


> 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 manually to 
> the QOM tree.
> (I am trying this option, but make check gets stuck !!!, debugging)
> Other possible solution is to add some kind of "CONFIG_MACHINE_IS_QOM_OBJECT" 
> define
> and use this in qdev_get_machine() implementation. (ugly?)
> Finally, is possible to be aware that may be a race when doing code review. 
> ("dangerous"?)
> Any thoughts?
> Thanks, 
> Marcel

SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

reply via email to

[Prev in Thread] Current Thread [Next in Thread]