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

[Michael Roth]

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