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

[David Gibson]

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 <address@hidden> 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

signature.asc
Description: PGP signature