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

[Greg Kurz]

On Wed, 13 Jun 2018 16:57:02 +1000
David Gibson <address@hidden> 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 <address@hidden>
> ---
>  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.