Re: [Qemu-devel] [PATCH v3 3/3] hw/boards: converted current_machine to be an instance of MachineCLass

[Andreas Färber]

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
workarounds.

Regards,
Andreas

> 
> 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