Re: [PATCH 5/5] error: Consistently name Error * objects err, and not errp

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 19/11/2025 1:08 pm, Markus Armbruster wrote:
> diff --git a/hw/xen/xen-pvh-common.c b/hw/xen/xen-pvh-common.c
> index b93ff80c85..3e62ec09d0 100644
> --- a/hw/xen/xen-pvh-common.c
> +++ b/hw/xen/xen-pvh-common.c
> @@ -101,7 +101,7 @@ static void xen_create_virtio_mmio_devices(XenPVHMachineState *s)
>  #ifdef CONFIG_TPM
>  static void xen_enable_tpm(XenPVHMachineState *s)
>  {
> -    Error *errp = NULL;
> +    Error *err = NULL;
>      DeviceState *dev;
>      SysBusDevice *busdev;
>  
> @@ -111,8 +111,15 @@ static void xen_enable_tpm(XenPVHMachineState *s)
>          return;
>      }
>      dev = qdev_new(TYPE_TPM_TIS_SYSBUS);
> -    object_property_set_link(OBJECT(dev), "tpmdev", OBJECT(be), &errp);
> -    object_property_set_str(OBJECT(dev), "tpmdev", be->id, &errp);
> +    /*
> +     * FIXME This use of &err is is wrong.  If both calls fail, the
> +     * second will trip error_setv()'s assertion.  If just one call
> +     * fails, we leak an Error object.  Setting the same property
> +     * twice (first to a QOM path, then to an ID string) is almost
> +     * certainly wrong, too.
> +     */
> +    object_property_set_link(OBJECT(dev), "tpmdev", OBJECT(be), &err);
> +    object_property_set_str(OBJECT(dev), "tpmdev", be->id, &err);

To your question, I don't know the answer, but I think it's far more
likely that the original author didn't grok the proper use of &errp,
than for this behaviour to be intentional.

Surely we just want a failure path and abort the construction if this
goes wrong?

~Andrew