* [Qemu-devel] [for-2.10 PATCH] spapr_drc: fix realize and unrealize
@ 2017-07-27 13:45 Greg Kurz
2017-07-27 20:50 ` Michael Roth
0 siblings, 1 reply; 6+ messages in thread
From: Greg Kurz @ 2017-07-27 13:45 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-ppc, David Gibson
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,
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [for-2.10 PATCH] spapr_drc: fix realize and unrealize
2017-07-27 13:45 [Qemu-devel] [for-2.10 PATCH] spapr_drc: fix realize and unrealize Greg Kurz
@ 2017-07-27 20:50 ` Michael Roth
2017-07-28 4:27 ` David Gibson
2017-07-28 10:01 ` Greg Kurz
0 siblings, 2 replies; 6+ messages in thread
From: Michael Roth @ 2017-07-27 20:50 UTC (permalink / raw)
To: Greg Kurz, qemu-devel; +Cc: qemu-ppc, David Gibson
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,
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [for-2.10 PATCH] spapr_drc: fix realize and unrealize
2017-07-27 20:50 ` Michael Roth
@ 2017-07-28 4:27 ` David Gibson
2017-08-02 8:14 ` Greg Kurz
2017-07-28 10:01 ` Greg Kurz
1 sibling, 1 reply; 6+ messages in thread
From: David Gibson @ 2017-07-28 4:27 UTC (permalink / raw)
To: Michael Roth; +Cc: Greg Kurz, qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 2933 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [for-2.10 PATCH] spapr_drc: fix realize and unrealize
2017-07-27 20:50 ` Michael Roth
2017-07-28 4:27 ` David Gibson
@ 2017-07-28 10:01 ` Greg Kurz
1 sibling, 0 replies; 6+ messages in thread
From: Greg Kurz @ 2017-07-28 10:01 UTC (permalink / raw)
To: Michael Roth; +Cc: qemu-devel, qemu-ppc, David Gibson
[-- Attachment #1: Type: text/plain, Size: 3470 bytes --]
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,
> >
> >
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [for-2.10 PATCH] spapr_drc: fix realize and unrealize
2017-07-28 4:27 ` David Gibson
@ 2017-08-02 8:14 ` Greg Kurz
2017-08-02 14:22 ` David Gibson
0 siblings, 1 reply; 6+ messages in thread
From: Greg Kurz @ 2017-08-02 8:14 UTC (permalink / raw)
To: David Gibson; +Cc: Michael Roth, qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 3204 bytes --]
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,
> > >
> > >
> >
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [for-2.10 PATCH] spapr_drc: fix realize and unrealize
2017-08-02 8:14 ` Greg Kurz
@ 2017-08-02 14:22 ` David Gibson
0 siblings, 0 replies; 6+ messages in thread
From: David Gibson @ 2017-08-02 14:22 UTC (permalink / raw)
To: Greg Kurz; +Cc: Michael Roth, qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 2371 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-08-02 14:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-27 13:45 [Qemu-devel] [for-2.10 PATCH] spapr_drc: fix realize and unrealize Greg Kurz
2017-07-27 20:50 ` Michael Roth
2017-07-28 4:27 ` David Gibson
2017-08-02 8:14 ` Greg Kurz
2017-08-02 14:22 ` David Gibson
2017-07-28 10:01 ` Greg Kurz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).