On Thu, 27 Jul 2017 15:50:37 -0500 Michael Roth <mdr...@linux.vnet.ibm.com> wrote:
> Quoting Greg Kurz (2017-07-27 08:45:47)
> > If object_property_add_alias() returns an error in realize(), we should
> > propagate it to the caller and certainly not unref the DRC.
>
> Indeed. I think that was the result of this part of the code
> originally living in spapr_dr_connector_new() during development,
> where it had previously made sense to free the object if there was a
> failure and then return NULL. We definitely shouldn't be during it
> now...
>
> >
> > Same thing goes for unrealize(). Since object_property_del() is the last
> > call, we can even get rid of the intermediate Error *.
> >
> > And finally, unrealize() should undo all registrations performed by
> > realize().
> >
> > Signed-off-by: Greg Kurz <gr...@kaod.org>
> > ---
> >
> > David,
> >
> > As agreed on the list, here's the version of the fix for 2.10. I'll respin
> > my PHB hotplug series on top of it.
> >
> > Cheers,
> >
> > --
> > Greg
> >
> > hw/ppc/spapr_drc.c | 15 ++++++---------
> > 1 file changed, 6 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 15bae5c216a9..47d94e782ac2 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -506,11 +506,11 @@ static void realize(DeviceState *d, Error **errp)
> > trace_spapr_drc_realize_child(spapr_drc_index(drc), child_name);
> > object_property_add_alias(root_container, link_name,
> > drc->owner, child_name, &err);
> > + g_free(child_name);
> > if (err) {
> > - error_report_err(err);
> > - object_unref(OBJECT(drc));
> > + error_propagate(errp, err);
> > + return;
>
>
> Since spapr_dr_connector_new() calls realize() with errp = NULL, we
Yeah, there are a many of places in the code where we still pass a NULL
errp... :-\
> basically lose the error now. I think maybe we should at least report it
> still, but even better would be to have _new() call object_property_* with
> error_abort, since callers of spapr_dr_connector_new() don't all check
> for the return value and even if they did the appropriate action would
> always be to report+abort() anyway.
>
I agree.
> Looks good otherwise.
>
> > }
> > - g_free(child_name);
> > vmstate_register(DEVICE(drc), spapr_drc_index(drc), &vmstate_spapr_drc,
> > drc);
> > qemu_register_reset(drc_reset, drc);
> > @@ -522,16 +522,13 @@ static void unrealize(DeviceState *d, Error **errp)
> > sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d);
> > Object *root_container;
> > char name[256];
> > - Error *err = NULL;
> >
> > trace_spapr_drc_unrealize(spapr_drc_index(drc));
> > + qemu_unregister_reset(drc_reset, drc);
> > + vmstate_unregister(DEVICE(drc), &vmstate_spapr_drc, drc);
> > root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
> > snprintf(name, sizeof(name), "%x", spapr_drc_index(drc));
> > - object_property_del(root_container, name, &err);
> > - if (err) {
> > - error_report_err(err);
> > - object_unref(OBJECT(drc));
> > - }
> > + object_property_del(root_container, name, errp);
> > }
> >
> > sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
> >
> >
>
pgp8uuJ34OfFd.pgp
Description: OpenPGP digital signature
