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

[Markus Armbruster <armbru@xxxxxxxxxx>]

Daniel P. Berrangé <berrange@xxxxxxxxxx> writes:

> On Wed, Nov 19, 2025 at 01:22:06PM +0000, Andrew Cooper wrote:
>> 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?
>
> In the caller of xen_enable_tpm, we just have error_report+exit calls,
> so there's no error propagation ability in the call chain.
>
> The caller will also skip  xen_enable_tpm unless a TPM was explicitly
> requested in the config.
>
> Given that, I'm inclined to say that the object_property_set_* calls
> in xen_enable_tpm should be using &error_abort, as a failure to setup
> the explicitly requested TPM should be fatal.

I *suspect* that the first call always fails, and the second one always
works.  If that's the case, the fix is to delete the first call, and
pass &error_abort to the second.