| Message ID | 5319087A.3090403@suse.de |
|---|---|
| State | New |
| Headers | show |
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? Thanks, Marcel > > Alexey, if we reach agreement here, this means for you that we can just > use type_register_static() in place of qemu_machine_register() to > register your custom machine type with interface added. > > Regards, > Andreas > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index b6deebd..749c83a 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -861,7 +861,8 @@ Object *qdev_get_machine(void) > static Object *dev; > > if (dev == NULL) { > - dev = container_get(object_get_root(), "/machine"); > + dev = object_resolve_path("/machine", NULL); > + g_assert(dev); > } > > return dev; > >
Il 07/03/2014 00:44, Andreas Färber ha scritto: > 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? Good idea! The smallest patches are (almost) always the best. :) Paolo > Alexey, if we reach agreement here, this means for you that we can just > use type_register_static() in place of qemu_machine_register() to > register your custom machine type with interface added. > > Regards, > Andreas > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index b6deebd..749c83a 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -861,7 +861,8 @@ Object *qdev_get_machine(void) > static Object *dev; > > if (dev == NULL) { > - dev = container_get(object_get_root(), "/machine"); > + dev = object_resolve_path("/machine", NULL); > + g_assert(dev); > } > > return dev; > >
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. Cheers, Andreas
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 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 >
Il 07/03/2014 17:22, Marcel Apfelbaum ha scritto: > 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 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) This is probably the right thing to do, but I guess it means it's better to leave this series to 2.1. Paolo
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. 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
On Tue, 2014-03-11 at 17:48 +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. > > 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. I completely agree, I liked the idea of "object_resolve_path" because the current code hides the *subtle* and *not straight forward* idea you just mentioned above. Meaning, it would be much easier to understand the code if it would just work, but maybe it does not worth all the workarounds. So, no problem, just "un-squash" this, Thanks for the help, Marcel > > 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 >
diff --git a/hw/core/qdev.c b/hw/core/qdev.c index b6deebd..749c83a 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -861,7 +861,8 @@ Object *qdev_get_machine(void) static Object *dev; if (dev == NULL) { - dev = container_get(object_get_root(), "/machine"); + dev = object_resolve_path("/machine", NULL); + g_assert(dev); } return dev;