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

[Daniel P. Berrangé <berrange@xxxxxxxxxx>]

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.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|