1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH 0/7] Better handling of machine specific per-cpu information
  4. View patch

[Qemu-devel] [PATCH 2/7] pnv: Add missing error check during cpu realize()

David Gibson posted 7 patches 7 years, 8 months ago
[Qemu-devel] [PATCH 2/7] pnv: Add missing error check during cpu realize()
Posted by David Gibson 7 years, 8 months ago 
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(+)

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) {
-- 
2.17.1
Re: [Qemu-devel] [PATCH 2/7] pnv: Add missing error check during cpu realize()
Posted by Cédric Le Goater 7 years, 8 months ago 
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) {
>
Re: [Qemu-devel] [PATCH 2/7] pnv: Add missing error check during cpu realize()
Posted by David Gibson 7 years, 8 months ago 
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) {
> > 
> 

-- 
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
Re: [Qemu-devel] [PATCH 2/7] pnv: Add missing error check during cpu realize()
Posted by Greg Kurz 7 years, 8 months ago 
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.
Re: [Qemu-devel] [PATCH 2/7] pnv: Add missing error check during cpu realize()
Posted by Cédric Le Goater 7 years, 8 months ago 
>> 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.
Re: [Qemu-devel] [PATCH 2/7] pnv: Add missing error check during cpu realize()
Posted by Greg Kurz 7 years, 8 months ago 
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.
Re: [Qemu-devel] [PATCH 2/7] pnv: Add missing error check during cpu realize()
Posted by David Gibson 7 years, 8 months ago 
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.

-- 
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
Re: [Qemu-devel] [PATCH 2/7] pnv: Add missing error check during cpu realize()
Posted by David Gibson 7 years, 8 months ago 
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.

-- 
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
Re: [Qemu-devel] [PATCH 2/7] pnv: Add missing error check during cpu realize()
Posted by David Gibson 7 years, 8 months ago 
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.

-- 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.