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 <marce...@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);
> +
> type_register_static(&static_prop_type);
> type_register_static(&dynamic_prop_type);
>
>
> Not yet suitable for squashing obviously.
Hi Andreas,
I found the reason why the 'make check' is getting stuck, it was
related to a bug in qtest library. I posted a patch that fixes this:
[PATCH V4] tests/libqtest: Fix possible deadlock in qtest initialization
I saw that you already squashed "object_resolve_path" into qdev_get_machine.
I just sent
[PATCH 0/3] tests/qdev-global-props: fixes due to machine conversion to QOM
which takes care of the 'make check' failures.
I think is OK now.
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
> >>
> >
> >
> >
>
>
