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>
---
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;
}
- 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,
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 <groug@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 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. 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, > >
On Thu, Jul 27, 2017 at 03:50:37PM -0500, Michael Roth 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... Yes, I think that's right. I've applied this fix to ppc-for-2.10. > > > > > 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> > > --- > > > > 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. > > > > > Since spapr_dr_connector_new() calls realize() with errp = NULL, we > 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. Well, except I think it should be &error_fatal, not &error_abort. But that can be a follow up patch. > > 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, > > > > > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Fri, 28 Jul 2017 14:27:45 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Thu, Jul 27, 2017 at 03:50:37PM -0500, Michael Roth 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... > > Yes, I think that's right. > > I've applied this fix to ppc-for-2.10. > > > > > > > > > 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> > > > --- > > > > > > 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. > > > > > > > > > Since spapr_dr_connector_new() calls realize() with errp = NULL, we > > 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. Well, except I think it should be &error_fatal, not > &error_abort. But that can be a follow up patch. > Hmmm... when we'll hotplug a PHB, an error during the realization of a PCI DRC would then terminate QEMU. I'd rather add an errp argument to spapr_dr_connector_new() and propagate the error instead. > > > > 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, > > > > > > > > >
On Wed, Aug 02, 2017 at 10:14:56AM +0200, Greg Kurz wrote: > On Fri, 28 Jul 2017 14:27:45 +1000 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Thu, Jul 27, 2017 at 03:50:37PM -0500, Michael Roth 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... > > > > Yes, I think that's right. > > > > I've applied this fix to ppc-for-2.10. > > > > > > > > > > > > > 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> > > > > --- > > > > > > > > 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. > > > > > > > > > > > > > Since spapr_dr_connector_new() calls realize() with errp = NULL, we > > > 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. Well, except I think it should be &error_fatal, not > > &error_abort. But that can be a follow up patch. > > > > Hmmm... when we'll hotplug a PHB, an error during the realization of > a PCI DRC would then terminate QEMU. I'd rather add an errp argument > to spapr_dr_connector_new() and propagate the error instead. Ah, good point. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
On Thu, 27 Jul 2017 15:50:37 -0500 Michael Roth <mdroth@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 <groug@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, > > > > >
© 2016 - 2024 Red Hat, Inc.