Re: [Qemu-devel] [for-2.10 PATCH] spapr_drc: fix realize and unrealize

[Greg Kurz]

On Fri, 28 Jul 2017 14:27:45 +1000
David Gibson <address@hidden> 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 <address@hidden>
> > > ---
> > > 
> > > 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,
> > > 
> > >   
> >   
>

pgpacWgZ5ZZfe.pgp
Description: OpenPGP digital signature