| Message ID | 20180613065707.30766-3-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | Better handling of machine specific per-cpu information | expand |
On 06/13/2018 08:57 AM, David Gibson wrote: > In pnv_core_realize() we call two functions with an Error * parameter in > succession, which means if they both cause errors we'll lose the first one. > Add an extra test/escape to fix this. I tend now to pass just NULL or &error_abort to object_property_add_child() and object_property_add_const_link(). These calls should just not fail. Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/pnv_core.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c > index 13ad7d9e04..efb68226bb 100644 > --- a/hw/ppc/pnv_core.c > +++ b/hw/ppc/pnv_core.c > @@ -173,6 +173,9 @@ static void pnv_core_realize(DeviceState *dev, Error **errp) > > snprintf(name, sizeof(name), "thread[%d]", i); > object_property_add_child(OBJECT(pc), name, obj, &local_err); > + if (local_err) { > + goto err; > + } > object_property_add_alias(obj, "core-pir", OBJECT(pc), > "pir", &local_err); > if (local_err) { >
On Wed, 13 Jun 2018 16:57:02 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > In pnv_core_realize() we call two functions with an Error * parameter in > succession, which means if they both cause errors we'll lose the first one. Not exactly. The error code doesn't allow that and QEMU will abort. static void error_setv(Error **errp, const char *src, int line, const char *func, ErrorClass err_class, const char *fmt, va_list ap, const char *suffix) { Error *err; int saved_errno = errno; if (errp == NULL) { return; } assert(*errp == NULL); > Add an extra test/escape to fix this. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/pnv_core.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c > index 13ad7d9e04..efb68226bb 100644 > --- a/hw/ppc/pnv_core.c > +++ b/hw/ppc/pnv_core.c > @@ -173,6 +173,9 @@ static void pnv_core_realize(DeviceState *dev, Error **errp) > > snprintf(name, sizeof(name), "thread[%d]", i); > object_property_add_child(OBJECT(pc), name, obj, &local_err); > + if (local_err) { > + goto err; > + } > object_property_add_alias(obj, "core-pir", OBJECT(pc), > "pir", &local_err); > if (local_err) { Hmm... the current error path seems to assume failures to be caused by object_property_add_child(). It hence unparents the previously parented CPUs, but not the current one. So we'll miss one call to object_unparent() if object_property_add_alias() fails.
On Wed, Jun 13, 2018 at 10:15:09AM +0200, Cédric Le Goater wrote: > On 06/13/2018 08:57 AM, David Gibson wrote: > > In pnv_core_realize() we call two functions with an Error * parameter in > > succession, which means if they both cause errors we'll lose the first one. > > Add an extra test/escape to fix this. > > I tend now to pass just NULL or &error_abort to object_property_add_child() > and object_property_add_const_link(). These calls should just not > fail. Hm, good point. Another day. > > Reviewed-by: Cédric Le Goater <clg@kaod.org> > > Thanks, > > C. > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > hw/ppc/pnv_core.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c > > index 13ad7d9e04..efb68226bb 100644 > > --- a/hw/ppc/pnv_core.c > > +++ b/hw/ppc/pnv_core.c > > @@ -173,6 +173,9 @@ static void pnv_core_realize(DeviceState *dev, Error **errp) > > > > snprintf(name, sizeof(name), "thread[%d]", i); > > object_property_add_child(OBJECT(pc), name, obj, &local_err); > > + if (local_err) { > > + goto err; > > + } > > object_property_add_alias(obj, "core-pir", OBJECT(pc), > > "pir", &local_err); > > if (local_err) { > > >
>> index 13ad7d9e04..efb68226bb 100644 >> --- a/hw/ppc/pnv_core.c >> +++ b/hw/ppc/pnv_core.c >> @@ -173,6 +173,9 @@ static void pnv_core_realize(DeviceState *dev, Error **errp) >> >> snprintf(name, sizeof(name), "thread[%d]", i); >> object_property_add_child(OBJECT(pc), name, obj, &local_err); >> + if (local_err) { >> + goto err; >> + } >> object_property_add_alias(obj, "core-pir", OBJECT(pc), >> "pir", &local_err); >> if (local_err) { > > Hmm... the current error path seems to assume failures to be > caused by object_property_add_child(). It hence unparents the > previously parented CPUs, but not the current one. So we'll > miss one call to object_unparent() if object_property_add_alias() > fails. yes, let's just put NULL or &error_abort instead. C.
On Wed, 13 Jun 2018 11:14:57 +0200 Cédric Le Goater <clg@kaod.org> wrote: > >> index 13ad7d9e04..efb68226bb 100644 > >> --- a/hw/ppc/pnv_core.c > >> +++ b/hw/ppc/pnv_core.c > >> @@ -173,6 +173,9 @@ static void pnv_core_realize(DeviceState *dev, Error **errp) > >> > >> snprintf(name, sizeof(name), "thread[%d]", i); > >> object_property_add_child(OBJECT(pc), name, obj, &local_err); > >> + if (local_err) { > >> + goto err; > >> + } > >> object_property_add_alias(obj, "core-pir", OBJECT(pc), > >> "pir", &local_err); > >> if (local_err) { > > > > Hmm... the current error path seems to assume failures to be > > caused by object_property_add_child(). It hence unparents the > > previously parented CPUs, but not the current one. So we'll > > miss one call to object_unparent() if object_property_add_alias() > > fails. > > yes, let's just put NULL or &error_abort instead. > NULL means we really don't care if the call fails or succeeds. &error_abort means we consider a failure to be a unrecoverable bug. So I would rather pass &error_abort here. But if the guest is already running and functional, and we hit the error during hotplug, does the guest really deserve to be aborted or should we just fail the hotplug ? > C.
On Wed, Jun 13, 2018 at 11:14:57AM +0200, Cédric Le Goater wrote: > >> index 13ad7d9e04..efb68226bb 100644 > >> --- a/hw/ppc/pnv_core.c > >> +++ b/hw/ppc/pnv_core.c > >> @@ -173,6 +173,9 @@ static void pnv_core_realize(DeviceState *dev, Error **errp) > >> > >> snprintf(name, sizeof(name), "thread[%d]", i); > >> object_property_add_child(OBJECT(pc), name, obj, &local_err); > >> + if (local_err) { > >> + goto err; > >> + } > >> object_property_add_alias(obj, "core-pir", OBJECT(pc), > >> "pir", &local_err); > >> if (local_err) { > > > > Hmm... the current error path seems to assume failures to be > > caused by object_property_add_child(). It hence unparents the > > previously parented CPUs, but not the current one. So we'll > > miss one call to object_unparent() if object_property_add_alias() > > fails. > > yes, let's just put NULL or &error_abort instead. Yeah, good idea, I'll change it in a new spin.
On Wed, Jun 13, 2018 at 11:42:07AM +0200, Greg Kurz wrote: > On Wed, 13 Jun 2018 11:14:57 +0200 > Cédric Le Goater <clg@kaod.org> wrote: > > > >> index 13ad7d9e04..efb68226bb 100644 > > >> --- a/hw/ppc/pnv_core.c > > >> +++ b/hw/ppc/pnv_core.c > > >> @@ -173,6 +173,9 @@ static void pnv_core_realize(DeviceState *dev, Error **errp) > > >> > > >> snprintf(name, sizeof(name), "thread[%d]", i); > > >> object_property_add_child(OBJECT(pc), name, obj, &local_err); > > >> + if (local_err) { > > >> + goto err; > > >> + } > > >> object_property_add_alias(obj, "core-pir", OBJECT(pc), > > >> "pir", &local_err); > > >> if (local_err) { > > > > > > Hmm... the current error path seems to assume failures to be > > > caused by object_property_add_child(). It hence unparents the > > > previously parented CPUs, but not the current one. So we'll > > > miss one call to object_unparent() if object_property_add_alias() > > > fails. > > > > yes, let's just put NULL or &error_abort instead. > > > > NULL means we really don't care if the call fails or succeeds. > > &error_abort means we consider a failure to be a unrecoverable bug. > > So I would rather pass &error_abort here. > > But if the guest is already running and functional, and we hit > the error during hotplug, does the guest really deserve to be > aborted or should we just fail the hotplug ? Ah, dammit, that's why it wasn't an abort in the first place. Yeah, we'd better propagate the errors.
On Wed, Jun 13, 2018 at 07:53:29PM +1000, David Gibson wrote: > On Wed, Jun 13, 2018 at 11:42:07AM +0200, Greg Kurz wrote: > > On Wed, 13 Jun 2018 11:14:57 +0200 > > Cédric Le Goater <clg@kaod.org> wrote: > > > > > >> index 13ad7d9e04..efb68226bb 100644 > > > >> --- a/hw/ppc/pnv_core.c > > > >> +++ b/hw/ppc/pnv_core.c > > > >> @@ -173,6 +173,9 @@ static void pnv_core_realize(DeviceState *dev, Error **errp) > > > >> > > > >> snprintf(name, sizeof(name), "thread[%d]", i); > > > >> object_property_add_child(OBJECT(pc), name, obj, &local_err); > > > >> + if (local_err) { > > > >> + goto err; > > > >> + } > > > >> object_property_add_alias(obj, "core-pir", OBJECT(pc), > > > >> "pir", &local_err); > > > >> if (local_err) { > > > > > > > > Hmm... the current error path seems to assume failures to be > > > > caused by object_property_add_child(). It hence unparents the > > > > previously parented CPUs, but not the current one. So we'll > > > > miss one call to object_unparent() if object_property_add_alias() > > > > fails. > > > > > > yes, let's just put NULL or &error_abort instead. > > > > > > > NULL means we really don't care if the call fails or succeeds. > > > > &error_abort means we consider a failure to be a unrecoverable bug. > > > > So I would rather pass &error_abort here. > > > > But if the guest is already running and functional, and we hit > > the error during hotplug, does the guest really deserve to be > > aborted or should we just fail the hotplug ? > > Ah, dammit, that's why it wasn't an abort in the first place. Yeah, > we'd better propagate the errors. No.. thinking about this yet again, we should be ok with error_abort. These really aren't things that should fail. If they do something has gone so horribly wrong, that I think an abort() is a reasonable reaction, even on hotplug.
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c index 13ad7d9e04..efb68226bb 100644 --- a/hw/ppc/pnv_core.c +++ b/hw/ppc/pnv_core.c @@ -173,6 +173,9 @@ static void pnv_core_realize(DeviceState *dev, Error **errp) snprintf(name, sizeof(name), "thread[%d]", i); object_property_add_child(OBJECT(pc), name, obj, &local_err); + if (local_err) { + goto err; + } object_property_add_alias(obj, "core-pir", OBJECT(pc), "pir", &local_err); if (local_err) {
In pnv_core_realize() we call two functions with an Error * parameter in succession, which means if they both cause errors we'll lose the first one. Add an extra test/escape to fix this. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/ppc/pnv_core.c | 3 +++ 1 file changed, 3 insertions(+)