This touches code in xen_enable_tpm() that is obviously wrong. Since
I don't know how to fix it properly, I'm adding a FIXME there.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
block/crypto.c | 8 ++++----
hw/acpi/ghes.c | 8 ++++----
hw/ppc/spapr.c | 16 ++++++++--------
hw/xen/xen-pvh-common.c | 13 ++++++++++---
nbd/common.c | 6 +++---
5 files changed, 29 insertions(+), 22 deletions(-)
diff --git a/block/crypto.c b/block/crypto.c
index b97d027444..36abb7af46 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -938,14 +938,14 @@ static void GRAPH_RDLOCK
block_crypto_amend_cleanup(BlockDriverState *bs)
{
BlockCrypto *crypto = bs->opaque;
- Error *errp = NULL;
+ Error *err = NULL;
/* release exclusive read/write permissions to the underlying file */
crypto->updating_keys = false;
- bdrv_child_refresh_perms(bs, bs->file, &errp);
+ bdrv_child_refresh_perms(bs, bs->file, &err);
- if (errp) {
- error_report_err(errp);
+ if (err) {
+ error_report_err(err);
}
}
diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
index 06555905ce..841a36e370 100644
--- a/hw/acpi/ghes.c
+++ b/hw/acpi/ghes.c
@@ -563,7 +563,7 @@ int acpi_ghes_memory_errors(AcpiGhesState *ags, uint16_t source_id,
const uint8_t guid[] =
UUID_LE(0xA5BC1114, 0x6F64, 0x4EDE, 0xB8, 0x63, 0x3E, 0x83, \
0xED, 0x7C, 0x83, 0xB1);
- Error *errp = NULL;
+ Error *err = NULL;
int data_length;
GArray *block;
@@ -583,12 +583,12 @@ int acpi_ghes_memory_errors(AcpiGhesState *ags, uint16_t source_id,
acpi_ghes_build_append_mem_cper(block, physical_address);
/* Report the error */
- ghes_record_cper_errors(ags, block->data, block->len, source_id, &errp);
+ ghes_record_cper_errors(ags, block->data, block->len, source_id, &err);
g_array_free(block, true);
- if (errp) {
- error_report_err(errp);
+ if (err) {
+ error_report_err(err);
return -1;
}
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 99b843ba2f..db5e98e458 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2847,7 +2847,7 @@ static void spapr_machine_init(MachineState *machine)
int i;
MemoryRegion *sysmem = get_system_memory();
long load_limit, fw_size;
- Error *errp = NULL;
+ Error *err = NULL;
NICInfo *nd;
if (!filename) {
@@ -2871,7 +2871,7 @@ static void spapr_machine_init(MachineState *machine)
/* Determine capabilities to run with */
spapr_caps_init(spapr);
- kvmppc_check_papr_resize_hpt(&errp);
+ kvmppc_check_papr_resize_hpt(&err);
if (spapr->resize_hpt == SPAPR_RESIZE_HPT_DEFAULT) {
/*
* If the user explicitly requested a mode we should either
@@ -2879,10 +2879,10 @@ static void spapr_machine_init(MachineState *machine)
* it's not set explicitly, we reset our mode to something
* that works
*/
- if (errp) {
+ if (err) {
spapr->resize_hpt = SPAPR_RESIZE_HPT_DISABLED;
- error_free(errp);
- errp = NULL;
+ error_free(err);
+ err = NULL;
} else {
spapr->resize_hpt = smc->resize_hpt_default;
}
@@ -2890,14 +2890,14 @@ static void spapr_machine_init(MachineState *machine)
assert(spapr->resize_hpt != SPAPR_RESIZE_HPT_DEFAULT);
- if ((spapr->resize_hpt != SPAPR_RESIZE_HPT_DISABLED) && errp) {
+ if ((spapr->resize_hpt != SPAPR_RESIZE_HPT_DISABLED) && err) {
/*
* User requested HPT resize, but this host can't supply it. Bail out
*/
- error_report_err(errp);
+ error_report_err(err);
exit(1);
}
- error_free(errp);
+ error_free(err);
spapr->rma_size = spapr_rma_size(spapr, &error_fatal);
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);
busdev = SYS_BUS_DEVICE(dev);
sysbus_realize_and_unref(busdev, &error_fatal);
sysbus_mmio_map(busdev, 0, s->cfg.tpm.base);
diff --git a/nbd/common.c b/nbd/common.c
index 2a133a66c3..f43cbaa15b 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -282,10 +282,10 @@ void nbd_set_socket_send_buffer(QIOChannelSocket *sioc)
#ifdef UNIX_STREAM_SOCKET_SEND_BUFFER_SIZE
if (sioc->localAddr.ss_family == AF_UNIX) {
size_t size = UNIX_STREAM_SOCKET_SEND_BUFFER_SIZE;
- Error *errp = NULL;
+ Error *err = NULL;
- if (qio_channel_socket_set_send_buffer(sioc, size, &errp) < 0) {
- warn_report_err(errp);
+ if (qio_channel_socket_set_send_buffer(sioc, size, &err) < 0) {
+ warn_report_err(err);
}
}
#endif /* UNIX_STREAM_SOCKET_SEND_BUFFER_SIZE */
--
2.49.0On 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
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 :|
Daniel P. Berrangé <berrange@redhat.com> 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.
© 2016 - 2025 Red Hat, Inc.