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

[Marcel Apfelbaum <marcel.a@redhat.com>]

On Fri, 2014-03-07 at 18:30 +0100, Andreas Färber wrote:
> 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 <marcel.a@redhat.com>
> >>>>
> >>>> 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.
> > 
> > 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 following hack fixes this particular failure for me (ran into it
> while trying to generate the HTML report):
> 
> diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.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);
> 
>      module_call_init(MODULE_INIT_QOM);
> +
> +    object_property_add_child(object_get_root(), "machine",
> object_new("container"), NULL);
> +
Great! Thanks! I was trying to create a null-machine, but for this scenario
a container is more than enough.

>      type_register_static(&static_prop_type);
>      type_register_static(&dynamic_prop_type);
> 
> 
> Not yet suitable for squashing obviously.
I tested it and make check passes for all architectures, so why not?
It seems elegant and not a hack (this scenario does not require an actual machine).

Thanks,
Marcel
> 
> Andreas
> 
> > 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
> > 
> >  
> > 
> >>
> >> Cheers,
> >> Andreas
> >>
> > 
> > 
> > 
> 
>