| Message ID | 157383332103.165747.15204186097237659466.stgit@bahia.lan |
|---|---|
| Headers | show |
| Series | ppc: Consolidate QOM links and pointers to the same object | expand |
On Fri, Nov 15, 2019 at 04:55:21PM +0100, Greg Kurz wrote: > There's a recurring pattern in the code where a const link is added to a > newly instanciated object and the link is then used in the object's realize > function to keep a pointer to the QOM entity which the link points to. > > void create_obj_b(Object *obj_a) > { > Object *obj_b; > > obj_b = object_new(TYPE_B); > object_property_add_const_link(obj_b, "link-to-a", obj_a, &error_abort); > object_property_set_bool(obj_b, true, "realized", &error_abort); > } > > void object_b_realize(DeviceState *dev, Error **errp) > { > Object *obj_a; > > obj_a = object_property_get_link(OBJECT(dev), "link-to-a", errp); > if (!obj_a) { > return; > } > > obj_b->obj_a = A(obj_a); // If obj_b->obj_a is changed, the link property > // still points to the original obj_a that was > // passed to object_property_add_const_link() > } > > Confusing bugs could arise if the pointer and the link go out of sync for > some reason. This can be avoided if the property is defined to directly use > the pointer with the DEFINE_PROP_LINK() macro. > > This series just does that for all occurences of the fragile pattern in > the XIVE and PNV code. > > Changes in v2: > - use DEFINE_PROP_LINK() instead of object_property_add_link() > - dropped public -> private changes in type definitions Applied to ppc-for-5.0.
Greg Kurz <groug@kaod.org> writes: > There's a recurring pattern in the code where a const link is added to a > newly instanciated object and the link is then used in the object's realize > function to keep a pointer to the QOM entity which the link points to. > > void create_obj_b(Object *obj_a) > { > Object *obj_b; > > obj_b = object_new(TYPE_B); > object_property_add_const_link(obj_b, "link-to-a", obj_a, &error_abort); > object_property_set_bool(obj_b, true, "realized", &error_abort); > } > > void object_b_realize(DeviceState *dev, Error **errp) > { > Object *obj_a; > > obj_a = object_property_get_link(OBJECT(dev), "link-to-a", errp); > if (!obj_a) { > return; > } > > obj_b->obj_a = A(obj_a); // If obj_b->obj_a is changed, the link property > // still points to the original obj_a that was > // passed to object_property_add_const_link() > } > > Confusing bugs could arise if the pointer and the link go out of sync for > some reason. This can be avoided if the property is defined to directly use > the pointer with the DEFINE_PROP_LINK() macro. > > This series just does that for all occurences of the fragile pattern in > the XIVE and PNV code. Have you looked for the pattern elsewhere?
On 18/11/2019 10:26, Markus Armbruster wrote: > Greg Kurz <groug@kaod.org> writes: > >> There's a recurring pattern in the code where a const link is added to a >> newly instanciated object and the link is then used in the object's realize >> function to keep a pointer to the QOM entity which the link points to. >> >> void create_obj_b(Object *obj_a) >> { >> Object *obj_b; >> >> obj_b = object_new(TYPE_B); >> object_property_add_const_link(obj_b, "link-to-a", obj_a, &error_abort); >> object_property_set_bool(obj_b, true, "realized", &error_abort); >> } >> >> void object_b_realize(DeviceState *dev, Error **errp) >> { >> Object *obj_a; >> >> obj_a = object_property_get_link(OBJECT(dev), "link-to-a", errp); >> if (!obj_a) { >> return; >> } >> >> obj_b->obj_a = A(obj_a); // If obj_b->obj_a is changed, the link property >> // still points to the original obj_a that was >> // passed to object_property_add_const_link() >> } >> >> Confusing bugs could arise if the pointer and the link go out of sync for >> some reason. This can be avoided if the property is defined to directly use >> the pointer with the DEFINE_PROP_LINK() macro. >> >> This series just does that for all occurences of the fragile pattern in >> the XIVE and PNV code. > > Have you looked for the pattern elsewhere? I can take care of the Aspeed machine. I followed the same pattern there. C.
On Mon, 18 Nov 2019 10:26:12 +0100 Markus Armbruster <armbru@redhat.com> wrote: > Greg Kurz <groug@kaod.org> writes: > > > There's a recurring pattern in the code where a const link is added to a > > newly instanciated object and the link is then used in the object's realize > > function to keep a pointer to the QOM entity which the link points to. > > > > void create_obj_b(Object *obj_a) > > { > > Object *obj_b; > > > > obj_b = object_new(TYPE_B); > > object_property_add_const_link(obj_b, "link-to-a", obj_a, &error_abort); > > object_property_set_bool(obj_b, true, "realized", &error_abort); > > } > > > > void object_b_realize(DeviceState *dev, Error **errp) > > { > > Object *obj_a; > > > > obj_a = object_property_get_link(OBJECT(dev), "link-to-a", errp); > > if (!obj_a) { > > return; > > } > > > > obj_b->obj_a = A(obj_a); // If obj_b->obj_a is changed, the link property > > // still points to the original obj_a that was > > // passed to object_property_add_const_link() > > } > > > > Confusing bugs could arise if the pointer and the link go out of sync for > > some reason. This can be avoided if the property is defined to directly use > > the pointer with the DEFINE_PROP_LINK() macro. > > > > This series just does that for all occurences of the fragile pattern in > > the XIVE and PNV code. > > Have you looked for the pattern elsewhere? > No I was focusing on the XICS/XIVE interrupt controller code for PPC machines only. I'll have a broader look.
There's a recurring pattern in the code where a const link is added to a newly instanciated object and the link is then used in the object's realize function to keep a pointer to the QOM entity which the link points to. void create_obj_b(Object *obj_a) { Object *obj_b; obj_b = object_new(TYPE_B); object_property_add_const_link(obj_b, "link-to-a", obj_a, &error_abort); object_property_set_bool(obj_b, true, "realized", &error_abort); } void object_b_realize(DeviceState *dev, Error **errp) { Object *obj_a; obj_a = object_property_get_link(OBJECT(dev), "link-to-a", errp); if (!obj_a) { return; } obj_b->obj_a = A(obj_a); // If obj_b->obj_a is changed, the link property // still points to the original obj_a that was // passed to object_property_add_const_link() } Confusing bugs could arise if the pointer and the link go out of sync for some reason. This can be avoided if the property is defined to directly use the pointer with the DEFINE_PROP_LINK() macro. This series just does that for all occurences of the fragile pattern in the XIVE and PNV code. Changes in v2: - use DEFINE_PROP_LINK() instead of object_property_add_link() - dropped public -> private changes in type definitions -- Greg --- Greg Kurz (8): xive: Link "cpu" property to XiveTCTX::cs pointer xive: Link "xive" property to XiveSource::xive pointer xive: Link "xive" property to XiveEndSource::xrtr pointer ppc/pnv: Link "psi" property to PnvLpc::psi pointer ppc/pnv: Link "psi" property to PnvOCC::psi pointer ppc/pnv: Link "chip" property to PnvHomer::chip pointer ppc/pnv: Link "chip" property to PnvCore::chip pointer ppc/pnv: Link "chip" property to PnvXive::chip pointer hw/intc/pnv_xive.c | 21 +++++++-------------- hw/intc/spapr_xive.c | 8 ++++---- hw/intc/xive.c | 48 +++++++++++++++--------------------------------- hw/ppc/pnv.c | 32 ++++++++++++++++---------------- hw/ppc/pnv_core.c | 10 ++-------- hw/ppc/pnv_homer.c | 20 ++++++++++---------- hw/ppc/pnv_lpc.c | 19 ++++++++----------- hw/ppc/pnv_occ.c | 20 +++++++++----------- hw/ppc/pnv_psi.c | 3 +-- 9 files changed, 72 insertions(+), 109 deletions(-)