Daniel P. Berrangé <[email protected]> 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.
