| Message ID | 150100557147.27487.5031800506520955290.stgit@bahia |
|---|---|
| State | New |
| Headers | show |
On Tue, Jul 25, 2017 at 07:59:31PM +0200, Greg Kurz wrote: > If object_property_add_alias() returns an error in realize(), we should > propagate it to the caller and certainly not unref the DRC. > > 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 <groug@kaod.org> I've applied this to ppc-for-2.11, but this looks like it could be a real bug fix. So I'm wondering if I should push it for 2.10 (we're in hard freeze, but bugfixes can still be applied). > --- > 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 e4e8383ec7b5..d72453bcb42f 100644 > --- a/hw/ppc/spapr_drc.c > +++ b/hw/ppc/spapr_drc.c > @@ -506,12 +506,12 @@ 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); > g_free(link_name); > if (err) { > - error_report_err(err); > - object_unref(OBJECT(drc)); > + error_propagate(errp, err); > + return; > } > - g_free(child_name); > vmstate_register(DEVICE(drc), spapr_drc_index(drc), &vmstate_spapr_drc, > drc); > qemu_register_reset(drc_reset, drc); > @@ -523,17 +523,14 @@ static void unrealize(DeviceState *d, Error **errp) > sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d); > Object *root_container; > gchar *name; > - 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); > name = g_strdup_printf("%x", spapr_drc_index(drc)); > - object_property_del(root_container, name, &err); > + object_property_del(root_container, name, errp); > g_free(name); > - if (err) { > - error_report_err(err); > - object_unref(OBJECT(drc)); > - } > } > > sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type, >
On Wed, 26 Jul 2017 14:04:59 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Tue, Jul 25, 2017 at 07:59:31PM +0200, Greg Kurz wrote: > > If object_property_add_alias() returns an error in realize(), we should > > propagate it to the caller and certainly not unref the DRC. > > > > 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 <groug@kaod.org> > > I've applied this to ppc-for-2.11, but this looks like it could be a > real bug fix. So I'm wondering if I should push it for 2.10 (we're in > hard freeze, but bugfixes can still be applied). > Yeah I guess it should also be merged in 2.10 but it has a dependency with patch 4 (spapr_drc: use g_strdup_printf() instead of snprintf()). If you prefer, maybe I can repost this as a standalone fix for 2.10 ? > > --- > > 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 e4e8383ec7b5..d72453bcb42f 100644 > > --- a/hw/ppc/spapr_drc.c > > +++ b/hw/ppc/spapr_drc.c > > @@ -506,12 +506,12 @@ 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); > > g_free(link_name); > > if (err) { > > - error_report_err(err); > > - object_unref(OBJECT(drc)); > > + error_propagate(errp, err); > > + return; > > } > > - g_free(child_name); > > vmstate_register(DEVICE(drc), spapr_drc_index(drc), &vmstate_spapr_drc, > > drc); > > qemu_register_reset(drc_reset, drc); > > @@ -523,17 +523,14 @@ static void unrealize(DeviceState *d, Error **errp) > > sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d); > > Object *root_container; > > gchar *name; > > - 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); > > name = g_strdup_printf("%x", spapr_drc_index(drc)); > > - object_property_del(root_container, name, &err); > > + object_property_del(root_container, name, errp); > > g_free(name); > > - if (err) { > > - error_report_err(err); > > - object_unref(OBJECT(drc)); > > - } > > } > > > > sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type, > > >
On Wed, Jul 26, 2017 at 11:36:43AM +0200, Greg Kurz wrote: > On Wed, 26 Jul 2017 14:04:59 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Tue, Jul 25, 2017 at 07:59:31PM +0200, Greg Kurz wrote: > > > If object_property_add_alias() returns an error in realize(), we should > > > propagate it to the caller and certainly not unref the DRC. > > > > > > 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 <groug@kaod.org> > > > > I've applied this to ppc-for-2.11, but this looks like it could be a > > real bug fix. So I'm wondering if I should push it for 2.10 (we're in > > hard freeze, but bugfixes can still be applied). > > > > Yeah I guess it should also be merged in 2.10 but it has a dependency > with patch 4 (spapr_drc: use g_strdup_printf() instead of snprintf()). > If you prefer, maybe I can repost this as a standalone fix for 2.10 > ? Yes, that would be helpful, thank you. > > > > --- > > > 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 e4e8383ec7b5..d72453bcb42f 100644 > > > --- a/hw/ppc/spapr_drc.c > > > +++ b/hw/ppc/spapr_drc.c > > > @@ -506,12 +506,12 @@ 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); > > > g_free(link_name); > > > if (err) { > > > - error_report_err(err); > > > - object_unref(OBJECT(drc)); > > > + error_propagate(errp, err); > > > + return; > > > } > > > - g_free(child_name); > > > vmstate_register(DEVICE(drc), spapr_drc_index(drc), &vmstate_spapr_drc, > > > drc); > > > qemu_register_reset(drc_reset, drc); > > > @@ -523,17 +523,14 @@ static void unrealize(DeviceState *d, Error **errp) > > > sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d); > > > Object *root_container; > > > gchar *name; > > > - 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); > > > name = g_strdup_printf("%x", spapr_drc_index(drc)); > > > - object_property_del(root_container, name, &err); > > > + object_property_del(root_container, name, errp); > > > g_free(name); > > > - if (err) { > > > - error_report_err(err); > > > - object_unref(OBJECT(drc)); > > > - } > > > } > > > > > > sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type, > > > > > >
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index e4e8383ec7b5..d72453bcb42f 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -506,12 +506,12 @@ 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); g_free(link_name); if (err) { - error_report_err(err); - object_unref(OBJECT(drc)); + error_propagate(errp, err); + return; } - g_free(child_name); vmstate_register(DEVICE(drc), spapr_drc_index(drc), &vmstate_spapr_drc, drc); qemu_register_reset(drc_reset, drc); @@ -523,17 +523,14 @@ static void unrealize(DeviceState *d, Error **errp) sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(d); Object *root_container; gchar *name; - 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); name = g_strdup_printf("%x", spapr_drc_index(drc)); - object_property_del(root_container, name, &err); + object_property_del(root_container, name, errp); g_free(name); - if (err) { - error_report_err(err); - object_unref(OBJECT(drc)); - } } sPAPRDRConnector *spapr_dr_connector_new(Object *owner, const char *type,
If object_property_add_alias() returns an error in realize(), we should propagate it to the caller and certainly not unref the DRC. 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 <groug@kaod.org> --- hw/ppc/spapr_drc.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)