* [PATCH v3 1/9] hw: eliminate qdev_try_new, isa_try_new & usb_try_new
2024-11-15 17:25 [PATCH v3 0/9] Require error handling for dynamically created objects Daniel P. Berrangé
@ 2024-11-15 17:25 ` Daniel P. Berrangé
2024-11-15 17:54 ` Peter Xu
2024-12-03 15:30 ` Markus Armbruster
2024-11-15 17:25 ` [PATCH v3 2/9] qom: refactor checking abstract property when creating instances Daniel P. Berrangé
` (8 subsequent siblings)
9 siblings, 2 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2024-11-15 17:25 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Peter Xu, Markus Armbruster,
Daniel P. Berrangé
These functions all return NULL rather than asserting, if the requested
type is not registered and also cannot be dynamically loaded.
In several cases their usage is pointless, since the caller then just
reports an error & exits anyway. Easier to just let qdev_new fail
with &error_fatal.
In other cases, the desired semantics are clearer to understand if the
caller directly checks module_object_class_by_name(), before calling
the regular qdev_new (or specialized equiv) function.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
hw/core/qdev.c | 9 ---------
hw/i386/pc.c | 22 ++++++++++------------
hw/isa/isa-bus.c | 5 -----
hw/s390x/s390-pci-bus.c | 6 +-----
hw/usb/bus.c | 6 ++----
include/hw/isa/isa.h | 1 -
include/hw/net/ne2000-isa.h | 16 ++++++----------
include/hw/qdev-core.h | 12 ------------
include/hw/usb.h | 5 -----
9 files changed, 19 insertions(+), 63 deletions(-)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 5f13111b77..960a704a96 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -149,15 +149,6 @@ DeviceState *qdev_new(const char *name)
return DEVICE(object_new(name));
}
-DeviceState *qdev_try_new(const char *name)
-{
- ObjectClass *oc = module_object_class_by_name(name);
- if (!oc) {
- return NULL;
- }
- return DEVICE(object_new_with_class(oc));
-}
-
static QTAILQ_HEAD(, DeviceListener) device_listeners
= QTAILQ_HEAD_INITIALIZER(device_listeners);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f9147fecbd..d668970bee 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -596,9 +596,11 @@ static gboolean pc_init_ne2k_isa(ISABus *bus, NICInfo *nd, Error **errp)
"maximum number of ISA NE2000 devices exceeded");
return false;
}
- isa_ne2000_init(bus, ne2000_io[nb_ne2k],
- ne2000_irq[nb_ne2k], nd);
- nb_ne2k++;
+ if (module_object_class_by_name(TYPE_ISA_NE2000)) {
+ isa_ne2000_init(bus, ne2000_io[nb_ne2k],
+ ne2000_irq[nb_ne2k], nd);
+ nb_ne2k++;
+ }
return true;
}
@@ -1087,7 +1089,7 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
int i;
DriveInfo *fd[MAX_FD];
qemu_irq *a20_line;
- ISADevice *i8042, *port92, *vmmouse;
+ ISADevice *i8042, *port92, *vmmouse = NULL;
serial_hds_isa_init(isa_bus, 0, MAX_ISA_SERIAL_PORTS);
parallel_hds_isa_init(isa_bus, MAX_PARALLEL_PORTS);
@@ -1117,9 +1119,9 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
i8042 = isa_create_simple(isa_bus, TYPE_I8042);
if (!no_vmport) {
isa_create_simple(isa_bus, TYPE_VMPORT);
- vmmouse = isa_try_new("vmmouse");
- } else {
- vmmouse = NULL;
+ if (module_object_class_by_name("vmmouse")) {
+ vmmouse = isa_new("vmmouse");
+ }
}
if (vmmouse) {
object_property_set_link(OBJECT(vmmouse), TYPE_I8042, OBJECT(i8042),
@@ -1163,11 +1165,7 @@ void pc_basic_device_init(struct PCMachineState *pcms,
if (pcms->hpet_enabled) {
qemu_irq rtc_irq;
- hpet = qdev_try_new(TYPE_HPET);
- if (!hpet) {
- error_report("couldn't create HPET device");
- exit(1);
- }
+ hpet = qdev_new(TYPE_HPET);
/*
* For pc-piix-*, hpet's intcap is always IRQ2. For pc-q35-*,
* use IRQ16~23, IRQ8 and IRQ2. If the user has already set
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index f1e0f14007..8aaf44a3ef 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -158,11 +158,6 @@ ISADevice *isa_new(const char *name)
return ISA_DEVICE(qdev_new(name));
}
-ISADevice *isa_try_new(const char *name)
-{
- return ISA_DEVICE(qdev_try_new(name));
-}
-
ISADevice *isa_create_simple(ISABus *bus, const char *name)
{
ISADevice *dev;
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 40b2567aa7..558f17d3ba 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -922,11 +922,7 @@ static S390PCIBusDevice *s390_pci_device_new(S390pciState *s,
Error *local_err = NULL;
DeviceState *dev;
- dev = qdev_try_new(TYPE_S390_PCI_DEVICE);
- if (!dev) {
- error_setg(errp, "zPCI device could not be created");
- return NULL;
- }
+ dev = qdev_new(TYPE_S390_PCI_DEVICE);
if (!object_property_set_str(OBJECT(dev), "target", target, &local_err)) {
object_unparent(OBJECT(dev));
diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index bfab2807d7..002f7737e4 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -394,7 +394,6 @@ void usb_claim_port(USBDevice *dev, Error **errp)
{
USBBus *bus = usb_bus_from_device(dev);
USBPort *port;
- USBDevice *hub;
assert(dev->port == NULL);
@@ -412,9 +411,8 @@ void usb_claim_port(USBDevice *dev, Error **errp)
} else {
if (bus->nfree == 1 && strcmp(object_get_typename(OBJECT(dev)), "usb-hub") != 0) {
/* Create a new hub and chain it on */
- hub = usb_try_new("usb-hub");
- if (hub) {
- usb_realize_and_unref(hub, bus, NULL);
+ if (module_object_class_by_name("usb-hub")) {
+ usb_realize_and_unref(usb_new("usb-hub"), bus, NULL);
}
}
if (bus->nfree == 0) {
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index 40d6224a4e..8475120849 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -81,7 +81,6 @@ IsaDma *isa_bus_get_dma(ISABus *bus, int nchan);
*/
qemu_irq isa_bus_get_irq(ISABus *bus, unsigned irqnum);
ISADevice *isa_new(const char *name);
-ISADevice *isa_try_new(const char *name);
bool isa_realize_and_unref(ISADevice *dev, ISABus *bus, Error **errp);
ISADevice *isa_create_simple(ISABus *bus, const char *name);
diff --git a/include/hw/net/ne2000-isa.h b/include/hw/net/ne2000-isa.h
index 73bae10ad1..aeb3e2bfd5 100644
--- a/include/hw/net/ne2000-isa.h
+++ b/include/hw/net/ne2000-isa.h
@@ -20,17 +20,13 @@
static inline ISADevice *isa_ne2000_init(ISABus *bus, int base, int irq,
NICInfo *nd)
{
- ISADevice *d;
+ ISADevice *d = isa_new(TYPE_ISA_NE2000);
+ DeviceState *dev = DEVICE(d);
- d = isa_try_new(TYPE_ISA_NE2000);
- if (d) {
- DeviceState *dev = DEVICE(d);
-
- qdev_prop_set_uint32(dev, "iobase", base);
- qdev_prop_set_uint32(dev, "irq", irq);
- qdev_set_nic_properties(dev, nd);
- isa_realize_and_unref(d, bus, &error_fatal);
- }
+ qdev_prop_set_uint32(dev, "iobase", base);
+ qdev_prop_set_uint32(dev, "irq", irq);
+ qdev_set_nic_properties(dev, nd);
+ isa_realize_and_unref(d, bus, &error_fatal);
return d;
}
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 5be9844412..020417d027 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -443,18 +443,6 @@ compat_props_add(GPtrArray *arr,
*/
DeviceState *qdev_new(const char *name);
-/**
- * qdev_try_new: Try to create a device on the heap
- * @name: device type to create
- *
- * This is like qdev_new(), except it returns %NULL when type @name
- * does not exist, rather than asserting.
- *
- * Return: a derived DeviceState object with a reference count of 1 or
- * NULL if type @name does not exist.
- */
-DeviceState *qdev_try_new(const char *name);
-
/**
* qdev_is_realized() - check if device is realized
* @dev: The device to check.
diff --git a/include/hw/usb.h b/include/hw/usb.h
index d46d96779a..bb778cb844 100644
--- a/include/hw/usb.h
+++ b/include/hw/usb.h
@@ -584,11 +584,6 @@ static inline USBDevice *usb_new(const char *name)
return USB_DEVICE(qdev_new(name));
}
-static inline USBDevice *usb_try_new(const char *name)
-{
- return USB_DEVICE(qdev_try_new(name));
-}
-
static inline bool usb_realize_and_unref(USBDevice *dev, USBBus *bus, Error **errp)
{
return qdev_realize_and_unref(&dev->qdev, &bus->qbus, errp);
--
2.46.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v3 1/9] hw: eliminate qdev_try_new, isa_try_new & usb_try_new
2024-11-15 17:25 ` [PATCH v3 1/9] hw: eliminate qdev_try_new, isa_try_new & usb_try_new Daniel P. Berrangé
@ 2024-11-15 17:54 ` Peter Xu
2024-11-15 18:34 ` Daniel P. Berrangé
2024-12-03 15:30 ` Markus Armbruster
1 sibling, 1 reply; 22+ messages in thread
From: Peter Xu @ 2024-11-15 17:54 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel, Paolo Bonzini, Markus Armbruster
On Fri, Nov 15, 2024 at 05:25:13PM +0000, Daniel P. Berrangé wrote:
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 40b2567aa7..558f17d3ba 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -922,11 +922,7 @@ static S390PCIBusDevice *s390_pci_device_new(S390pciState *s,
> Error *local_err = NULL;
> DeviceState *dev;
>
> - dev = qdev_try_new(TYPE_S390_PCI_DEVICE);
> - if (!dev) {
> - error_setg(errp, "zPCI device could not be created");
> - return NULL;
> - }
> + dev = qdev_new(TYPE_S390_PCI_DEVICE);
This one used to allow failures, but now it asserts. Especially, see:
b6e67ecc7b ("s390x/pci: properly fail if the zPCI device cannot be created")
Would it be safer to use module_object_class_by_name() too here?
All the rest changes look sane.
>
> if (!object_property_set_str(OBJECT(dev), "target", target, &local_err)) {
> object_unparent(OBJECT(dev));
--
Peter Xu
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v3 1/9] hw: eliminate qdev_try_new, isa_try_new & usb_try_new
2024-11-15 17:54 ` Peter Xu
@ 2024-11-15 18:34 ` Daniel P. Berrangé
0 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2024-11-15 18:34 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Paolo Bonzini, Markus Armbruster
On Fri, Nov 15, 2024 at 12:54:10PM -0500, Peter Xu wrote:
> On Fri, Nov 15, 2024 at 05:25:13PM +0000, Daniel P. Berrangé wrote:
> > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> > index 40b2567aa7..558f17d3ba 100644
> > --- a/hw/s390x/s390-pci-bus.c
> > +++ b/hw/s390x/s390-pci-bus.c
> > @@ -922,11 +922,7 @@ static S390PCIBusDevice *s390_pci_device_new(S390pciState *s,
> > Error *local_err = NULL;
> > DeviceState *dev;
> >
> > - dev = qdev_try_new(TYPE_S390_PCI_DEVICE);
> > - if (!dev) {
> > - error_setg(errp, "zPCI device could not be created");
> > - return NULL;
> > - }
> > + dev = qdev_new(TYPE_S390_PCI_DEVICE);
>
> This one used to allow failures, but now it asserts. Especially, see:
>
> b6e67ecc7b ("s390x/pci: properly fail if the zPCI device cannot be created")
>
> Would it be safer to use module_object_class_by_name() too here?
Yes, my bad. I was mixed up with the HPET case which did exit(), this
one must propagate the error.
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 :|
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 1/9] hw: eliminate qdev_try_new, isa_try_new & usb_try_new
2024-11-15 17:25 ` [PATCH v3 1/9] hw: eliminate qdev_try_new, isa_try_new & usb_try_new Daniel P. Berrangé
2024-11-15 17:54 ` Peter Xu
@ 2024-12-03 15:30 ` Markus Armbruster
2024-12-05 16:21 ` Daniel P. Berrangé
1 sibling, 1 reply; 22+ messages in thread
From: Markus Armbruster @ 2024-12-03 15:30 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel, Paolo Bonzini, Peter Xu
Daniel P. Berrangé <berrange@redhat.com> writes:
> These functions all return NULL rather than asserting, if the requested
> type is not registered and also cannot be dynamically loaded.
>
> In several cases their usage is pointless, since the caller then just
> reports an error & exits anyway. Easier to just let qdev_new fail
> with &error_fatal.
Uh, this sounds as if you'd turn assertion failures by fatal errors,
which could be fine, but more than just "eliminate qdev_try_new...".
Turns out you aren't. qdev_new(), isa_new() and usb_new() are all thin
wrappers around object_new(), which does not assert, but treats errors
as fatal:
Object *object_new(const char *typename)
{
TypeImpl *ti = type_get_or_load_by_name(typename, &error_fatal);
return object_new_with_type(ti);
}
type_get_or_load_by_name() succeeds if
* type @typename is compiled into this binary, or
* exactly one module providing it is known to this binary, and loading
it succeeds.
Put a pin into this for later.
Suggest something like
The difference between qdev_try_new() and qdev_try() is that the
former returns failure, while the latter treats it as fatal and
terminates the process. Same for isa_try_new() and usb_try_new().
A comment in hw/i2c/i2c.h mentions i2c_slave_try_new(), but it doesn't
exist, and never has. Suggest to eliminate that, too.
> In other cases, the desired semantics are clearer to understand if the
> caller directly checks module_object_class_by_name(), before calling
> the regular qdev_new (or specialized equiv) function.
This tacitly assumes qdev_try_new() & friends fail exactly when
module_object_class_by_name() fails. True, but not obvious at this
point.
It's true, because it also fails exactly when type_get_or_load_by_name()
returns null:
ObjectClass *object_class_by_name(const char *typename)
{
TypeImpl *type = type_get_by_name_noload(typename);
if (!type) {
return NULL;
}
type_initialize(type);
return type->class;
}
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> hw/core/qdev.c | 9 ---------
> hw/i386/pc.c | 22 ++++++++++------------
> hw/isa/isa-bus.c | 5 -----
> hw/s390x/s390-pci-bus.c | 6 +-----
> hw/usb/bus.c | 6 ++----
> include/hw/isa/isa.h | 1 -
> include/hw/net/ne2000-isa.h | 16 ++++++----------
> include/hw/qdev-core.h | 12 ------------
> include/hw/usb.h | 5 -----
> 9 files changed, 19 insertions(+), 63 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 5f13111b77..960a704a96 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -149,15 +149,6 @@ DeviceState *qdev_new(const char *name)
> return DEVICE(object_new(name));
> }
>
> -DeviceState *qdev_try_new(const char *name)
> -{
> - ObjectClass *oc = module_object_class_by_name(name);
> - if (!oc) {
> - return NULL;
> - }
> - return DEVICE(object_new_with_class(oc));
> -}
> -
> static QTAILQ_HEAD(, DeviceListener) device_listeners
> = QTAILQ_HEAD_INITIALIZER(device_listeners);
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f9147fecbd..d668970bee 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -596,9 +596,11 @@ static gboolean pc_init_ne2k_isa(ISABus *bus, NICInfo *nd, Error **errp)
> "maximum number of ISA NE2000 devices exceeded");
> return false;
> }
> - isa_ne2000_init(bus, ne2000_io[nb_ne2k],
> - ne2000_irq[nb_ne2k], nd);
> - nb_ne2k++;
> + if (module_object_class_by_name(TYPE_ISA_NE2000)) {
> + isa_ne2000_init(bus, ne2000_io[nb_ne2k],
> + ne2000_irq[nb_ne2k], nd);
> + nb_ne2k++;
> + }
This gave me pause until I saw the change to isa_ne2000_init() below.
There, you replace isa_try_new() by isa_new(). Before the patch,
isa_ne2000_init() can fail, afterwards it treats errors as fatal. And
that's why you need to guard against failure here.
In other words, you lifted the guard out of isa_ne2000_init() into its
sole caller. Fine, just less than obvious in review.
> return true;
> }
>
> @@ -1087,7 +1089,7 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
> int i;
> DriveInfo *fd[MAX_FD];
> qemu_irq *a20_line;
> - ISADevice *i8042, *port92, *vmmouse;
> + ISADevice *i8042, *port92, *vmmouse = NULL;
>
> serial_hds_isa_init(isa_bus, 0, MAX_ISA_SERIAL_PORTS);
> parallel_hds_isa_init(isa_bus, MAX_PARALLEL_PORTS);
> @@ -1117,9 +1119,9 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
> i8042 = isa_create_simple(isa_bus, TYPE_I8042);
> if (!no_vmport) {
> isa_create_simple(isa_bus, TYPE_VMPORT);
> - vmmouse = isa_try_new("vmmouse");
> - } else {
> - vmmouse = NULL;
> + if (module_object_class_by_name("vmmouse")) {
> + vmmouse = isa_new("vmmouse");
> + }
> }
> if (vmmouse) {
> object_property_set_link(OBJECT(vmmouse), TYPE_I8042, OBJECT(i8042),
This is now like
vmmouse = NULL;
if (...) {
if (module_object_class_by_name("vmmouse")) {
vmmouse = isa_new("vmmouse");
}
}
if (vmmouse) {
object_property_set_link(OBJECT(vmmouse), TYPE_I8042, OBJECT(i8042),
&error_abort);
isa_realize_and_unref(vmmouse, isa_bus, &error_fatal);
}
We could straighten control flow like this:
if (...) {
if (module_object_class_by_name("vmmouse")) {
vmmouse = isa_new("vmmouse");
object_property_set_link(OBJECT(vmmouse), TYPE_I8042,
OBJECT(i8042), &error_abort);
isa_realize_and_unref(vmmouse, isa_bus, &error_fatal);
}
}
But there is a more fundamental issue.
pc_superio_init() creates onboard devices.
With CONFIG_MODULES off, it creates a "vmmouse" device exactly when the
type is compiled into this binary. This makes the guest machine type
depend on build configuration. I consider this questionable; I'd prefer
such things to be explicit in the C code. But let's ignore this.
With modules, it creates the device when the type is compiled into this
binary, or is known to it and loadable. Now the machine type
additionally depends on deployment (exactly one module providing it was
deployed, and it's from the same build, and ...), and on the host's
state (module loading succeeds).
Onboard devices are an integral, guest-visible part of the machine type.
Such parts can be optional and user-configurable.
Silently not creating a device due to intricacies of deployment we can
perhaps handwave away as user error, or more humorously, as a really
opaque way for the user to configure the guest.
Silently not creating it just because the machine is in a funny state,
say temporarily lacks the resources to load a DSO, is plainly wrong.
Not this patch's fault. Doesn't make it less wrong :)
> @@ -1163,11 +1165,7 @@ void pc_basic_device_init(struct PCMachineState *pcms,
> if (pcms->hpet_enabled) {
> qemu_irq rtc_irq;
>
> - hpet = qdev_try_new(TYPE_HPET);
> - if (!hpet) {
> - error_report("couldn't create HPET device");
> - exit(1);
> - }
> + hpet = qdev_new(TYPE_HPET);
This replaces the error message "couldn't create HPET device" by one
provided by QOM. These are:
* When the type is not known to this binary: "unknown type 'hpet'".
* When the type is known, but not compiled in, and the module can't be
loaded for whatever reason: "could not load a module for type 'hpet':
MORE", where MORE is the error message provided by module_load_qom().
Worth at least hinting at this in the commit message?
> /*
> * For pc-piix-*, hpet's intcap is always IRQ2. For pc-q35-*,
> * use IRQ16~23, IRQ8 and IRQ2. If the user has already set
> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
> index f1e0f14007..8aaf44a3ef 100644
> --- a/hw/isa/isa-bus.c
> +++ b/hw/isa/isa-bus.c
> @@ -158,11 +158,6 @@ ISADevice *isa_new(const char *name)
> return ISA_DEVICE(qdev_new(name));
> }
>
> -ISADevice *isa_try_new(const char *name)
> -{
> - return ISA_DEVICE(qdev_try_new(name));
> -}
> -
> ISADevice *isa_create_simple(ISABus *bus, const char *name)
> {
> ISADevice *dev;
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 40b2567aa7..558f17d3ba 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -922,11 +922,7 @@ static S390PCIBusDevice *s390_pci_device_new(S390pciState *s,
> Error *local_err = NULL;
> DeviceState *dev;
>
> - dev = qdev_try_new(TYPE_S390_PCI_DEVICE);
> - if (!dev) {
> - error_setg(errp, "zPCI device could not be created");
> - return NULL;
> - }
> + dev = qdev_new(TYPE_S390_PCI_DEVICE);
Likewise.
>
> if (!object_property_set_str(OBJECT(dev), "target", target, &local_err)) {
> object_unparent(OBJECT(dev));
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index bfab2807d7..002f7737e4 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -394,7 +394,6 @@ void usb_claim_port(USBDevice *dev, Error **errp)
> {
> USBBus *bus = usb_bus_from_device(dev);
> USBPort *port;
> - USBDevice *hub;
>
> assert(dev->port == NULL);
>
> @@ -412,9 +411,8 @@ void usb_claim_port(USBDevice *dev, Error **errp)
> } else {
> if (bus->nfree == 1 && strcmp(object_get_typename(OBJECT(dev)), "usb-hub") != 0) {
> /* Create a new hub and chain it on */
> - hub = usb_try_new("usb-hub");
> - if (hub) {
> - usb_realize_and_unref(hub, bus, NULL);
> + if (module_object_class_by_name("usb-hub")) {
> + usb_realize_and_unref(usb_new("usb-hub"), bus, NULL);
Another instance of silent non-creation of an onboard device when
deployment is bad host is in a funny state.
> }
> }
> if (bus->nfree == 0) {
> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
> index 40d6224a4e..8475120849 100644
> --- a/include/hw/isa/isa.h
> +++ b/include/hw/isa/isa.h
> @@ -81,7 +81,6 @@ IsaDma *isa_bus_get_dma(ISABus *bus, int nchan);
> */
> qemu_irq isa_bus_get_irq(ISABus *bus, unsigned irqnum);
> ISADevice *isa_new(const char *name);
> -ISADevice *isa_try_new(const char *name);
> bool isa_realize_and_unref(ISADevice *dev, ISABus *bus, Error **errp);
> ISADevice *isa_create_simple(ISABus *bus, const char *name);
>
> diff --git a/include/hw/net/ne2000-isa.h b/include/hw/net/ne2000-isa.h
> index 73bae10ad1..aeb3e2bfd5 100644
> --- a/include/hw/net/ne2000-isa.h
> +++ b/include/hw/net/ne2000-isa.h
> @@ -20,17 +20,13 @@
> static inline ISADevice *isa_ne2000_init(ISABus *bus, int base, int irq,
> NICInfo *nd)
> {
> - ISADevice *d;
> + ISADevice *d = isa_new(TYPE_ISA_NE2000);
> + DeviceState *dev = DEVICE(d);
>
> - d = isa_try_new(TYPE_ISA_NE2000);
> - if (d) {
> - DeviceState *dev = DEVICE(d);
> -
> - qdev_prop_set_uint32(dev, "iobase", base);
> - qdev_prop_set_uint32(dev, "irq", irq);
> - qdev_set_nic_properties(dev, nd);
> - isa_realize_and_unref(d, bus, &error_fatal);
> - }
> + qdev_prop_set_uint32(dev, "iobase", base);
> + qdev_prop_set_uint32(dev, "irq", irq);
> + qdev_set_nic_properties(dev, nd);
> + isa_realize_and_unref(d, bus, &error_fatal);
> return d;
> }
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 5be9844412..020417d027 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -443,18 +443,6 @@ compat_props_add(GPtrArray *arr,
> */
> DeviceState *qdev_new(const char *name);
>
> -/**
> - * qdev_try_new: Try to create a device on the heap
> - * @name: device type to create
> - *
> - * This is like qdev_new(), except it returns %NULL when type @name
> - * does not exist, rather than asserting.
> - *
> - * Return: a derived DeviceState object with a reference count of 1 or
> - * NULL if type @name does not exist.
> - */
> -DeviceState *qdev_try_new(const char *name);
> -
> /**
> * qdev_is_realized() - check if device is realized
> * @dev: The device to check.
> diff --git a/include/hw/usb.h b/include/hw/usb.h
> index d46d96779a..bb778cb844 100644
> --- a/include/hw/usb.h
> +++ b/include/hw/usb.h
> @@ -584,11 +584,6 @@ static inline USBDevice *usb_new(const char *name)
> return USB_DEVICE(qdev_new(name));
> }
>
> -static inline USBDevice *usb_try_new(const char *name)
> -{
> - return USB_DEVICE(qdev_try_new(name));
> -}
> -
> static inline bool usb_realize_and_unref(USBDevice *dev, USBBus *bus, Error **errp)
> {
> return qdev_realize_and_unref(&dev->qdev, &bus->qbus, errp);
Maybe I'm having another scatter-brained day, but I found the patch
somewhat confusing in review. Happy to suggest a possible split if
you're interested.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v3 1/9] hw: eliminate qdev_try_new, isa_try_new & usb_try_new
2024-12-03 15:30 ` Markus Armbruster
@ 2024-12-05 16:21 ` Daniel P. Berrangé
0 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2024-12-05 16:21 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Paolo Bonzini, Peter Xu
On Tue, Dec 03, 2024 at 04:30:06PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > These functions all return NULL rather than asserting, if the requested
> > type is not registered and also cannot be dynamically loaded.
> >
> > In several cases their usage is pointless, since the caller then just
> > reports an error & exits anyway. Easier to just let qdev_new fail
> > with &error_fatal.
>
> Uh, this sounds as if you'd turn assertion failures by fatal errors,
> which could be fine, but more than just "eliminate qdev_try_new...".
>
> Turns out you aren't. qdev_new(), isa_new() and usb_new() are all thin
> wrappers around object_new(), which does not assert, but treats errors
> as fatal:
>
> Object *object_new(const char *typename)
> {
> TypeImpl *ti = type_get_or_load_by_name(typename, &error_fatal);
>
> return object_new_with_type(ti);
> }
>
> type_get_or_load_by_name() succeeds if
>
> * type @typename is compiled into this binary, or
>
> * exactly one module providing it is known to this binary, and loading
> it succeeds.
>
> Put a pin into this for later.
>
> Suggest something like
>
> The difference between qdev_try_new() and qdev_try() is that the
> former returns failure, while the latter treats it as fatal and
> terminates the process. Same for isa_try_new() and usb_try_new().
>
> A comment in hw/i2c/i2c.h mentions i2c_slave_try_new(), but it doesn't
> exist, and never has. Suggest to eliminate that, too.
>
> > In other cases, the desired semantics are clearer to understand if the
> > caller directly checks module_object_class_by_name(), before calling
> > the regular qdev_new (or specialized equiv) function.
>
> This tacitly assumes qdev_try_new() & friends fail exactly when
> module_object_class_by_name() fails. True, but not obvious at this
> point.
>
> It's true, because it also fails exactly when type_get_or_load_by_name()
> returns null:
>
> ObjectClass *object_class_by_name(const char *typename)
> {
> TypeImpl *type = type_get_by_name_noload(typename);
>
> if (!type) {
> return NULL;
> }
>
> type_initialize(type);
>
> return type->class;
> }
>
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index f9147fecbd..d668970bee 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -596,9 +596,11 @@ static gboolean pc_init_ne2k_isa(ISABus *bus, NICInfo *nd, Error **errp)
> > "maximum number of ISA NE2000 devices exceeded");
> > return false;
> > }
> > - isa_ne2000_init(bus, ne2000_io[nb_ne2k],
> > - ne2000_irq[nb_ne2k], nd);
> > - nb_ne2k++;
> > + if (module_object_class_by_name(TYPE_ISA_NE2000)) {
> > + isa_ne2000_init(bus, ne2000_io[nb_ne2k],
> > + ne2000_irq[nb_ne2k], nd);
> > + nb_ne2k++;
> > + }
>
> This gave me pause until I saw the change to isa_ne2000_init() below.
> There, you replace isa_try_new() by isa_new(). Before the patch,
> isa_ne2000_init() can fail, afterwards it treats errors as fatal. And
> that's why you need to guard against failure here.
>
> In other words, you lifted the guard out of isa_ne2000_init() into its
> sole caller. Fine, just less than obvious in review.
Yeah, actually this is a pre-existing bug I should fix in a
preceeding patch. isa_ne2000_init can fail today, but we
don't check the return value, and unconditionally do
"nb_ne2k++". So nb_ne2k is wrong if isa_ne2000_init ever
fails. Not sure if this has any bad functional effect,
but conceptually it is clearly a bug.
>
> > return true;
> > }
> >
> > @@ -1087,7 +1089,7 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
> > int i;
> > DriveInfo *fd[MAX_FD];
> > qemu_irq *a20_line;
> > - ISADevice *i8042, *port92, *vmmouse;
> > + ISADevice *i8042, *port92, *vmmouse = NULL;
> >
> > serial_hds_isa_init(isa_bus, 0, MAX_ISA_SERIAL_PORTS);
> > parallel_hds_isa_init(isa_bus, MAX_PARALLEL_PORTS);
> > @@ -1117,9 +1119,9 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
> > i8042 = isa_create_simple(isa_bus, TYPE_I8042);
> > if (!no_vmport) {
> > isa_create_simple(isa_bus, TYPE_VMPORT);
> > - vmmouse = isa_try_new("vmmouse");
> > - } else {
> > - vmmouse = NULL;
> > + if (module_object_class_by_name("vmmouse")) {
> > + vmmouse = isa_new("vmmouse");
> > + }
> > }
> > if (vmmouse) {
> > object_property_set_link(OBJECT(vmmouse), TYPE_I8042, OBJECT(i8042),
>
> This is now like
>
> vmmouse = NULL;
> if (...) {
> if (module_object_class_by_name("vmmouse")) {
> vmmouse = isa_new("vmmouse");
> }
> }
> if (vmmouse) {
> object_property_set_link(OBJECT(vmmouse), TYPE_I8042, OBJECT(i8042),
> &error_abort);
> isa_realize_and_unref(vmmouse, isa_bus, &error_fatal);
> }
>
> We could straighten control flow like this:
>
> if (...) {
> if (module_object_class_by_name("vmmouse")) {
> vmmouse = isa_new("vmmouse");
> object_property_set_link(OBJECT(vmmouse), TYPE_I8042,
> OBJECT(i8042), &error_abort);
> isa_realize_and_unref(vmmouse, isa_bus, &error_fatal);
> }
> }
>
> But there is a more fundamental issue.
>
> pc_superio_init() creates onboard devices.
>
> With CONFIG_MODULES off, it creates a "vmmouse" device exactly when the
> type is compiled into this binary. This makes the guest machine type
> depend on build configuration. I consider this questionable; I'd prefer
> such things to be explicit in the C code. But let's ignore this.
Yeah, I had the same horrified realization that we'd made machine ABI
vary based on installed pkgs :-( Not sure how to get us out of that
mess easily.
> Silently not creating it just because the machine is in a funny state,
> say temporarily lacks the resources to load a DSO, is plainly wrong.
>
> Not this patch's fault. Doesn't make it less wrong :)
Agreed, we definitely need to distinguish "module not installed",
from all other types of failure to load a module.
> > @@ -1163,11 +1165,7 @@ void pc_basic_device_init(struct PCMachineState *pcms,
> > if (pcms->hpet_enabled) {
> > qemu_irq rtc_irq;
> >
> > - hpet = qdev_try_new(TYPE_HPET);
> > - if (!hpet) {
> > - error_report("couldn't create HPET device");
> > - exit(1);
> > - }
> > + hpet = qdev_new(TYPE_HPET);
>
> This replaces the error message "couldn't create HPET device" by one
> provided by QOM. These are:
>
> * When the type is not known to this binary: "unknown type 'hpet'".
>
> * When the type is known, but not compiled in, and the module can't be
> loaded for whatever reason: "could not load a module for type 'hpet':
> MORE", where MORE is the error message provided by module_load_qom().
>
> Worth at least hinting at this in the commit message?
Sure.
> > diff --git a/include/hw/usb.h b/include/hw/usb.h
> > index d46d96779a..bb778cb844 100644
> > --- a/include/hw/usb.h
> > +++ b/include/hw/usb.h
> > @@ -584,11 +584,6 @@ static inline USBDevice *usb_new(const char *name)
> > return USB_DEVICE(qdev_new(name));
> > }
> >
> > -static inline USBDevice *usb_try_new(const char *name)
> > -{
> > - return USB_DEVICE(qdev_try_new(name));
> > -}
> > -
> > static inline bool usb_realize_and_unref(USBDevice *dev, USBBus *bus, Error **errp)
> > {
> > return qdev_realize_and_unref(&dev->qdev, &bus->qbus, errp);
>
> Maybe I'm having another scatter-brained day, but I found the patch
> somewhat confusing in review. Happy to suggest a possible split if
> you're interested.
I can have another think about changing it. Mostly I was just working
backwards when creating this, by deleting the methods I wanted to
remove and the patching up the build failures, so there wasn't much
thought put into the split of this one.
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 :|
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 2/9] qom: refactor checking abstract property when creating instances
2024-11-15 17:25 [PATCH v3 0/9] Require error handling for dynamically created objects Daniel P. Berrangé
2024-11-15 17:25 ` [PATCH v3 1/9] hw: eliminate qdev_try_new, isa_try_new & usb_try_new Daniel P. Berrangé
@ 2024-11-15 17:25 ` Daniel P. Berrangé
2024-11-15 17:54 ` Peter Xu
2024-11-15 17:25 ` [PATCH v3 3/9] qom: allow failure of object_new_with_class Daniel P. Berrangé
` (7 subsequent siblings)
9 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2024-11-15 17:25 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Peter Xu, Markus Armbruster,
Daniel P. Berrangé
Push an Error object into object_initialize_with_type, so that
reporting of attempts to create an abstract type is handled at
the lowest level.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
qom/object.c | 30 ++++++++++++++++++++----------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/qom/object.c b/qom/object.c
index 9edc06d391..9632a894ee 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -559,14 +559,20 @@ static void object_class_property_init_all(Object *obj)
}
}
-static void object_initialize_with_type(Object *obj, size_t size, TypeImpl *type)
+static bool object_initialize_with_type(Object *obj, size_t size,
+ TypeImpl *type, Error **errp)
{
type_initialize(type);
g_assert(type->instance_size >= sizeof(Object));
- g_assert(type->abstract == false);
g_assert(size >= type->instance_size);
+ if (type->abstract) {
+ error_setg(errp, "Abstract type '%s' cannot be instantiated",
+ type->name);
+ return false;
+ }
+
memset(obj, 0, type->instance_size);
obj->class = type->class;
object_ref(obj);
@@ -575,13 +581,15 @@ static void object_initialize_with_type(Object *obj, size_t size, TypeImpl *type
NULL, object_property_free);
object_init_with_type(obj, type);
object_post_init_with_type(obj, type);
+
+ return true;
}
void object_initialize(void *data, size_t size, const char *typename)
{
TypeImpl *type = type_get_or_load_by_name(typename, &error_fatal);
- object_initialize_with_type(data, size, type);
+ object_initialize_with_type(data, size, type, &error_abort);
}
bool object_initialize_child_with_props(Object *parentobj,
@@ -753,7 +761,7 @@ typedef union {
} qemu_max_align_t;
#endif
-static Object *object_new_with_type(Type type)
+static Object *object_new_with_type(Type type, Error **errp)
{
Object *obj;
size_t size, align;
@@ -777,7 +785,10 @@ static Object *object_new_with_type(Type type)
obj_free = qemu_vfree;
}
- object_initialize_with_type(obj, size, type);
+ if (!object_initialize_with_type(obj, size, type, errp)) {
+ obj_free(obj);
+ return NULL;
+ }
obj->free = obj_free;
return obj;
@@ -785,14 +796,14 @@ static Object *object_new_with_type(Type type)
Object *object_new_with_class(ObjectClass *klass)
{
- return object_new_with_type(klass->type);
+ return object_new_with_type(klass->type, &error_abort);
}
Object *object_new(const char *typename)
{
TypeImpl *ti = type_get_or_load_by_name(typename, &error_fatal);
- return object_new_with_type(ti);
+ return object_new_with_type(ti, &error_abort);
}
@@ -829,11 +840,10 @@ Object *object_new_with_propv(const char *typename,
return NULL;
}
- if (object_class_is_abstract(klass)) {
- error_setg(errp, "object type '%s' is abstract", typename);
+ obj = object_new_with_type(klass->type, errp);
+ if (!obj) {
return NULL;
}
- obj = object_new_with_type(klass->type);
if (!object_set_propv(obj, errp, vargs)) {
goto error;
--
2.46.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v3 3/9] qom: allow failure of object_new_with_class
2024-11-15 17:25 [PATCH v3 0/9] Require error handling for dynamically created objects Daniel P. Berrangé
2024-11-15 17:25 ` [PATCH v3 1/9] hw: eliminate qdev_try_new, isa_try_new & usb_try_new Daniel P. Berrangé
2024-11-15 17:25 ` [PATCH v3 2/9] qom: refactor checking abstract property when creating instances Daniel P. Berrangé
@ 2024-11-15 17:25 ` Daniel P. Berrangé
2024-11-15 17:25 ` [PATCH v3 4/9] qom: introduce object_new_dynamic() Daniel P. Berrangé
` (6 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2024-11-15 17:25 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Peter Xu, Markus Armbruster,
Daniel P. Berrangé
Since object_new_with_class() accepts a non-const parameter for
the class, callers should be prepared for failures from unexpected
input. Add an Error parameter for this and make callers check.
If the caller does not already have an Error parameter, it is
satisfactory to use &error_abort if the class parameter choice is
not driven by untrusted user input.
This conversion allows removal of any object_class_is_abstract()
checks immediately before object_new_with_class().
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
accel/accel-user.c | 4 +++-
include/qom/object.h | 9 +++++++--
net/net.c | 3 ++-
qom/object.c | 4 ++--
qom/object_interfaces.c | 7 +++----
qom/qom-qmp-cmds.c | 11 ++++++-----
system/vl.c | 6 ++++--
target/i386/cpu-apic.c | 8 +++++++-
target/i386/cpu-sysemu.c | 11 ++++++++---
target/i386/cpu.c | 4 ++--
target/s390x/cpu_models_sysemu.c | 7 +++++--
11 files changed, 49 insertions(+), 25 deletions(-)
diff --git a/accel/accel-user.c b/accel/accel-user.c
index 22b6a1a1a8..df673ec0e4 100644
--- a/accel/accel-user.c
+++ b/accel/accel-user.c
@@ -9,6 +9,7 @@
#include "qemu/osdep.h"
#include "qemu/accel.h"
+#include "qapi/error.h"
AccelState *current_accel(void)
{
@@ -18,7 +19,8 @@ AccelState *current_accel(void)
AccelClass *ac = accel_find("tcg");
g_assert(ac != NULL);
- accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac)));
+ accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac),
+ &error_abort));
}
return accel;
}
diff --git a/include/qom/object.h b/include/qom/object.h
index 43c135984a..11ee472719 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -606,14 +606,19 @@ struct InterfaceClass
/**
* object_new_with_class:
* @klass: The class to instantiate.
+ * @errp: pointer to be filled with error details on failure
*
* This function will initialize a new object using heap allocated memory.
* The returned object has a reference count of 1, and will be freed when
* the last reference is dropped.
*
- * Returns: The newly allocated and instantiated object.
+ * If an instance of @klass is not permitted to be instantiated, an
+ * error will be raised. This can happen if @klass is abstract.
+ *
+ * Returns: The newly allocated and instantiated object, or NULL
+ * on error.
*/
-Object *object_new_with_class(ObjectClass *klass);
+Object *object_new_with_class(ObjectClass *klass, Error **errp);
/**
* object_new:
diff --git a/net/net.c b/net/net.c
index 7ef6885876..fbbfe602a4 100644
--- a/net/net.c
+++ b/net/net.c
@@ -948,7 +948,8 @@ GPtrArray *qemu_get_nic_models(const char *device_type)
* create this property during instance_init, so we have to create
* a temporary instance here to be able to check it.
*/
- Object *obj = object_new_with_class(OBJECT_CLASS(dc));
+ Object *obj = object_new_with_class(OBJECT_CLASS(dc),
+ &error_abort);
if (object_property_find(obj, "netdev")) {
g_ptr_array_add(nic_models, (gpointer)name);
}
diff --git a/qom/object.c b/qom/object.c
index 9632a894ee..ad5b3b9582 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -794,9 +794,9 @@ static Object *object_new_with_type(Type type, Error **errp)
return obj;
}
-Object *object_new_with_class(ObjectClass *klass)
+Object *object_new_with_class(ObjectClass *klass, Error **errp)
{
- return object_new_with_type(klass->type, &error_abort);
+ return object_new_with_type(klass->type, errp);
}
Object *object_new(const char *typename)
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 1a6f29c053..967b906755 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -102,13 +102,12 @@ Object *user_creatable_add_type(const char *type, const char *id,
return NULL;
}
- if (object_class_is_abstract(klass)) {
- error_setg(errp, "object type '%s' is abstract", type);
+ assert(qdict);
+ obj = object_new_with_class(klass, errp);
+ if (!obj) {
return NULL;
}
- assert(qdict);
- obj = object_new_with_class(klass);
object_set_properties_from_qdict(obj, qdict, v, &local_err);
if (local_err) {
goto out;
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index 46e4562300..4a8e269fef 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -134,14 +134,15 @@ ObjectPropertyInfoList *qmp_device_list_properties(const char *typename,
return NULL;
}
- if (!object_class_dynamic_cast(klass, TYPE_DEVICE)
- || object_class_is_abstract(klass)) {
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "typename",
- "a non-abstract device type");
+ if (!object_class_dynamic_cast(klass, TYPE_DEVICE)) {
+ error_setg(errp, "Object '%s' is not a device type", typename);
return NULL;
}
- obj = object_new_with_class(klass);
+ obj = object_new_with_class(klass, errp);
+ if (!obj) {
+ return NULL;
+ }
object_property_iter_init(&iter, obj);
while ((prop = object_property_iter_next(&iter))) {
diff --git a/system/vl.c b/system/vl.c
index d217b3d64d..f4eec7f35c 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2117,7 +2117,8 @@ static void qemu_create_machine(QDict *qdict)
MachineClass *machine_class = select_machine(qdict, &error_fatal);
object_set_machine_compat_props(machine_class->compat_props);
- current_machine = MACHINE(object_new_with_class(OBJECT_CLASS(machine_class)));
+ current_machine = MACHINE(object_new_with_class(OBJECT_CLASS(machine_class),
+ &error_fatal));
object_property_add_child(object_get_root(), "machine",
OBJECT(current_machine));
object_property_add_child(container_get(OBJECT(current_machine),
@@ -2327,7 +2328,8 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
}
goto bad;
}
- accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac)));
+ accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac),
+ &error_fatal));
object_apply_compat_props(OBJECT(accel));
qemu_opt_foreach(opts, accelerator_set_property,
accel,
diff --git a/target/i386/cpu-apic.c b/target/i386/cpu-apic.c
index d397ec94dc..8a518c50c7 100644
--- a/target/i386/cpu-apic.c
+++ b/target/i386/cpu-apic.c
@@ -43,12 +43,18 @@ void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
{
APICCommonState *apic;
APICCommonClass *apic_class = apic_get_class(errp);
+ Object *apicobj;
if (!apic_class) {
return;
}
- cpu->apic_state = DEVICE(object_new_with_class(OBJECT_CLASS(apic_class)));
+ apicobj = object_new_with_class(OBJECT_CLASS(apic_class),
+ errp);
+ if (!apicobj) {
+ return;
+ }
+ cpu->apic_state = DEVICE(apicobj);
object_property_add_child(OBJECT(cpu), "lapic",
OBJECT(cpu->apic_state));
object_unref(OBJECT(cpu->apic_state));
diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
index 227ac021f6..612ff09e57 100644
--- a/target/i386/cpu-sysemu.c
+++ b/target/i386/cpu-sysemu.c
@@ -156,15 +156,20 @@ static X86CPU *x86_cpu_from_model(const char *model, QObject *props,
{
X86CPU *xc = NULL;
X86CPUClass *xcc;
+ Object *xcobj;
Error *err = NULL;
xcc = X86_CPU_CLASS(cpu_class_by_name(TYPE_X86_CPU, model));
if (xcc == NULL) {
- error_setg(&err, "CPU model '%s' not found", model);
- goto out;
+ error_setg(errp, "CPU model '%s' not found", model);
+ return NULL;
}
- xc = X86_CPU(object_new_with_class(OBJECT_CLASS(xcc)));
+ xcobj = object_new_with_class(OBJECT_CLASS(xcc), errp);
+ if (!xcobj) {
+ return NULL;
+ }
+ xc = X86_CPU(xcobj);
if (props) {
object_apply_props(OBJECT(xc), props, props_arg_name, &err);
if (err) {
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 3725dbbc4b..d4315df29f 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5973,7 +5973,7 @@ static GSList *get_sorted_cpu_model_list(void)
static char *x86_cpu_class_get_model_id(X86CPUClass *xc)
{
- Object *obj = object_new_with_class(OBJECT_CLASS(xc));
+ Object *obj = object_new_with_class(OBJECT_CLASS(xc), &error_abort);
char *r = object_property_get_str(obj, "model-id", &error_abort);
object_unref(obj);
return r;
@@ -6071,7 +6071,7 @@ static void x86_cpu_class_check_missing_features(X86CPUClass *xcc,
return;
}
- xc = X86_CPU(object_new_with_class(OBJECT_CLASS(xcc)));
+ xc = X86_CPU(object_new_with_class(OBJECT_CLASS(xcc), &error_abort));
x86_cpu_expand_features(xc, &err);
if (err) {
diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c
index f6df691b66..7fe3093056 100644
--- a/target/s390x/cpu_models_sysemu.c
+++ b/target/s390x/cpu_models_sysemu.c
@@ -69,7 +69,7 @@ static void create_cpu_model_list(ObjectClass *klass, void *opaque)
if (cpu_list_data->model) {
Object *obj;
S390CPU *sc;
- obj = object_new_with_class(klass);
+ obj = object_new_with_class(klass, &error_abort);
sc = S390_CPU(obj);
if (sc->model) {
info->has_unavailable_features = true;
@@ -116,7 +116,10 @@ static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
error_setg(errp, "The CPU definition '%s' requires KVM", info->name);
return;
}
- obj = object_new_with_class(oc);
+ obj = object_new_with_class(oc, errp);
+ if (!obj) {
+ return;
+ }
cpu = S390_CPU(obj);
if (!cpu->model) {
--
2.46.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v3 4/9] qom: introduce object_new_dynamic()
2024-11-15 17:25 [PATCH v3 0/9] Require error handling for dynamically created objects Daniel P. Berrangé
` (2 preceding siblings ...)
2024-11-15 17:25 ` [PATCH v3 3/9] qom: allow failure of object_new_with_class Daniel P. Berrangé
@ 2024-11-15 17:25 ` Daniel P. Berrangé
2024-11-15 17:25 ` [PATCH v3 5/9] convert code to object_new_dynamic() where appropriate Daniel P. Berrangé
` (5 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2024-11-15 17:25 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Peter Xu, Markus Armbruster,
Daniel P. Berrangé
object_new() has a failure scenario where it will assert() if given
an abstract type. Callers which are creating objects based on user
input, or unknown/untrusted type names, must manually check the
result of object_class_is_abstract() before calling object_new()
to propagate an Error, instead of asserting.
Introduce a object_new_dynamic() method which is a counterpart to
object_new() that directly returns an Error, instead of asserting.
This new method is to be used where the typename is specified
dynamically by code separate from the immediate caller.
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
include/qom/object.h | 27 +++++++++++++++++++++++++++
qom/object.c | 9 +++++++++
2 files changed, 36 insertions(+)
diff --git a/include/qom/object.h b/include/qom/object.h
index 11ee472719..4fc01336c4 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -624,14 +624,41 @@ Object *object_new_with_class(ObjectClass *klass, Error **errp);
* object_new:
* @typename: The name of the type of the object to instantiate.
*
+ * This method should be used where @typename is statically specified
+ * from a const string at build time, where the caller does not expect
+ * failure to be possible.
+ *
* This function will initialize a new object using heap allocated memory.
* The returned object has a reference count of 1, and will be freed when
* the last reference is dropped.
*
+ * If an instance of @typename is not permitted to be instantiated, an
+ * assert will be raised. This can happen if @typename is abstract.
+ *
* Returns: The newly allocated and instantiated object.
*/
Object *object_new(const char *typename);
+/**
+ * object_new_dynamic:
+ * @typename: The name of the type of the object to instantiate.
+ * @errp: pointer to be filled with error details on failure
+ *
+ * This method should be used where @typename is dynamically chosen
+ * at runtime, which has the possibility of unexpected choices leading
+ * to failures.
+ *
+ * This function will initialize a new object using heap allocated memory.
+ * The returned object has a reference count of 1, and will be freed when
+ * the last reference is dropped.
+ *
+ * If an instance of @typename is not permitted to be instantiated, an
+ * error will be raised. This can happen if @typename is abstract.
+ *
+ * Returns: The newly allocated and instantiated object.
+ */
+Object *object_new_dynamic(const char *typename, Error **errp);
+
/**
* object_new_with_props:
* @typename: The name of the type of the object to instantiate.
diff --git a/qom/object.c b/qom/object.c
index ad5b3b9582..42ef40a1fd 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -806,6 +806,15 @@ Object *object_new(const char *typename)
return object_new_with_type(ti, &error_abort);
}
+Object *object_new_dynamic(const char *typename, Error **errp)
+{
+ TypeImpl *ti = type_get_or_load_by_name(typename, errp);
+ if (!ti) {
+ return NULL;
+ }
+
+ return object_new_with_type(ti, errp);
+}
Object *object_new_with_props(const char *typename,
Object *parent,
--
2.46.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v3 5/9] convert code to object_new_dynamic() where appropriate
2024-11-15 17:25 [PATCH v3 0/9] Require error handling for dynamically created objects Daniel P. Berrangé
` (3 preceding siblings ...)
2024-11-15 17:25 ` [PATCH v3 4/9] qom: introduce object_new_dynamic() Daniel P. Berrangé
@ 2024-11-15 17:25 ` Daniel P. Berrangé
2024-11-15 17:25 ` [PATCH v3 6/9] qom: enforce use of static, const string with object_new() Daniel P. Berrangé
` (4 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2024-11-15 17:25 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Peter Xu, Markus Armbruster,
Daniel P. Berrangé
In cases where object_new() is not being passed a static, const
string, the caller cannot be sure what type they are instantiating.
There is a risk that instantiation could fail, if it is an abstract
type.
Convert such cases over to use object_new_dynamic() such that they
are forced to expect failure. In some cases failure can be easily
propagated, but in others using &error_abort or &error_fatal will
maintain existnig behaviour.
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
chardev/char.c | 6 +++++-
hw/arm/aspeed.c | 4 ++--
hw/arm/exynos4210.c | 3 ++-
hw/arm/highbank.c | 2 +-
hw/arm/integratorcp.c | 2 +-
hw/arm/mps3r.c | 3 ++-
hw/arm/realview.c | 3 ++-
hw/arm/sbsa-ref.c | 3 ++-
hw/arm/versatilepb.c | 2 +-
hw/arm/vexpress.c | 2 +-
hw/arm/virt.c | 6 ++++--
hw/arm/xilinx_zynq.c | 3 ++-
hw/core/bus.c | 2 +-
hw/core/cpu-common.c | 2 +-
hw/core/qdev.c | 2 +-
hw/i386/x86-common.c | 5 ++++-
hw/i386/xen/xen-pvh.c | 2 +-
hw/intc/xics.c | 5 ++++-
hw/mips/cps.c | 3 ++-
hw/pci-host/pnv_phb.c | 5 ++++-
hw/ppc/e500.c | 2 +-
hw/ppc/pnv.c | 4 ++--
hw/ppc/pnv_core.c | 5 ++++-
hw/ppc/spapr.c | 2 +-
hw/ppc/spapr_cpu_core.c | 5 ++++-
hw/ppc/spapr_drc.c | 2 +-
hw/s390x/s390-virtio-ccw.c | 8 +++++++-
hw/sparc/leon3.c | 2 +-
hw/sparc/sun4m.c | 2 +-
hw/sparc64/sparc64.c | 2 +-
hw/vfio/common.c | 6 +++++-
hw/vfio/container.c | 7 ++++++-
qom/qom-qmp-cmds.c | 5 ++++-
target/arm/arm-qmp-cmds.c | 5 ++++-
target/loongarch/loongarch-qmp-cmds.c | 5 ++++-
target/mips/cpu.c | 2 +-
target/riscv/riscv-qmp-cmds.c | 5 ++++-
target/xtensa/cpu.c | 2 +-
tests/unit/check-qom-interface.c | 3 ++-
tests/unit/test-smp-parse.c | 20 ++++++++++----------
40 files changed, 107 insertions(+), 52 deletions(-)
diff --git a/chardev/char.c b/chardev/char.c
index a1722aa076..5951b8bef5 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -996,7 +996,11 @@ static Chardev *chardev_new(const char *id, const char *typename,
assert(g_str_has_prefix(typename, "chardev-"));
assert(id);
- obj = object_new(typename);
+ obj = object_new_dynamic(typename, errp);
+ if (!obj) {
+ return NULL;
+ }
+
chr = CHARDEV(obj);
chr->handover_yank_instance = handover_yank_instance;
chr->label = g_strdup(id);
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 6ca145362c..71196b0a4b 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -385,7 +385,7 @@ static void aspeed_machine_init(MachineState *machine)
DriveInfo *emmc0 = NULL;
bool boot_emmc;
- bmc->soc = ASPEED_SOC(object_new(amc->soc_name));
+ bmc->soc = ASPEED_SOC(object_new_dynamic(amc->soc_name, &error_fatal));
object_property_add_child(OBJECT(machine), "soc", OBJECT(bmc->soc));
object_unref(OBJECT(bmc->soc));
sc = ASPEED_SOC_GET_CLASS(bmc->soc);
@@ -1594,7 +1594,7 @@ static void aspeed_minibmc_machine_init(MachineState *machine)
sysclk = clock_new(OBJECT(machine), "SYSCLK");
clock_set_hz(sysclk, SYSCLK_FRQ);
- bmc->soc = ASPEED_SOC(object_new(amc->soc_name));
+ bmc->soc = ASPEED_SOC(object_new_dynamic(amc->soc_name, &error_fatal));
object_property_add_child(OBJECT(machine), "soc", OBJECT(bmc->soc));
object_unref(OBJECT(bmc->soc));
qdev_connect_clock_in(DEVICE(bmc->soc), "sysclk", sysclk);
diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
index e3f1de2631..b966b0cd06 100644
--- a/hw/arm/exynos4210.c
+++ b/hw/arm/exynos4210.c
@@ -554,7 +554,8 @@ static void exynos4210_realize(DeviceState *socdev, Error **errp)
int i, n;
for (n = 0; n < EXYNOS4210_NCPUS; n++) {
- Object *cpuobj = object_new(ARM_CPU_TYPE_NAME("cortex-a9"));
+ Object *cpuobj = object_new_dynamic(ARM_CPU_TYPE_NAME("cortex-a9"),
+ &error_fatal);
object_property_add_child(OBJECT(s), "cpu[*]", cpuobj);
/* By default A9 CPUs have EL3 enabled. This board does not currently
diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index f103921d49..07740eecdb 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -206,7 +206,7 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
Object *cpuobj;
ARMCPU *cpu;
- cpuobj = object_new(machine->cpu_type);
+ cpuobj = object_new_dynamic(machine->cpu_type, &error_abort);
cpu = ARM_CPU(cpuobj);
object_property_add_child(OBJECT(machine), "cpu[*]", cpuobj);
diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index feb0dd63df..2d1adfd7f8 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -595,7 +595,7 @@ static void integratorcp_init(MachineState *machine)
DriveInfo *dinfo;
int i;
- cpuobj = object_new(machine->cpu_type);
+ cpuobj = object_new_dynamic(machine->cpu_type, &error_fatal);
/* By default ARM1176 CPUs have EL3 enabled. This board does not
* currently support EL3 so the CPU EL3 property is disabled before
diff --git a/hw/arm/mps3r.c b/hw/arm/mps3r.c
index 4d55a6564c..60c2138b4a 100644
--- a/hw/arm/mps3r.c
+++ b/hw/arm/mps3r.c
@@ -387,7 +387,8 @@ static void mps3r_common_init(MachineState *machine)
memory_region_add_subregion_overlap(&mms->cpu_sysmem[i], 0,
&mms->sysmem_alias[i], -1);
- mms->cpu[i] = object_new(machine->cpu_type);
+ mms->cpu[i] = object_new_dynamic(machine->cpu_type,
+ &error_fatal);
object_property_set_link(mms->cpu[i], "memory",
OBJECT(&mms->cpu_sysmem[i]), &error_abort);
object_property_set_int(mms->cpu[i], "reset-cbar",
diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index b186f965c6..214c03fb20 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -116,7 +116,8 @@ static void realview_init(MachineState *machine,
}
for (n = 0; n < smp_cpus; n++) {
- Object *cpuobj = object_new(machine->cpu_type);
+ Object *cpuobj = object_new_dynamic(machine->cpu_type,
+ &error_fatal);
/* By default A9,A15 and ARM1176 CPUs have EL3 enabled. This board
* does not currently support EL3 so the CPU EL3 property is disabled
diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index e3195d5449..a0006c9af0 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -767,7 +767,8 @@ static void sbsa_ref_init(MachineState *machine)
break;
}
- cpuobj = object_new(possible_cpus->cpus[n].type);
+ cpuobj = object_new_dynamic(possible_cpus->cpus[n].type,
+ &error_fatal);
object_property_set_int(cpuobj, "mp-affinity",
possible_cpus->cpus[n].arch_id, NULL);
diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index d48235453e..eb77dfc593 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -205,7 +205,7 @@ static void versatile_init(MachineState *machine, int board_id)
exit(1);
}
- cpuobj = object_new(machine->cpu_type);
+ cpuobj = object_new_dynamic(machine->cpu_type, &error_fatal);
/* By default ARM1176 CPUs have EL3 enabled. This board does not
* currently support EL3 so the CPU EL3 property is disabled before
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index de815d84cc..98ad6299a8 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -217,7 +217,7 @@ static void init_cpus(MachineState *ms, const char *cpu_type,
/* Create the actual CPUs */
for (n = 0; n < smp_cpus; n++) {
- Object *cpuobj = object_new(cpu_type);
+ Object *cpuobj = object_new_dynamic(cpu_type, &error_abort);
if (!secure) {
object_property_set_bool(cpuobj, "has_el3", false, NULL);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 1a381e9a2b..f80ab50d41 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2125,7 +2125,8 @@ static void machvirt_init(MachineState *machine)
* we are about to deal with. Once this is done, get rid of
* the object.
*/
- cpuobj = object_new(possible_cpus->cpus[0].type);
+ cpuobj = object_new_dynamic(possible_cpus->cpus[0].type,
+ &error_fatal);
armcpu = ARM_CPU(cpuobj);
pa_bits = arm_pamax(armcpu);
@@ -2231,7 +2232,8 @@ static void machvirt_init(MachineState *machine)
break;
}
- cpuobj = object_new(possible_cpus->cpus[n].type);
+ cpuobj = object_new_dynamic(possible_cpus->cpus[n].type,
+ &error_fatal);
object_property_set_int(cpuobj, "mp-affinity",
possible_cpus->cpus[n].arch_id, NULL);
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index fde4d946b7..40a725782d 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -218,7 +218,8 @@ static void zynq_init(MachineState *machine)
}
for (n = 0; n < smp_cpus; n++) {
- Object *cpuobj = object_new(machine->cpu_type);
+ Object *cpuobj = object_new_dynamic(machine->cpu_type,
+ &error_fatal);
object_property_set_int(cpuobj, "midr", ZYNQ_BOARD_MIDR,
&error_fatal);
diff --git a/hw/core/bus.c b/hw/core/bus.c
index b9d89495cd..2de714b274 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -163,7 +163,7 @@ BusState *qbus_new(const char *typename, DeviceState *parent, const char *name)
{
BusState *bus;
- bus = BUS(object_new(typename));
+ bus = BUS(object_new_dynamic(typename, &error_abort));
qbus_init_internal(bus, parent, name);
return bus;
diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index 09c7903594..8768ae39ed 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -57,7 +57,7 @@ bool cpu_exists(int64_t id)
CPUState *cpu_create(const char *typename)
{
Error *err = NULL;
- CPUState *cpu = CPU(object_new(typename));
+ CPUState *cpu = CPU(object_new_dynamic(typename, &error_abort));
if (!qdev_realize(DEVICE(cpu), NULL, &err)) {
error_report_err(err);
object_unref(OBJECT(cpu));
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 960a704a96..7fcbbe431b 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -146,7 +146,7 @@ bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp)
DeviceState *qdev_new(const char *name)
{
- return DEVICE(object_new(name));
+ return DEVICE(object_new_dynamic(name, &error_abort));
}
static QTAILQ_HEAD(, DeviceListener) device_listeners
diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
index bc360a9ea4..df38235eeb 100644
--- a/hw/i386/x86-common.c
+++ b/hw/i386/x86-common.c
@@ -55,7 +55,10 @@ static size_t pvh_start_addr;
static void x86_cpu_new(X86MachineState *x86ms, int64_t apic_id, Error **errp)
{
- Object *cpu = object_new(MACHINE(x86ms)->cpu_type);
+ Object *cpu = object_new_dynamic(MACHINE(x86ms)->cpu_type, errp);
+ if (!cpu) {
+ return;
+ }
if (!object_property_set_uint(cpu, "apic-id", apic_id, errp)) {
goto out;
diff --git a/hw/i386/xen/xen-pvh.c b/hw/i386/xen/xen-pvh.c
index f1f02d3311..9aeb4e430b 100644
--- a/hw/i386/xen/xen-pvh.c
+++ b/hw/i386/xen/xen-pvh.c
@@ -28,7 +28,7 @@ struct XenPVHx86State {
static DeviceState *xen_pvh_cpu_new(MachineState *ms,
int64_t apic_id)
{
- Object *cpu = object_new(ms->cpu_type);
+ Object *cpu = object_new_dynamic(ms->cpu_type, &error_abort);
object_property_add_child(OBJECT(ms), "cpu[*]", cpu);
object_property_set_uint(cpu, "apic-id", apic_id, &error_fatal);
diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index e893363dc9..2f06193bcd 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -377,7 +377,10 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
{
Object *obj;
- obj = object_new(type);
+ obj = object_new_dynamic(type, errp);
+ if (!obj) {
+ return NULL;
+ }
object_property_add_child(cpu, type, obj);
object_unref(obj);
object_property_set_link(obj, ICP_PROP_XICS, OBJECT(xi), &error_abort);
diff --git a/hw/mips/cps.c b/hw/mips/cps.c
index 13046628cd..b8407bb191 100644
--- a/hw/mips/cps.c
+++ b/hw/mips/cps.c
@@ -74,7 +74,8 @@ static void mips_cps_realize(DeviceState *dev, Error **errp)
}
for (int i = 0; i < s->num_vp; i++) {
- MIPSCPU *cpu = MIPS_CPU(object_new(s->cpu_type));
+ MIPSCPU *cpu = MIPS_CPU(object_new_dynamic(s->cpu_type,
+ &error_abort));
CPUMIPSState *env = &cpu->env;
object_property_set_bool(OBJECT(cpu), "big-endian", s->cpu_is_bigendian,
diff --git a/hw/pci-host/pnv_phb.c b/hw/pci-host/pnv_phb.c
index d4c118d443..6e833835d6 100644
--- a/hw/pci-host/pnv_phb.c
+++ b/hw/pci-host/pnv_phb.c
@@ -131,7 +131,10 @@ static void pnv_phb_realize(DeviceState *dev, Error **errp)
g_assert_not_reached();
}
- phb->backend = object_new(phb_typename);
+ phb->backend = object_new_dynamic(phb_typename, errp);
+ if (!phb->backend) {
+ return;
+ }
object_property_add_child(OBJECT(dev), "phb-backend", phb->backend);
/* Passthrough child device properties to the proxy device */
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 46261223f3..8ba34f0972 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -939,7 +939,7 @@ void ppce500_init(MachineState *machine)
PowerPCCPU *cpu;
CPUState *cs;
- cpu = POWERPC_CPU(object_new(machine->cpu_type));
+ cpu = POWERPC_CPU(object_new_dynamic(machine->cpu_type, &error_fatal));
env = &cpu->env;
cs = CPU(cpu);
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index f0f0d7567d..75420c9413 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1459,7 +1459,7 @@ static void pnv_chip_power8_instance_init(Object *obj)
chip8->num_phbs = pcc->num_phbs;
for (i = 0; i < chip8->num_phbs; i++) {
- Object *phb = object_new(TYPE_PNV_PHB);
+ Object *phb = object_new_dynamic(TYPE_PNV_PHB, &error_fatal);
/*
* We need the chip to parent the PHB to allow the DT
@@ -2376,7 +2376,7 @@ static void pnv_chip_core_realize(PnvChip *chip, Error **errp)
continue;
}
- pnv_core = PNV_CORE(object_new(typename));
+ pnv_core = PNV_CORE(object_new_dynamic(typename, &error_fatal));
snprintf(core_name, sizeof(core_name), "core[%d]", core_hwid);
object_property_add_child(OBJECT(chip), core_name, OBJECT(pnv_core));
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index a30693990b..4c62103021 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -361,7 +361,10 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
PowerPCCPU *cpu;
PnvCPUState *pnv_cpu;
- obj = object_new(typename);
+ obj = object_new_dynamic(typename, &local_err);
+ if (!obj) {
+ goto err;
+ }
cpu = POWERPC_CPU(obj);
pc->threads[i] = POWERPC_CPU(obj);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 5c02037c56..ca71242fb0 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2693,7 +2693,7 @@ static void spapr_init_cpus(SpaprMachineState *spapr)
}
if (i < boot_cores_nr) {
- Object *core = object_new(type);
+ Object *core = object_new_dynamic(type, &error_fatal);
int nr_threads = smp_threads;
/* Handle the partially filled core for older machine types */
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index ada439e831..aa9704e7ea 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -298,7 +298,10 @@ static PowerPCCPU *spapr_create_vcpu(SpaprCpuCore *sc, int i, Error **errp)
PowerPCCPU *cpu;
CPUPPCState *env;
- obj = object_new(scc->cpu_type);
+ obj = object_new_dynamic(scc->cpu_type, errp);
+ if (!obj) {
+ return NULL;
+ }
cs = CPU(obj);
cpu = POWERPC_CPU(obj);
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 1484e3209d..56835ebf2b 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -554,7 +554,7 @@ static void drc_unrealize(DeviceState *d)
SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
uint32_t id)
{
- SpaprDrc *drc = SPAPR_DR_CONNECTOR(object_new(type));
+ SpaprDrc *drc = SPAPR_DR_CONNECTOR(object_new_dynamic(type, &error_fatal));
g_autofree char *prop_name = NULL;
drc->id = id;
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index fe03f716f3..5441dc4c32 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -55,9 +55,15 @@ static Error *pv_mig_blocker;
static S390CPU *s390x_new_cpu(const char *typename, uint32_t core_id,
Error **errp)
{
- S390CPU *cpu = S390_CPU(object_new(typename));
+ Object *cpuobj = object_new_dynamic(typename, errp);
+ S390CPU *cpu = NULL;
S390CPU *ret = NULL;
+ if (!cpuobj) {
+ return NULL;
+ }
+
+ cpu = S390_CPU(cpuobj);
if (!object_property_set_int(OBJECT(cpu), "core-id", core_id, errp)) {
goto out;
}
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 6aaa04cb19..519169af1e 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -277,7 +277,7 @@ static void leon3_generic_hw_init(MachineState *machine)
for (i = 0; i < machine->smp.cpus; i++) {
/* Init CPU */
- cpu = SPARC_CPU(object_new(machine->cpu_type));
+ cpu = SPARC_CPU(object_new_dynamic(machine->cpu_type, &error_fatal));
qdev_init_gpio_in_named(DEVICE(cpu), leon3_start_cpu, "start_cpu", 1);
qdev_init_gpio_in_named(DEVICE(cpu), leon3_set_pil_in, "pil", 1);
qdev_realize(DEVICE(cpu), NULL, &error_fatal);
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index d52e6a7213..c7e47af1d5 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -802,7 +802,7 @@ static void cpu_devinit(const char *cpu_type, unsigned int id,
SPARCCPU *cpu;
CPUSPARCState *env;
- cpu = SPARC_CPU(object_new(cpu_type));
+ cpu = SPARC_CPU(object_new_dynamic(cpu_type, &error_fatal));
env = &cpu->env;
qemu_register_reset(sun4m_cpu_reset, cpu);
diff --git a/hw/sparc64/sparc64.c b/hw/sparc64/sparc64.c
index 3091cde586..9e032e6124 100644
--- a/hw/sparc64/sparc64.c
+++ b/hw/sparc64/sparc64.c
@@ -272,7 +272,7 @@ SPARCCPU *sparc64_cpu_devinit(const char *cpu_type, uint64_t prom_addr)
uint32_t stick_frequency = 100 * 1000000;
uint32_t hstick_frequency = 100 * 1000000;
- cpu = SPARC_CPU(object_new(cpu_type));
+ cpu = SPARC_CPU(object_new_dynamic(cpu_type, &error_fatal));
qdev_init_gpio_in_named(DEVICE(cpu), sparc64_cpu_set_ivec_irq,
"ivec-irq", IVEC_MAX);
qdev_realize(DEVICE(cpu), NULL, &error_fatal);
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index dcef44fe55..9799fd8627 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1550,7 +1550,11 @@ bool vfio_attach_device(char *name, VFIODevice *vbasedev,
if (!vbasedev->mdev) {
- hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename));
+ Object *obj = object_new_dynamic(ops->hiod_typename, errp);
+ if (!obj) {
+ return false;
+ }
+ hiod = HOST_IOMMU_DEVICE(obj);
vbasedev->hiod = hiod;
}
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 9ccdb639ac..6642d2f79b 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -419,6 +419,7 @@ static VFIOContainer *vfio_create_container(int fd, VFIOGroup *group,
{
int iommu_type;
const char *vioc_name;
+ Object *obj;
VFIOContainer *container;
iommu_type = vfio_get_iommu_type(fd, errp);
@@ -432,7 +433,11 @@ static VFIOContainer *vfio_create_container(int fd, VFIOGroup *group,
vioc_name = vfio_get_iommu_class_name(iommu_type);
- container = VFIO_IOMMU_LEGACY(object_new(vioc_name));
+ obj = object_new_dynamic(vioc_name, errp);
+ if (!obj) {
+ return NULL;
+ }
+ container = VFIO_IOMMU_LEGACY(obj);
container->fd = fd;
container->iommu_type = iommu_type;
return container;
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index 4a8e269fef..a32855659e 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -203,7 +203,10 @@ ObjectPropertyInfoList *qmp_qom_list_properties(const char *typename,
if (object_class_is_abstract(klass)) {
object_class_property_iter_init(&iter, klass);
} else {
- obj = object_new(typename);
+ obj = object_new_dynamic(typename, errp);
+ if (!obj) {
+ return NULL;
+ }
object_property_iter_init(&iter, obj);
}
while ((prop = object_property_iter_next(&iter))) {
diff --git a/target/arm/arm-qmp-cmds.c b/target/arm/arm-qmp-cmds.c
index 3cc8cc738b..61024f480d 100644
--- a/target/arm/arm-qmp-cmds.c
+++ b/target/arm/arm-qmp-cmds.c
@@ -150,7 +150,10 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
}
}
- obj = object_new(object_class_get_name(oc));
+ obj = object_new_dynamic(object_class_get_name(oc), errp);
+ if (!obj) {
+ return NULL;
+ }
if (model->props) {
Visitor *visitor;
diff --git a/target/loongarch/loongarch-qmp-cmds.c b/target/loongarch/loongarch-qmp-cmds.c
index 782fd511fd..5f565e2236 100644
--- a/target/loongarch/loongarch-qmp-cmds.c
+++ b/target/loongarch/loongarch-qmp-cmds.c
@@ -83,7 +83,10 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
return NULL;
}
- obj = object_new(object_class_get_name(oc));
+ obj = object_new_dynamic(object_class_get_name(oc), errp);
+ if (!obj) {
+ return NULL;
+ }
expansion_info = g_new0(CpuModelExpansionInfo, 1);
expansion_info->model = g_malloc0(sizeof(*expansion_info->model));
diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index d0a43b6d5c..1bf872c3e0 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -648,7 +648,7 @@ MIPSCPU *mips_cpu_create_with_clock(const char *cpu_type, Clock *cpu_refclk,
{
DeviceState *cpu;
- cpu = DEVICE(object_new(cpu_type));
+ cpu = DEVICE(object_new_dynamic(cpu_type, &error_abort));
qdev_connect_clock_in(cpu, "clk-in", cpu_refclk);
object_property_set_bool(OBJECT(cpu), "big-endian", is_big_endian,
&error_abort);
diff --git a/target/riscv/riscv-qmp-cmds.c b/target/riscv/riscv-qmp-cmds.c
index d363dc318d..61f30b6bc6 100644
--- a/target/riscv/riscv-qmp-cmds.c
+++ b/target/riscv/riscv-qmp-cmds.c
@@ -188,7 +188,10 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
return NULL;
}
- obj = object_new(object_class_get_name(oc));
+ obj = object_new_dynamic(object_class_get_name(oc), errp);
+ if (!obj) {
+ return NULL;
+ }
riscv_check_if_cpu_available(RISCV_CPU(obj), &local_err);
if (local_err != NULL) {
diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
index 6f9039abae..3e036a1191 100644
--- a/target/xtensa/cpu.c
+++ b/target/xtensa/cpu.c
@@ -204,7 +204,7 @@ XtensaCPU *xtensa_cpu_create_with_clock(const char *cpu_type, Clock *cpu_refclk)
{
DeviceState *cpu;
- cpu = DEVICE(object_new(cpu_type));
+ cpu = DEVICE(object_new_dynamic(cpu_type, &error_abort));
qdev_connect_clock_in(cpu, "clk-in", cpu_refclk);
qdev_realize(cpu, NULL, &error_abort);
diff --git a/tests/unit/check-qom-interface.c b/tests/unit/check-qom-interface.c
index c99be97ed8..e4532ae814 100644
--- a/tests/unit/check-qom-interface.c
+++ b/tests/unit/check-qom-interface.c
@@ -13,6 +13,7 @@
#include "qom/object.h"
#include "qemu/module.h"
+#include "qapi/error.h"
#define TYPE_TEST_IF "test-interface"
@@ -67,7 +68,7 @@ static const TypeInfo intermediate_impl_info = {
static void test_interface_impl(const char *type)
{
- Object *obj = object_new(type);
+ Object *obj = object_new_dynamic(type, &error_abort);
TestIf *iobj = TEST_IF(obj);
TestIfClass *ioc = TEST_IF_GET_CLASS(iobj);
diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index f9bccb56ab..f4893d5f24 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -1008,7 +1008,7 @@ static void machine_full_topo_class_init(ObjectClass *oc, void *data)
static void test_generic_valid(const void *opaque)
{
const char *machine_type = opaque;
- Object *obj = object_new(machine_type);
+ Object *obj = object_new_dynamic(machine_type, &error_abort);
MachineState *ms = MACHINE(obj);
MachineClass *mc = MACHINE_GET_CLASS(obj);
SMPTestData data = {};
@@ -1027,7 +1027,7 @@ static void test_generic_valid(const void *opaque)
static void test_generic_invalid(const void *opaque)
{
const char *machine_type = opaque;
- Object *obj = object_new(machine_type);
+ Object *obj = object_new_dynamic(machine_type, &error_abort);
MachineState *ms = MACHINE(obj);
MachineClass *mc = MACHINE_GET_CLASS(obj);
SMPTestData data = {};
@@ -1046,7 +1046,7 @@ static void test_generic_invalid(const void *opaque)
static void test_with_modules(const void *opaque)
{
const char *machine_type = opaque;
- Object *obj = object_new(machine_type);
+ Object *obj = object_new_dynamic(machine_type, &error_abort);
MachineState *ms = MACHINE(obj);
MachineClass *mc = MACHINE_GET_CLASS(obj);
SMPTestData data = {};
@@ -1096,7 +1096,7 @@ static void test_with_modules(const void *opaque)
static void test_with_dies(const void *opaque)
{
const char *machine_type = opaque;
- Object *obj = object_new(machine_type);
+ Object *obj = object_new_dynamic(machine_type, &error_abort);
MachineState *ms = MACHINE(obj);
MachineClass *mc = MACHINE_GET_CLASS(obj);
SMPTestData data = {};
@@ -1146,7 +1146,7 @@ static void test_with_dies(const void *opaque)
static void test_with_modules_dies(const void *opaque)
{
const char *machine_type = opaque;
- Object *obj = object_new(machine_type);
+ Object *obj = object_new_dynamic(machine_type, &error_abort);
MachineState *ms = MACHINE(obj);
MachineClass *mc = MACHINE_GET_CLASS(obj);
SMPTestData data = {};
@@ -1207,7 +1207,7 @@ static void test_with_modules_dies(const void *opaque)
static void test_with_clusters(const void *opaque)
{
const char *machine_type = opaque;
- Object *obj = object_new(machine_type);
+ Object *obj = object_new_dynamic(machine_type, &error_abort);
MachineState *ms = MACHINE(obj);
MachineClass *mc = MACHINE_GET_CLASS(obj);
SMPTestData data = {};
@@ -1257,7 +1257,7 @@ static void test_with_clusters(const void *opaque)
static void test_with_books(const void *opaque)
{
const char *machine_type = opaque;
- Object *obj = object_new(machine_type);
+ Object *obj = object_new_dynamic(machine_type, &error_abort);
MachineState *ms = MACHINE(obj);
MachineClass *mc = MACHINE_GET_CLASS(obj);
SMPTestData data = {};
@@ -1307,7 +1307,7 @@ static void test_with_books(const void *opaque)
static void test_with_drawers(const void *opaque)
{
const char *machine_type = opaque;
- Object *obj = object_new(machine_type);
+ Object *obj = object_new_dynamic(machine_type, &error_abort);
MachineState *ms = MACHINE(obj);
MachineClass *mc = MACHINE_GET_CLASS(obj);
SMPTestData data = {};
@@ -1357,7 +1357,7 @@ static void test_with_drawers(const void *opaque)
static void test_with_drawers_books(const void *opaque)
{
const char *machine_type = opaque;
- Object *obj = object_new(machine_type);
+ Object *obj = object_new_dynamic(machine_type, &error_abort);
MachineState *ms = MACHINE(obj);
MachineClass *mc = MACHINE_GET_CLASS(obj);
SMPTestData data = {};
@@ -1418,7 +1418,7 @@ static void test_with_drawers_books(const void *opaque)
static void test_full_topo(const void *opaque)
{
const char *machine_type = opaque;
- Object *obj = object_new(machine_type);
+ Object *obj = object_new_dynamic(machine_type, &error_abort);
MachineState *ms = MACHINE(obj);
MachineClass *mc = MACHINE_GET_CLASS(obj);
SMPTestData data = {};
--
2.46.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v3 6/9] qom: enforce use of static, const string with object_new()
2024-11-15 17:25 [PATCH v3 0/9] Require error handling for dynamically created objects Daniel P. Berrangé
` (4 preceding siblings ...)
2024-11-15 17:25 ` [PATCH v3 5/9] convert code to object_new_dynamic() where appropriate Daniel P. Berrangé
@ 2024-11-15 17:25 ` Daniel P. Berrangé
2024-11-15 17:25 ` [PATCH v3 7/9] qom: introduce qdev_new_dynamic() Daniel P. Berrangé
` (3 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2024-11-15 17:25 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Peter Xu, Markus Armbruster,
Daniel P. Berrangé
Since object_new() will assert(), it should only be used in scenarios
where the caller knows exactly what type it is asking to be created,
and can thus be confident in avoiding abstract types.
Enforce this by using a macro wrapper which types to paste "" to the
type name. This will generate a compile error if not passed a static
const string, forcing callers to use object_new_dynamic() instead.
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
include/qom/object.h | 12 +++++++++++-
qom/object.c | 3 ++-
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/include/qom/object.h b/include/qom/object.h
index 4fc01336c4..2d5a0d84b5 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -637,7 +637,17 @@ Object *object_new_with_class(ObjectClass *klass, Error **errp);
*
* Returns: The newly allocated and instantiated object.
*/
-Object *object_new(const char *typename);
+
+/*
+ * NB, object_new_internal is just an internal helper, wrapped by
+ * the object_new() macro which prevents invokation unless given
+ * a static, const string.
+ *
+ * Code should call object_new(), or object_new_dynamic(), not
+ * object_new_internal().
+ */
+Object *object_new_internal(const char *typename);
+#define object_new(typename) object_new_internal(typename "")
/**
* object_new_dynamic:
diff --git a/qom/object.c b/qom/object.c
index 42ef40a1fd..ee56710348 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -799,7 +799,8 @@ Object *object_new_with_class(ObjectClass *klass, Error **errp)
return object_new_with_type(klass->type, errp);
}
-Object *object_new(const char *typename)
+/* Only to be called via the 'object_new' macro */
+Object *object_new_internal(const char *typename)
{
TypeImpl *ti = type_get_or_load_by_name(typename, &error_fatal);
--
2.46.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v3 7/9] qom: introduce qdev_new_dynamic()
2024-11-15 17:25 [PATCH v3 0/9] Require error handling for dynamically created objects Daniel P. Berrangé
` (5 preceding siblings ...)
2024-11-15 17:25 ` [PATCH v3 6/9] qom: enforce use of static, const string with object_new() Daniel P. Berrangé
@ 2024-11-15 17:25 ` Daniel P. Berrangé
2024-11-15 17:55 ` Peter Xu
2024-11-15 17:25 ` [PATCH v3 8/9] convert code to qdev_new_dynamic() where appropriate Daniel P. Berrangé
` (2 subsequent siblings)
9 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2024-11-15 17:25 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Peter Xu, Markus Armbruster,
Daniel P. Berrangé
qdev_new() has a failure scenario where it will assert() if given
an abstract type. Callers which are creating qdevs based on user
input, or unknown/untrusted type names, must manually check the
result of qdev_class_is_abstract() before calling qdev_new()
to propagate an Error, instead of asserting.
Introduce a qdev_new_dynamic() method which is a counterpart to
qdev_new() that directly returns an Error, instead of asserting.
This new method is to be used where the typename is specified
dynamically by code separate from the immediate caller.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
hw/core/qdev.c | 5 +++++
include/hw/qdev-core.h | 27 +++++++++++++++++++++++++++
2 files changed, 32 insertions(+)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 7fcbbe431b..eceba33222 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -149,6 +149,11 @@ DeviceState *qdev_new(const char *name)
return DEVICE(object_new_dynamic(name, &error_abort));
}
+DeviceState *qdev_new_dynamic(const char *name, Error **errp)
+{
+ return DEVICE(object_new_dynamic(name, errp));
+}
+
static QTAILQ_HEAD(, DeviceListener) device_listeners
= QTAILQ_HEAD_INITIALIZER(device_listeners);
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 020417d027..566c5ef277 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -435,14 +435,41 @@ compat_props_add(GPtrArray *arr,
* qdev_new: Create a device on the heap
* @name: device type to create (we assert() that this type exists)
*
+ * This method should be used where @name is statically specified
+ * from a const string at build time, where the caller does not expect
+ * failure to be possible.
+ *
* This only allocates the memory and initializes the device state
* structure, ready for the caller to set properties if they wish.
* The device still needs to be realized.
*
+ * If an instance of @name is not permitted to be instantiated, an
+ * assert will be raised. This can happen if @name is abstract.
+ *
* Return: a derived DeviceState object with a reference count of 1.
*/
DeviceState *qdev_new(const char *name);
+/**
+ * qdev_new_dynamic: Create a device on the heap
+ * @name: device type to create (we assert() that this type exists)
+ * @errp: pointer to be filled with error details on failure
+ *
+ * This method must be used where @name is dynamically chosen
+ * at runtime, which has the possibility of unexpected choices leading
+ * to failures.
+ *
+ * This only allocates the memory and initializes the device state
+ * structure, ready for the caller to set properties if they wish.
+ * The device still needs to be realized.
+ *
+ * If an instance of @name is not permitted to be instantiated, an
+ * error will be reported. This can happen if @name is abstract.
+ *
+ * Return: a derived DeviceState object with a reference count of 1.
+ */
+DeviceState *qdev_new_dynamic(const char *name, Error **errp);
+
/**
* qdev_is_realized() - check if device is realized
* @dev: The device to check.
--
2.46.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v3 7/9] qom: introduce qdev_new_dynamic()
2024-11-15 17:25 ` [PATCH v3 7/9] qom: introduce qdev_new_dynamic() Daniel P. Berrangé
@ 2024-11-15 17:55 ` Peter Xu
0 siblings, 0 replies; 22+ messages in thread
From: Peter Xu @ 2024-11-15 17:55 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel, Paolo Bonzini, Markus Armbruster
On Fri, Nov 15, 2024 at 05:25:19PM +0000, Daniel P. Berrangé wrote:
> qdev_new() has a failure scenario where it will assert() if given
> an abstract type. Callers which are creating qdevs based on user
> input, or unknown/untrusted type names, must manually check the
> result of qdev_class_is_abstract() before calling qdev_new()
> to propagate an Error, instead of asserting.
>
> Introduce a qdev_new_dynamic() method which is a counterpart to
> qdev_new() that directly returns an Error, instead of asserting.
> This new method is to be used where the typename is specified
> dynamically by code separate from the immediate caller.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v3 8/9] convert code to qdev_new_dynamic() where appropriate
2024-11-15 17:25 [PATCH v3 0/9] Require error handling for dynamically created objects Daniel P. Berrangé
` (6 preceding siblings ...)
2024-11-15 17:25 ` [PATCH v3 7/9] qom: introduce qdev_new_dynamic() Daniel P. Berrangé
@ 2024-11-15 17:25 ` Daniel P. Berrangé
2024-11-15 17:25 ` [PATCH v3 9/9] hw: enforce use of static, const string with qdev_new() Daniel P. Berrangé
2024-12-04 11:07 ` [PATCH v3 0/9] Require error handling for dynamically created objects Markus Armbruster
9 siblings, 0 replies; 22+ messages in thread
From: Daniel P. Berrangé @ 2024-11-15 17:25 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Peter Xu, Markus Armbruster,
Daniel P. Berrangé
In cases where qdev_new() is not being passed a static, const
string, the caller cannot be sure what type they are instantiating.
There is a risk that instantiation could fail, if it is an abstract
type.
Convert such cases over to use qdev_new_dynamic() such that they
are forced to expect failure. In some cases failure can be easily
propagated, but in others using &error_abort or &error_fatal will
maintain existing behaviour.
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
hw/arm/aspeed.c | 2 +-
hw/arm/npcm7xx_boards.c | 2 +-
hw/arm/sbsa-ref.c | 4 ++--
hw/arm/vexpress.c | 2 +-
hw/arm/virt.c | 4 ++--
hw/audio/soundhw.c | 2 +-
hw/block/xen-block.c | 7 ++++++-
hw/core/sysbus.c | 2 +-
hw/i2c/core.c | 2 +-
hw/isa/isa-bus.c | 2 +-
hw/pci/pci.c | 2 +-
hw/ppc/pnv.c | 2 +-
hw/s390x/s390-virtio-ccw.c | 2 +-
hw/scsi/scsi-bus.c | 5 ++++-
hw/ssi/ssi.c | 2 +-
include/hw/usb.h | 2 +-
include/qom/object.h | 4 ++--
net/net.c | 4 ++--
system/qdev-monitor.c | 5 ++++-
19 files changed, 34 insertions(+), 23 deletions(-)
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 71196b0a4b..19dbcee64a 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -320,7 +320,7 @@ void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
DriveInfo *dinfo = drive_get(IF_MTD, 0, unit0 + i);
DeviceState *dev;
- dev = qdev_new(flashtype);
+ dev = qdev_new_dynamic(flashtype, &error_fatal);
if (dinfo) {
qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
}
diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
index e229efb447..098beeab63 100644
--- a/hw/arm/npcm7xx_boards.c
+++ b/hw/arm/npcm7xx_boards.c
@@ -83,7 +83,7 @@ static void npcm7xx_connect_flash(NPCM7xxFIUState *fiu, int cs_no,
DeviceState *flash;
qemu_irq flash_cs;
- flash = qdev_new(flash_type);
+ flash = qdev_new_dynamic(flash_type, &error_fatal);
if (dinfo) {
qdev_prop_set_drive(flash, "drive", blk_by_legacy_dinfo(dinfo));
}
diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index a0006c9af0..12e0a70981 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -419,7 +419,7 @@ static void create_its(SBSAMachineState *sms)
const char *itsclass = its_class_name();
DeviceState *dev;
- dev = qdev_new(itsclass);
+ dev = qdev_new_dynamic(itsclass, &error_fatal);
object_property_set_link(OBJECT(dev), "parent-gicv3", OBJECT(sms->gic),
&error_abort);
@@ -438,7 +438,7 @@ static void create_gic(SBSAMachineState *sms, MemoryRegion *mem)
gictype = gicv3_class_name();
- sms->gic = qdev_new(gictype);
+ sms->gic = qdev_new_dynamic(gictype, &error_fatal);
qdev_prop_set_uint32(sms->gic, "revision", 3);
qdev_prop_set_uint32(sms->gic, "num-cpu", smp_cpus);
/*
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 98ad6299a8..e13c66b838 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -239,7 +239,7 @@ static void init_cpus(MachineState *ms, const char *cpu_type,
* this must happen after the CPUs are created because a15mpcore_priv
* wires itself up to the CPU's generic_timer gpio out lines.
*/
- dev = qdev_new(privdev);
+ dev = qdev_new_dynamic(privdev, &error_fatal);
qdev_prop_set_uint32(dev, "num-cpu", smp_cpus);
busdev = SYS_BUS_DEVICE(dev);
sysbus_realize_and_unref(busdev, &error_fatal);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index f80ab50d41..57b1735380 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -715,7 +715,7 @@ static void create_its(VirtMachineState *vms)
return;
}
- dev = qdev_new(itsclass);
+ dev = qdev_new_dynamic(itsclass, &error_fatal);
object_property_set_link(OBJECT(dev), "parent-gicv3", OBJECT(vms->gic),
&error_abort);
@@ -791,7 +791,7 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
default:
g_assert_not_reached();
}
- vms->gic = qdev_new(gictype);
+ vms->gic = qdev_new_dynamic(gictype, &error_fatal);
qdev_prop_set_uint32(vms->gic, "revision", revision);
qdev_prop_set_uint32(vms->gic, "num-cpu", smp_cpus);
/* Note that the num-irq property counts both internal and external
diff --git a/hw/audio/soundhw.c b/hw/audio/soundhw.c
index d18fd9fa05..17ed3a8084 100644
--- a/hw/audio/soundhw.c
+++ b/hw/audio/soundhw.c
@@ -132,7 +132,7 @@ void soundhw_init(void)
}
if (c->typename) {
- DeviceState *dev = qdev_new(c->typename);
+ DeviceState *dev = qdev_new_dynamic(c->typename, &error_fatal);
qdev_prop_set_string(dev, "audiodev", audiodev_id);
qdev_realize_and_unref(dev, bus, &error_fatal);
} else {
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index aed1d5c330..fd5fa7b6e7 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -1034,6 +1034,7 @@ static void xen_block_device_create(XenBackendInstance *backend,
XenDevice *xendev = NULL;
const char *type;
XenBlockDevice *blockdev;
+ DeviceState *dev;
if (qemu_strtoul(name, NULL, 10, &number)) {
error_setg(errp, "failed to parse name '%s'", name);
@@ -1075,7 +1076,11 @@ static void xen_block_device_create(XenBackendInstance *backend,
goto fail;
}
- xendev = XEN_DEVICE(qdev_new(type));
+ dev = qdev_new_dynamic(type, errp);
+ if (!dev) {
+ goto fail;
+ }
+ xendev = XEN_DEVICE(dev);
blockdev = XEN_BLOCK_DEVICE(xendev);
if (!object_property_set_str(OBJECT(xendev), "vdev", vdev,
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index e64d99c8ed..e472a969f1 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -221,7 +221,7 @@ DeviceState *sysbus_create_varargs(const char *name,
qemu_irq irq;
int n;
- dev = qdev_new(name);
+ dev = qdev_new_dynamic(name, &error_fatal);
s = SYS_BUS_DEVICE(dev);
sysbus_realize_and_unref(s, &error_fatal);
if (addr != (hwaddr)-1) {
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 4cf30b2c86..a430f4e767 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -369,7 +369,7 @@ I2CSlave *i2c_slave_new(const char *name, uint8_t addr)
{
DeviceState *dev;
- dev = qdev_new(name);
+ dev = qdev_new_dynamic(name, &error_fatal);
qdev_prop_set_uint8(dev, "address", addr);
return I2C_SLAVE(dev);
}
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 8aaf44a3ef..34330547cb 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -155,7 +155,7 @@ int isa_register_portio_list(ISADevice *dev,
ISADevice *isa_new(const char *name)
{
- return ISA_DEVICE(qdev_new(name));
+ return ISA_DEVICE(qdev_new_dynamic(name, &error_fatal));
}
ISADevice *isa_create_simple(ISABus *bus, const char *name)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 1416ae202c..51338320c1 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2186,7 +2186,7 @@ static PCIDevice *pci_new_internal(int devfn, bool multifunction,
{
DeviceState *dev;
- dev = qdev_new(name);
+ dev = qdev_new_dynamic(name, &error_fatal);
qdev_prop_set_int32(dev, "addr", devfn);
qdev_prop_set_bit(dev, "multifunction", multifunction);
return PCI_DEVICE(dev);
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 75420c9413..e81c562967 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1115,7 +1115,7 @@ static void pnv_init(MachineState *machine)
pnv->chips = g_new0(PnvChip *, pnv->num_chips);
for (i = 0; i < pnv->num_chips; i++) {
char chip_name[32];
- Object *chip = OBJECT(qdev_new(chip_typename));
+ Object *chip = OBJECT(qdev_new_dynamic(chip_typename, &error_fatal));
uint64_t chip_ram_size = pnv_chip_get_ram_size(pnv, i);
pnv->chips[i] = PNV_CHIP(chip);
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 5441dc4c32..b276b5e77f 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -242,7 +242,7 @@ static void s390_create_sclpconsole(SCLPDevice *sclp,
BusState *ev_fac_bus = sclp_get_event_facility_bus(ef);
DeviceState *dev;
- dev = qdev_new(type);
+ dev = qdev_new_dynamic(type, &error_fatal);
object_property_add_child(OBJECT(ef), type, OBJECT(dev));
qdev_prop_set_chr(dev, "chardev", chardev);
qdev_realize_and_unref(dev, ev_fac_bus, &error_fatal);
diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 53eff5dd3d..23be8ebca4 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -396,7 +396,10 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, BlockBackend *blk,
driver = "scsi-hd";
}
}
- dev = qdev_new(driver);
+ dev = qdev_new_dynamic(driver, errp);
+ if (!dev) {
+ return NULL;
+ }
name = g_strdup_printf("legacy[%d]", unit);
object_property_add_child(OBJECT(bus), name, OBJECT(dev));
g_free(name);
diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
index 3f357e8f16..712bb1781c 100644
--- a/hw/ssi/ssi.c
+++ b/hw/ssi/ssi.c
@@ -141,7 +141,7 @@ bool ssi_realize_and_unref(DeviceState *dev, SSIBus *bus, Error **errp)
DeviceState *ssi_create_peripheral(SSIBus *bus, const char *name)
{
- DeviceState *dev = qdev_new(name);
+ DeviceState *dev = qdev_new_dynamic(name, &error_fatal);
ssi_realize_and_unref(dev, bus, &error_fatal);
return dev;
diff --git a/include/hw/usb.h b/include/hw/usb.h
index bb778cb844..bb90098d39 100644
--- a/include/hw/usb.h
+++ b/include/hw/usb.h
@@ -581,7 +581,7 @@ void usb_pcap_data(USBPacket *p, bool setup);
static inline USBDevice *usb_new(const char *name)
{
- return USB_DEVICE(qdev_new(name));
+ return USB_DEVICE(qdev_new_dynamic(name, &error_fatal));
}
static inline bool usb_realize_and_unref(USBDevice *dev, USBBus *bus, Error **errp)
diff --git a/include/qom/object.h b/include/qom/object.h
index 2d5a0d84b5..4e660da84a 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -654,7 +654,7 @@ Object *object_new_internal(const char *typename);
* @typename: The name of the type of the object to instantiate.
* @errp: pointer to be filled with error details on failure
*
- * This method should be used where @typename is dynamically chosen
+ * This method must be used where @typename is dynamically chosen
* at runtime, which has the possibility of unexpected choices leading
* to failures.
*
@@ -663,7 +663,7 @@ Object *object_new_internal(const char *typename);
* the last reference is dropped.
*
* If an instance of @typename is not permitted to be instantiated, an
- * error will be raised. This can happen if @typename is abstract.
+ * error will be reported. This can happen if @typename is abstract.
*
* Returns: The newly allocated and instantiated object.
*/
diff --git a/net/net.c b/net/net.c
index fbbfe602a4..fa89ec8e03 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1175,7 +1175,7 @@ DeviceState *qemu_create_nic_device(const char *typename, bool match_default,
return NULL;
}
- dev = qdev_new(typename);
+ dev = qdev_new_dynamic(typename, &error_fatal);
qdev_set_nic_properties(dev, nd);
return dev;
}
@@ -1225,7 +1225,7 @@ void qemu_create_nic_bus_devices(BusState *bus, const char *parent_type,
continue;
}
- dev = qdev_new(model);
+ dev = qdev_new_dynamic(model, &error_fatal);
qdev_set_nic_properties(dev, nd);
qdev_realize_and_unref(dev, bus, &error_fatal);
}
diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index 4c09b38ffb..3138192cf8 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -686,7 +686,10 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
}
/* create device */
- dev = qdev_new(driver);
+ dev = qdev_new_dynamic(driver, errp);
+ if (!dev) {
+ return NULL;
+ }
/* Check whether the hotplug is allowed by the machine */
if (phase_check(PHASE_MACHINE_READY)) {
--
2.46.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v3 9/9] hw: enforce use of static, const string with qdev_new()
2024-11-15 17:25 [PATCH v3 0/9] Require error handling for dynamically created objects Daniel P. Berrangé
` (7 preceding siblings ...)
2024-11-15 17:25 ` [PATCH v3 8/9] convert code to qdev_new_dynamic() where appropriate Daniel P. Berrangé
@ 2024-11-15 17:25 ` Daniel P. Berrangé
2024-11-15 17:55 ` Peter Xu
2024-12-04 11:07 ` [PATCH v3 0/9] Require error handling for dynamically created objects Markus Armbruster
9 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2024-11-15 17:25 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Peter Xu, Markus Armbruster,
Daniel P. Berrangé
Since qdev_new() will assert(), it should only be used in scenarios
where the caller knows exactly what type it is asking to be created,
and can thus be confident in avoiding abstract types.
Enforce this by using a macro wrapper which types to paste "" to the
type name. This will generate a compile error if not passed a static
const string, forcing callers to use qdev_new_dynamic() instead.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
hw/core/qdev.c | 3 ++-
include/hw/qdev-core.h | 12 +++++++++++-
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index eceba33222..968fa33a95 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -144,7 +144,8 @@ bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp)
return true;
}
-DeviceState *qdev_new(const char *name)
+/* Only to be called via the 'qdev_new' macro */
+DeviceState *qdev_new_internal(const char *name)
{
return DEVICE(object_new_dynamic(name, &error_abort));
}
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 566c5ef277..335dcd31b0 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -448,7 +448,17 @@ compat_props_add(GPtrArray *arr,
*
* Return: a derived DeviceState object with a reference count of 1.
*/
-DeviceState *qdev_new(const char *name);
+
+/*
+ * NB, qdev_new_internal is just an internal helper, wrapped by
+ * the qdev_new() macro which prevents invokation unless given
+ * a static, const string.
+ *
+ * Code should call qdev_new(), or qdev_new_dynamic(), not
+ * qdev_new_internal().
+ */
+DeviceState *qdev_new_internal(const char *name);
+#define qdev_new(name) qdev_new_internal(name "")
/**
* qdev_new_dynamic: Create a device on the heap
--
2.46.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v3 9/9] hw: enforce use of static, const string with qdev_new()
2024-11-15 17:25 ` [PATCH v3 9/9] hw: enforce use of static, const string with qdev_new() Daniel P. Berrangé
@ 2024-11-15 17:55 ` Peter Xu
0 siblings, 0 replies; 22+ messages in thread
From: Peter Xu @ 2024-11-15 17:55 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel, Paolo Bonzini, Markus Armbruster
On Fri, Nov 15, 2024 at 05:25:21PM +0000, Daniel P. Berrangé wrote:
> Since qdev_new() will assert(), it should only be used in scenarios
> where the caller knows exactly what type it is asking to be created,
> and can thus be confident in avoiding abstract types.
>
> Enforce this by using a macro wrapper which types to paste "" to the
> type name. This will generate a compile error if not passed a static
> const string, forcing callers to use qdev_new_dynamic() instead.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v3 0/9] Require error handling for dynamically created objects
2024-11-15 17:25 [PATCH v3 0/9] Require error handling for dynamically created objects Daniel P. Berrangé
` (8 preceding siblings ...)
2024-11-15 17:25 ` [PATCH v3 9/9] hw: enforce use of static, const string with qdev_new() Daniel P. Berrangé
@ 2024-12-04 11:07 ` Markus Armbruster
2024-12-05 16:04 ` Daniel P. Berrangé
9 siblings, 1 reply; 22+ messages in thread
From: Markus Armbruster @ 2024-12-04 11:07 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel, Paolo Bonzini, Peter Xu
Daniel P. Berrangé <berrange@redhat.com> writes:
> NB, this series is targetting 10.0, NOT for 9.2 freeze.
>
> With code like
>
> Object *obj = object_new(TYPE_BLAH)
>
> the caller can be pretty confident that they will successfully create
> an object instance of TYPE_BLAH. They know exactly what type has been
> requested, so it passing an abstract type for example, it is a clear
> programmer error that they'll get an assertion failure.
>
> Conversely with code like
>
> void somefunc(const char *typename) {
> Object * obj = object_new(typename)
> ...
> }
>
> all bets are off, because the call of object_new() knows nothing
> about what 'typename' resolves to.
We know nothing *locally*.
Commonly, a non-local argument can demonstrate safety. Only when the
type name comes from the user, we truly know nothing.
> It could easily be an abstract
> type.
It could also be no type at all.
> As a result, many code paths have added a manual check ahead
> of time
>
> if (object_class_is_abstract(typename)) {
> error_setg(errp, ....)
> }
Actually, object_class_is_abstract() takes an ObjectClass, not a type
name string.
The actual guards we use are variations of
klass = object_class_by_name(typename);
if (!klass) {
error_setg(errp, "invalid object type: %s", typename);
return NULL;
}
if (object_class_is_abstract(klass)) {
error_setg(errp, "object type '%s' is abstract", typename);
return NULL;
}
which covers "no type at all", too.
Sometimes, we use module_object_class_by_name() instead, which I believe
additionally loads the module providing the type, if any. Which of the
two should be used where is a mystery to me, and I suspect we're getting
it wrong in places. But this is turning into a digression. To
hopefully maintain focus, I'm pretending modules don't exist until later
in this message.
Sometimes, we throw in an object_class_dynamic_cast(klass, T) to check
@typename resolves to a subtype of some T.
> ...except for where we forget to do this, such as qdev_new().
We did not forget it there! It's by design a thin wrapper around
object_new(), with preconditions just like object_new().
> Overall 'object_new' is a bad design because it is inherantly
> unsafe to call with unvalidated typenames.
To be fair, object_new() was not designed for use with user-provided
type names. When it chokes on type names not provided by the user, it's
clearly a programming error, and assert() is a perfectly fine way to
catch programming errors. Same for qdev_new().
However, we do in fact use these functions with user-provided type
names, if rarely. When we do, we need to validate the type name before
we pass it to them.
Trouble is the validation code is a bit involved, and reimplementing it
everywhere it's needed is asking for bugs.
Creating and using more interfaces that are more convenient for this
purpose would avoid that.
> This problem is made worse by the proposal to introduce the idea
> of 'singleton' classes[1].
>
> Thus, this series suggests a way to improve safety at build
> time. The core idea is to allow 'object_new' to continue to be
> used *if-and-only-if* given a static, const string, because that
> scenario indicates the caller is aware of what type they are
> creating at build time.
>
> A new 'object_new_dynamic' method is proposed for cases where
> the typename is dynamically chosen at runtime. This method has
> an "Error **errp" parameter, which can report when an abstract
> type is created, leaving the assert()s only for scenarios which
> are unambiguous programmer errors.
>
> With a little macro magic, we guarantee a compile error is
> generated if 'object_new' is called with a dynamic type, forcing
> all potentially unsafe code over to object_new_dynamic.
Three cases:
1. Type name is literal string. No change. This is the most common
case.
2. It's not.
2a. Type name is user-provided. This is rare. We replace
if (... guard ...) {
... return failure ...
}
obj = object_new(...);
by
obj = object_new_dynamic(..., errp);
if (!obj) {
... return failure ...
}
This is an improvement.
2b. It's not. We should replace
obj = object_new(...);
by
obj = object_new_dynamic(..., &error_abort);
Exact same behavior, just wordier, to placate the compiler.
Tolerable as long as it's relatively rare.
But I'm not sure it's worthwhile. All it really does is helping
some towards not getting case 2a wrong. But 2a is rare.
Same for similar interfaces, e.g. qdev_new().
> This is more tractable than adding 'Error **errp' to 'object_new'
> as only a handful of places use a dynamic type name.
True!
Alright, enter modules.
Modules break a fundamental design assumption: object_new() on a
compiled-in type name is safe, i.e. the failure modes are all
programming errors.
Modules add new failure modes that are *not* programming errors:
* The module providing the type was not deployed correctly.
* It was, but the host system lacks the resources to load it.
Before modules, object_new(T) was safe unless T was user-provided.
Which implies it's safe when T is a literal string.
Since modules, object_new(T) is safe unless T is user-provided or the
type named by it is compiled as module. This does *not* imply it's safe
when T is a literal string.
When looking at a use of object_new(), whether the argument names a type
that could be compiled as module cannot be known locally. Therefore, we
cannot know locally whether we need to handle failure, either with a
suitable guard or by switching to a new function like
object_new_dynamic(). This is bad.
Breaking fundamental design assumptions tends to have ugly and expensive
consequences. Consequences like having to rework every single call of
object_new() & friends.
Can we reduce the damage? Maybe. What if we create a
module_object_new() that takes an Error **, and make object_new() crash
& burn when the @typename argument resolves to a type provided by a
module?
Maybe module_object_new() and object_new_dynamic() could be fused into a
single function with a better name.
> With this series, my objections to Peter Xu's singleton series[1]
> would be largely nullified.
>
> [1] https://lists.nongnu.org/archive/html/qemu-devel/2024-10/msg05524.html
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v3 0/9] Require error handling for dynamically created objects
2024-12-04 11:07 ` [PATCH v3 0/9] Require error handling for dynamically created objects Markus Armbruster
@ 2024-12-05 16:04 ` Daniel P. Berrangé
2024-12-06 8:25 ` Markus Armbruster
0 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2024-12-05 16:04 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Paolo Bonzini, Peter Xu
On Wed, Dec 04, 2024 at 12:07:58PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > NB, this series is targetting 10.0, NOT for 9.2 freeze.
> >
> > With code like
> >
> > Object *obj = object_new(TYPE_BLAH)
> >
> > the caller can be pretty confident that they will successfully create
> > an object instance of TYPE_BLAH. They know exactly what type has been
> > requested, so it passing an abstract type for example, it is a clear
> > programmer error that they'll get an assertion failure.
> >
> > Conversely with code like
> >
> > void somefunc(const char *typename) {
> > Object * obj = object_new(typename)
> > ...
> > }
> >
> > all bets are off, because the call of object_new() knows nothing
> > about what 'typename' resolves to.
>
> We know nothing *locally*.
>
> Commonly, a non-local argument can demonstrate safety. Only when the
> type name comes from the user, we truly know nothing.
...except for the failures introduced by modules not being installed,
then all bets are off for all types unless you happen to recall
which have been modularized so far.
> > It could easily be an abstract
> > type.
>
> It could also be no type at all.
>
> > As a result, many code paths have added a manual check ahead
> > of time
> >
> > if (object_class_is_abstract(typename)) {
> > error_setg(errp, ....)
> > }
>
> Actually, object_class_is_abstract() takes an ObjectClass, not a type
> name string.
>
> The actual guards we use are variations of
>
> klass = object_class_by_name(typename);
> if (!klass) {
> error_setg(errp, "invalid object type: %s", typename);
> return NULL;
> }
>
> if (object_class_is_abstract(klass)) {
> error_setg(errp, "object type '%s' is abstract", typename);
> return NULL;
> }
>
> which covers "no type at all", too.
>
> Sometimes, we use module_object_class_by_name() instead, which I believe
> additionally loads the module providing the type, if any. Which of the
> two should be used where is a mystery to me, and I suspect we're getting
> it wrong in places. But this is turning into a digression. To
> hopefully maintain focus, I'm pretending modules don't exist until later
> in this message.
Yeah, I'm not a fan of having the separate module_object_class_by_name,
because it requires us to remember whether something has been modularized
or not.
> Sometimes, we throw in an object_class_dynamic_cast(klass, T) to check
> @typename resolves to a subtype of some T.
>
> > ...except for where we forget to do this, such as qdev_new().
>
> We did not forget it there! It's by design a thin wrapper around
> object_new(), with preconditions just like object_new().
Yes, I think what I meant to write here, was "...except for where
we forgot todo this in *callers* of qdev_new that take user input"
> > Overall 'object_new' is a bad design because it is inherantly
> > unsafe to call with unvalidated typenames.
>
> To be fair, object_new() was not designed for use with user-provided
> type names. When it chokes on type names not provided by the user, it's
> clearly a programming error, and assert() is a perfectly fine way to
> catch programming errors. Same for qdev_new().
>
> However, we do in fact use these functions with user-provided type
> names, if rarely. When we do, we need to validate the type name before
> we pass it to them.
>
> Trouble is the validation code is a bit involved, and reimplementing it
> everywhere it's needed is asking for bugs.
> Creating and using more interfaces that are more convenient for this
> purpose would avoid that.
Yep, I don't have confidence in an API that will assert if the caller
forgot to validate the pre-conditions that can be triggered by user
input (or potentially other unexpected scenarios like something being
switched over to a module).
> > This problem is made worse by the proposal to introduce the idea
> > of 'singleton' classes[1].
> >
> > Thus, this series suggests a way to improve safety at build
> > time. The core idea is to allow 'object_new' to continue to be
> > used *if-and-only-if* given a static, const string, because that
> > scenario indicates the caller is aware of what type they are
> > creating at build time.
> >
> > A new 'object_new_dynamic' method is proposed for cases where
> > the typename is dynamically chosen at runtime. This method has
> > an "Error **errp" parameter, which can report when an abstract
> > type is created, leaving the assert()s only for scenarios which
> > are unambiguous programmer errors.
> >
> > With a little macro magic, we guarantee a compile error is
> > generated if 'object_new' is called with a dynamic type, forcing
> > all potentially unsafe code over to object_new_dynamic.
>
> Three cases:
>
> 1. Type name is literal string. No change. This is the most common
> case.
>
> 2. It's not.
>
> 2a. Type name is user-provided. This is rare. We replace
>
> if (... guard ...) {
> ... return failure ...
> }
> obj = object_new(...);
>
> by
>
> obj = object_new_dynamic(..., errp);
> if (!obj) {
> ... return failure ...
> }
>
> This is an improvement.
>
> 2b. It's not. We should replace
>
> obj = object_new(...);
>
> by
>
> obj = object_new_dynamic(..., &error_abort);
>
> Exact same behavior, just wordier, to placate the compiler.
> Tolerable as long as it's relatively rare.
>
> But I'm not sure it's worthwhile. All it really does is helping
> some towards not getting case 2a wrong. But 2a is rare.
Yes, 2a is fairly rare, but this is amplified by the consequences
of getting it wrong, which are an assert killing your running VM.
My goal was to make it much harder to screw up and trigger an
assert, even if that makes some valid uses more verbose.
> > This is more tractable than adding 'Error **errp' to 'object_new'
> > as only a handful of places use a dynamic type name.
>
> True!
>
> Alright, enter modules.
>
> Modules break a fundamental design assumption: object_new() on a
> compiled-in type name is safe, i.e. the failure modes are all
> programming errors.
>
> Modules add new failure modes that are *not* programming errors:
>
> * The module providing the type was not deployed correctly.
>
> * It was, but the host system lacks the resources to load it.
Hmm, yes, I hadn't considered the 2nd problem. That's more
unpleasant, as libvirt may well have queried QEMU earlier to
detect the missing module, and assume all is safe if it is
present.
>
> Before modules, object_new(T) was safe unless T was user-provided.
> Which implies it's safe when T is a literal string.
>
> Since modules, object_new(T) is safe unless T is user-provided or the
> type named by it is compiled as module. This does *not* imply it's safe
> when T is a literal string.
Agreed.
> When looking at a use of object_new(), whether the argument names a type
> that could be compiled as module cannot be known locally. Therefore, we
> cannot know locally whether we need to handle failure, either with a
> suitable guard or by switching to a new function like
> object_new_dynamic(). This is bad.
>
> Breaking fundamental design assumptions tends to have ugly and expensive
> consequences. Consequences like having to rework every single call of
> object_new() & friends.
>
> Can we reduce the damage? Maybe. What if we create a
> module_object_new() that takes an Error **, and make object_new() crash
> & burn when the @typename argument resolves to a type provided by a
> module?
I'm doubtful about a design where maintainers have to choose the
right API, based on mental knowledge of what is a module or not.
The place where object_new is called is typically distinct from
the module impl, so the knowledge is separated. This opens the
door to forgetting to change code from object_new to module_object_new.
This is what motivated my attempt to try to force compile time errors
scenarios which had a high chance of being user specified types.
I don't have a good answer for how to extend compile time validation
to cover non-user specified types that might be modules, without
changnig 'object_new' itself to add "Error **errp" and convert as
many callers as possible to propagate errors. That's a huge pile
of tedious work and in many cases would deteriorate to &error_abort
since some key common use scenarios lack a "Error *errp" to propagate
into.
> Maybe module_object_new() and object_new_dynamic() could be fused into a
> single function with a better name.
>
> > With this series, my objections to Peter Xu's singleton series[1]
> > would be largely nullified.
> >
> > [1] https://lists.nongnu.org/archive/html/qemu-devel/2024-10/msg05524.html
>
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 :|
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v3 0/9] Require error handling for dynamically created objects
2024-12-05 16:04 ` Daniel P. Berrangé
@ 2024-12-06 8:25 ` Markus Armbruster
2024-12-06 10:57 ` Daniel P. Berrangé
0 siblings, 1 reply; 22+ messages in thread
From: Markus Armbruster @ 2024-12-06 8:25 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel, Paolo Bonzini, Peter Xu
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Wed, Dec 04, 2024 at 12:07:58PM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>> > NB, this series is targetting 10.0, NOT for 9.2 freeze.
>> >
>> > With code like
>> >
>> > Object *obj = object_new(TYPE_BLAH)
>> >
>> > the caller can be pretty confident that they will successfully create
>> > an object instance of TYPE_BLAH. They know exactly what type has been
>> > requested, so it passing an abstract type for example, it is a clear
>> > programmer error that they'll get an assertion failure.
>> >
>> > Conversely with code like
>> >
>> > void somefunc(const char *typename) {
>> > Object * obj = object_new(typename)
>> > ...
>> > }
>> >
>> > all bets are off, because the call of object_new() knows nothing
>> > about what 'typename' resolves to.
>>
>> We know nothing *locally*.
>>
>> Commonly, a non-local argument can demonstrate safety. Only when the
>> type name comes from the user, we truly know nothing.
>
> ...except for the failures introduced by modules not being installed,
> then all bets are off for all types unless you happen to recall
> which have been modularized so far.
True. I was pretending modules don't exist until later in the message.
>> > It could easily be an abstract
>> > type.
>>
>> It could also be no type at all.
>>
>> > As a result, many code paths have added a manual check ahead
>> > of time
>> >
>> > if (object_class_is_abstract(typename)) {
>> > error_setg(errp, ....)
>> > }
>>
>> Actually, object_class_is_abstract() takes an ObjectClass, not a type
>> name string.
>>
>> The actual guards we use are variations of
>>
>> klass = object_class_by_name(typename);
>> if (!klass) {
>> error_setg(errp, "invalid object type: %s", typename);
>> return NULL;
>> }
>>
>> if (object_class_is_abstract(klass)) {
>> error_setg(errp, "object type '%s' is abstract", typename);
>> return NULL;
>> }
>>
>> which covers "no type at all", too.
>>
>> Sometimes, we use module_object_class_by_name() instead, which I believe
>> additionally loads the module providing the type, if any. Which of the
>> two should be used where is a mystery to me, and I suspect we're getting
>> it wrong in places. But this is turning into a digression. To
>> hopefully maintain focus, I'm pretending modules don't exist until later
>> in this message.
>
> Yeah, I'm not a fan of having the separate module_object_class_by_name,
> because it requires us to remember whether something has been modularized
> or not.
>
>> Sometimes, we throw in an object_class_dynamic_cast(klass, T) to check
>> @typename resolves to a subtype of some T.
>>
>> > ...except for where we forget to do this, such as qdev_new().
>>
>> We did not forget it there! It's by design a thin wrapper around
>> object_new(), with preconditions just like object_new().
>
> Yes, I think what I meant to write here, was "...except for where
> we forgot todo this in *callers* of qdev_new that take user input"
Correct.
>> > Overall 'object_new' is a bad design because it is inherantly
>> > unsafe to call with unvalidated typenames.
>>
>> To be fair, object_new() was not designed for use with user-provided
>> type names. When it chokes on type names not provided by the user, it's
>> clearly a programming error, and assert() is a perfectly fine way to
>> catch programming errors. Same for qdev_new().
>>
>> However, we do in fact use these functions with user-provided type
>> names, if rarely. When we do, we need to validate the type name before
>> we pass it to them.
>>
>> Trouble is the validation code is a bit involved, and reimplementing it
>> everywhere it's needed is asking for bugs.
>>
>> Creating and using more interfaces that are more convenient for this
>> purpose would avoid that.
>
> Yep, I don't have confidence in an API that will assert if the caller
> forgot to validate the pre-conditions that can be triggered by user
> input (or potentially other unexpected scenarios like something being
> switched over to a module).
Modules broke object_new(), but I'd rather not call object_new()'s
design bad for not accomodating a feature tacked on half-baked almost a
decade later. But let's discuss modules further down.
Asserting preconditions isn't the problem; this is how preconditions
*should* be checked. The problem is error-prone preconditions.
Using string type names is in theory error-prone: the compiler cannot
check the type name is valid. It could be invalid because of a typo, or
because it names a type that's not linked into this binary.
The compiler could check with an enumeration, but then the header
defining needed to be included basically everywhere QOM is used, and
changed all the time.
So QOM went with strings. I can't remember "invalid type name" bugs
surviving even basic testing in more than a decade of QOM use.
Except for *user-supplied* type names. These need to be validated, we
failed to factor out common validation code, and ended up with bugs in
some of the copies.
>> > This problem is made worse by the proposal to introduce the idea
>> > of 'singleton' classes[1].
>> >
>> > Thus, this series suggests a way to improve safety at build
>> > time. The core idea is to allow 'object_new' to continue to be
>> > used *if-and-only-if* given a static, const string, because that
>> > scenario indicates the caller is aware of what type they are
>> > creating at build time.
>> >
>> > A new 'object_new_dynamic' method is proposed for cases where
>> > the typename is dynamically chosen at runtime. This method has
>> > an "Error **errp" parameter, which can report when an abstract
>> > type is created, leaving the assert()s only for scenarios which
>> > are unambiguous programmer errors.
>> >
>> > With a little macro magic, we guarantee a compile error is
>> > generated if 'object_new' is called with a dynamic type, forcing
>> > all potentially unsafe code over to object_new_dynamic.
>>
>> Three cases:
>>
>> 1. Type name is literal string. No change. This is the most common
>> case.
>>
>> 2. It's not.
>>
>> 2a. Type name is user-provided. This is rare. We replace
>>
>> if (... guard ...) {
>> ... return failure ...
>> }
>> obj = object_new(...);
>>
>> by
>>
>> obj = object_new_dynamic(..., errp);
>> if (!obj) {
>> ... return failure ...
>> }
>>
>> This is an improvement.
>>
>> 2b. It's not. We should replace
>>
>> obj = object_new(...);
>>
>> by
>>
>> obj = object_new_dynamic(..., &error_abort);
>>
>> Exact same behavior, just wordier, to placate the compiler.
>> Tolerable as long as it's relatively rare.
>>
>> But I'm not sure it's worthwhile. All it really does is helping
>> some towards not getting case 2a wrong. But 2a is rare.
>
> Yes, 2a is fairly rare, but this is amplified by the consequences
> of getting it wrong, which are an assert killing your running VM.
> My goal was to make it much harder to screw up and trigger an
> assert, even if that makes some valid uses more verbose.
Has this been a problem in practice? We have thirteen years of
experience...
>> > This is more tractable than adding 'Error **errp' to 'object_new'
>> > as only a handful of places use a dynamic type name.
>>
>> True!
>>
>> Alright, enter modules.
>>
>> Modules break a fundamental design assumption: object_new() on a
>> compiled-in type name is safe, i.e. the failure modes are all
>> programming errors.
>>
>> Modules add new failure modes that are *not* programming errors:
>>
>> * The module providing the type was not deployed correctly.
>>
>> * It was, but the host system lacks the resources to load it.
>
> Hmm, yes, I hadn't considered the 2nd problem. That's more
> unpleasant, as libvirt may well have queried QEMU earlier to
> detect the missing module, and assume all is safe if it is
> present.
The first problem could perhaps be hand-waved away: must deploy all
modules or else. But that partly defeats the purpose of modules, namely
keeping unwanted dependencies off the host.
The second problem cannot: assertion failure on otherwise survivable
resource shortage is unequivocally wrong.
For more on module trouble, see "Problem 3: Loadable modules" in my memo
"Dynamic & heterogeneous machines, initial configuration: problems"[*]
>> Before modules, object_new(T) was safe unless T was user-provided.
>> Which implies it's safe when T is a literal string.
>>
>> Since modules, object_new(T) is safe unless T is user-provided or the
>> type named by it is compiled as module. This does *not* imply it's safe
>> when T is a literal string.
>
> Agreed.
>
>> When looking at a use of object_new(), whether the argument names a type
>> that could be compiled as module cannot be known locally. Therefore, we
>> cannot know locally whether we need to handle failure, either with a
>> suitable guard or by switching to a new function like
>> object_new_dynamic(). This is bad.
>>
>> Breaking fundamental design assumptions tends to have ugly and expensive
>> consequences. Consequences like having to rework every single call of
>> object_new() & friends.
>>
>> Can we reduce the damage? Maybe. What if we create a
>> module_object_new() that takes an Error **, and make object_new() crash
>> & burn when the @typename argument resolves to a type provided by a
>> module?
>
> I'm doubtful about a design where maintainers have to choose the
> right API, based on mental knowledge of what is a module or not.
> The place where object_new is called is typically distinct from
> the module impl, so the knowledge is separated. This opens the
> door to forgetting to change code from object_new to module_object_new.
> This is what motivated my attempt to try to force compile time errors
> scenarios which had a high chance of being user specified types.
We'd have to rely on the test suite here, as we've done for "type name
is valid".
> I don't have a good answer for how to extend compile time validation
> to cover non-user specified types that might be modules, without
> changnig 'object_new' itself to add "Error **errp" and convert as
> many callers as possible to propagate errors. That's a huge pile
> of tedious work and in many cases would deteriorate to &error_abort
> since some key common use scenarios lack a "Error *errp" to propagate
> into.
I can offer two ideas.
I'll start with devices for reasons that will become apparent in a
minute.
The first idea is straighforward in conception: since the problem is
modules breaking existing design assumptions, unbreak them.
Device creation cannot fail, only realize can. Could we delay the
problematic failure modes introduced by modules from creation to
realize?
When creating the real thing fails, create a dummy instead. Of course,
the dummy needs to be sufficiently functional to provide for the things
we do with devices before realize, such as introspection.
Note that we already link information on modules into the binary, so
that the binary knows which modules provide a certain object. To enable
sufficiently functional dummies, we'd have to link more.
The difficulty is "the things we do with devices before realize": do we
even know?
The other difficulty is that objects don't have realize. User-creatable
objects have complete, which is kind of similar. See also "Problem 5:
QOM lacks a clear life cycle" in my memo "Dynamic & heterogeneous
machines, initial configuration: problems"[*].
The second idea is a variation of your idea to provide two interfaces
for object creation, where using the wrong one won't compile: a common
one that cannot fail, i.e. object_new(), and an uncommon one that can.
Let's call that one object_try_new() for now.
Your proposed "string literal" as a useful approximation of "cannot
fail". Modules defeat that.
What if we switch from strings to something more expressive?
Step one: replace string type names by symbols
Change
#define TYPE_FOO "foo"
Object *object_new(const char *typename);
to something like
extern const TypeInfoSafe foo_info;
#define TYPE_FOO &foo_info
Object *object_new(const TypeInfoSafe *type_info);
Step two: different symbols for safe and unsafe types
extern const TypeInfoUnsafe bar_info;
#define TYPE_BAR &bar_info
Object *object_try_new(const TypeInfoUnsafe *type_info);
Now you cannot pass bar_info to object_new().
For a module-enabled TYPE_BAR, we already have something like
module_obj(TYPE_BAR)
Make macro module_obj() require its argument to be TypeInfoUnsafe.
Voilà, the compiler enforces use of object_try_new() for objects
provided by loadable modules.
There will be some fallout around computed type names such as
ACCEL_OPS_NAME(). Fairly rare, I think.
More fallout around passing TYPE_ macros to functions that accept both
safe and unsafe types. How common is that?
Worth exploring?
>> Maybe module_object_new() and object_new_dynamic() could be fused into a
>> single function with a better name.
>>
>> > With this series, my objections to Peter Xu's singleton series[1]
>> > would be largely nullified.
>> >
>> > [1] https://lists.nongnu.org/archive/html/qemu-devel/2024-10/msg05524.html
>>
>
> With regards,
> Daniel
[*] Message-ID: <87o7d1i7ky.fsf@pond.sub.org>
https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v3 0/9] Require error handling for dynamically created objects
2024-12-06 8:25 ` Markus Armbruster
@ 2024-12-06 10:57 ` Daniel P. Berrangé
2024-12-07 7:37 ` Markus Armbruster
0 siblings, 1 reply; 22+ messages in thread
From: Daniel P. Berrangé @ 2024-12-06 10:57 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Paolo Bonzini, Peter Xu
On Fri, Dec 06, 2024 at 09:25:54AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > On Wed, Dec 04, 2024 at 12:07:58PM +0100, Markus Armbruster wrote:
> >> To be fair, object_new() was not designed for use with user-provided
> >> type names. When it chokes on type names not provided by the user, it's
> >> clearly a programming error, and assert() is a perfectly fine way to
> >> catch programming errors. Same for qdev_new().
> >>
> >> However, we do in fact use these functions with user-provided type
> >> names, if rarely. When we do, we need to validate the type name before
> >> we pass it to them.
> >>
> >> Trouble is the validation code is a bit involved, and reimplementing it
> >> everywhere it's needed is asking for bugs.
> >>
> >> Creating and using more interfaces that are more convenient for this
> >> purpose would avoid that.
> >
> > Yep, I don't have confidence in an API that will assert if the caller
> > forgot to validate the pre-conditions that can be triggered by user
> > input (or potentially other unexpected scenarios like something being
> > switched over to a module).
>
> Modules broke object_new(), but I'd rather not call object_new()'s
> design bad for not accomodating a feature tacked on half-baked almost a
> decade later. But let's discuss modules further down.
>
> Asserting preconditions isn't the problem; this is how preconditions
> *should* be checked. The problem is error-prone preconditions.
Yep, pre-conditions need to be something developers can be reasonably
expected to accurately comply with.
> Using string type names is in theory error-prone: the compiler cannot
> check the type name is valid. It could be invalid because of a typo, or
> because it names a type that's not linked into this binary.
> The compiler could check with an enumeration, but then the header
> defining needed to be included basically everywhere QOM is used, and
> changed all the time.
>
> So QOM went with strings. I can't remember "invalid type name" bugs
> surviving even basic testing in more than a decade of QOM use.
Yep, at least for static object creation using since we're using the
pattern "object_new(TYPE_BLAH)" - even if TYPE_BLAH contains a typo,
it'll be the same typo passed in the .name = TYPE_BLAH of TypeInfo,
so all will work fine if following normal code patterns.
> Except for *user-supplied* type names. These need to be validated, we
> failed to factor out common validation code, and ended up with bugs in
> some of the copies.
Yep
> >> Three cases:
> >>
> >> 1. Type name is literal string. No change. This is the most common
> >> case.
> >>
> >> 2. It's not.
> >>
> >> 2a. Type name is user-provided. This is rare. We replace
> >>
> >> if (... guard ...) {
> >> ... return failure ...
> >> }
> >> obj = object_new(...);
> >>
> >> by
> >>
> >> obj = object_new_dynamic(..., errp);
> >> if (!obj) {
> >> ... return failure ...
> >> }
> >>
> >> This is an improvement.
> >>
> >> 2b. It's not. We should replace
> >>
> >> obj = object_new(...);
> >>
> >> by
> >>
> >> obj = object_new_dynamic(..., &error_abort);
> >>
> >> Exact same behavior, just wordier, to placate the compiler.
> >> Tolerable as long as it's relatively rare.
> >>
> >> But I'm not sure it's worthwhile. All it really does is helping
> >> some towards not getting case 2a wrong. But 2a is rare.
> >
> > Yes, 2a is fairly rare, but this is amplified by the consequences
> > of getting it wrong, which are an assert killing your running VM.
> > My goal was to make it much harder to screw up and trigger an
> > assert, even if that makes some valid uses more verbose.
>
> Has this been a problem in practice? We have thirteen years of
> experience...
No, but this series came out of Peter's proposal to introduce the
idea of Singleton classes, which would cause object_new to assert
in fun new scenarios. Effectively adding a new pre-condition and
would thus require all places which pass a dynamic type name to
object_new(), to be updated with a "if singleton..." check. I
wasn't happy with the idea of adding that precondition without a
way to enforce that we've not missed checks somewhere in the code.
Of course this pre-condition applies to static object_new calls
too, but those are less risky as the developer (probably) has the
mental context that the static object_new call is for a singleton.
> > I don't have a good answer for how to extend compile time validation
> > to cover non-user specified types that might be modules, without
> > changnig 'object_new' itself to add "Error **errp" and convert as
> > many callers as possible to propagate errors. That's a huge pile
> > of tedious work and in many cases would deteriorate to &error_abort
> > since some key common use scenarios lack a "Error *errp" to propagate
> > into.
>
> I can offer two ideas.
>
> I'll start with devices for reasons that will become apparent in a
> minute.
>
> The first idea is straighforward in conception: since the problem is
> modules breaking existing design assumptions, unbreak them.
>
> Device creation cannot fail, only realize can. Could we delay the
> problematic failure modes introduced by modules from creation to
> realize?
>
> When creating the real thing fails, create a dummy instead. Of course,
> the dummy needs to be sufficiently functional to provide for the things
> we do with devices before realize, such as introspection.
>
> Note that we already link information on modules into the binary, so
> that the binary knows which modules provide a certain object. To enable
> sufficiently functional dummies, we'd have to link more.
>
> The difficulty is "the things we do with devices before realize": do we
> even know?
Yeah, the idea of a dummy stub until realize is called fills me with
worry. It feels like something where it would be really easy to make
a mistake and have code that crashes interacting with an unrealized
object that doesn't have the struct fields you expect it to have, or
has the struct fields, but not initialized since no 'init' method
was run.
A slight refinement of your idea would be to break anything modular
into 2 distinct objects classes. MyDeviceFacade and MyDeviceImpl.
Creators of the device always call object_new(TYPE_MY_DEVICE_FACADE),
and the realize() method would load the module and make thje call
to object_new(TYPE_MY_DEVICE_IMPL).
Making something currently built-in, into a module, would involve
a bunch of tedious refactoring work, so I don't much like the
thought of choosing this as a design approach.
> The other difficulty is that objects don't have realize. User-creatable
> objects have complete, which is kind of similar. See also "Problem 5:
> QOM lacks a clear life cycle" in my memo "Dynamic & heterogeneous
> machines, initial configuration: problems"[*].
It would be nice to have a unified model between object and devices
for the complete/realize approach, but that's a slight tangent.
> The second idea is a variation of your idea to provide two interfaces
> for object creation, where using the wrong one won't compile: a common
> one that cannot fail, i.e. object_new(), and an uncommon one that can.
> Let's call that one object_try_new() for now.
>
> Your proposed "string literal" as a useful approximation of "cannot
> fail". Modules defeat that.
>
> What if we switch from strings to something more expressive?
>
> Step one: replace string type names by symbols
>
> Change
>
> #define TYPE_FOO "foo"
>
> Object *object_new(const char *typename);
>
> to something like
>
> extern const TypeInfoSafe foo_info;
> #define TYPE_FOO &foo_info
>
> Object *object_new(const TypeInfoSafe *type_info);
>
> Step two: different symbols for safe and unsafe types
>
> extern const TypeInfoUnsafe bar_info;
> #define TYPE_BAR &bar_info
>
> Object *object_try_new(const TypeInfoUnsafe *type_info);
>
> Now you cannot pass bar_info to object_new().
>
> For a module-enabled TYPE_BAR, we already have something like
>
> module_obj(TYPE_BAR)
>
> Make macro module_obj() require its argument to be TypeInfoUnsafe.
>
> Voilà, the compiler enforces use of object_try_new() for objects
> provided by loadable modules.
>
> There will be some fallout around computed type names such as
> ACCEL_OPS_NAME(). Fairly rare, I think.
>
> More fallout around passing TYPE_ macros to functions that accept both
> safe and unsafe types. How common is that?
Perhaps more common than we care to admit. eg most block device drivers
are safe, except for a few we modularized which are unsafe. Most ui
frontends would be safe, except for a few we modularized. This pattern
of "except for a few we modularized" has been repeated all over, and
conceptually that's not a bad thing, as we wanted to make it easy to
modularize things incrementally.
Looking at our current /usr/bin/qemu-system-XXX binaries, they range in
size from 6 MB to 30 MB, stripped, ignoring linked libraries. Considering
work on the qemu-system-any binary that is intended to unify all targets,
I wouldn't be surprised if it came out at over 100 MB with all devices
from all targets included.
Is qemu-system-any pushing us to a place where our approach to modules
is in fact wrong ?
Modularizing piecemeal let us cull the big offenders that pulled in
huge external libraries.
People still complain QEMU is "too big" and binaries linked to too
many legacy devices.
With my distro hat on, if we had 'qemu-system-any' would I really
want to have it as monolithic binary ?
I think I would want to have loadable TCG backends for each target,
and I would want all the devices for each target to be loadable too.
eg, so I could have a 'qemu-system-any' RPM with just the core, and
'qemu-system-modules-arm', 'qemu-system-modules-x86_64', etc, or
even more fine grained than that.
IOW, everything is a module by default. Not necccessarily
1 object == 1 module, more "N objects == 1 module", but certainly
with very few objects built-in.
In such a world, IMHO, it doesn't make sense to have TypeInfoSafe
and TypeInfoUnsafe, with different object_new/object_try_new
methods. I think we would have to accept that object_new must
get an "Error **errp", and possibly even the 'init" method too.
It would force us to make sure we can propagage into errp in
all the key places we can't do so today wrt object lifecycles.
Overall I've talked myself into believing my series here is not
worthwhile, as it doesn't solve a big enough problem, and it
needs somethign more ambituous.
> >> Maybe module_object_new() and object_new_dynamic() could be fused into a
> >> single function with a better name.
> >>
> >> > With this series, my objections to Peter Xu's singleton series[1]
> >> > would be largely nullified.
> >> >
> >> > [1] https://lists.nongnu.org/archive/html/qemu-devel/2024-10/msg05524.html
>
> [*] Message-ID: <87o7d1i7ky.fsf@pond.sub.org>
> https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/
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 :|
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v3 0/9] Require error handling for dynamically created objects
2024-12-06 10:57 ` Daniel P. Berrangé
@ 2024-12-07 7:37 ` Markus Armbruster
0 siblings, 0 replies; 22+ messages in thread
From: Markus Armbruster @ 2024-12-07 7:37 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-devel, Paolo Bonzini, Peter Xu, Philippe Mathieu-Daudé
Cc: Phil, because we're now discusing qemu-system-any.
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Fri, Dec 06, 2024 at 09:25:54AM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>> > On Wed, Dec 04, 2024 at 12:07:58PM +0100, Markus Armbruster wrote:
>> >> To be fair, object_new() was not designed for use with user-provided
>> >> type names. When it chokes on type names not provided by the user, it's
>> >> clearly a programming error, and assert() is a perfectly fine way to
>> >> catch programming errors. Same for qdev_new().
>> >>
>> >> However, we do in fact use these functions with user-provided type
>> >> names, if rarely. When we do, we need to validate the type name before
>> >> we pass it to them.
>> >>
>> >> Trouble is the validation code is a bit involved, and reimplementing it
>> >> everywhere it's needed is asking for bugs.
>> >>
>> >> Creating and using more interfaces that are more convenient for this
>> >> purpose would avoid that.
>> >
>> > Yep, I don't have confidence in an API that will assert if the caller
>> > forgot to validate the pre-conditions that can be triggered by user
>> > input (or potentially other unexpected scenarios like something being
>> > switched over to a module).
>>
>> Modules broke object_new(), but I'd rather not call object_new()'s
>> design bad for not accomodating a feature tacked on half-baked almost a
>> decade later. But let's discuss modules further down.
>>
>> Asserting preconditions isn't the problem; this is how preconditions
>> *should* be checked. The problem is error-prone preconditions.
>
> Yep, pre-conditions need to be something developers can be reasonably
> expected to accurately comply with.
>
>> Using string type names is in theory error-prone: the compiler cannot
>> check the type name is valid. It could be invalid because of a typo, or
>> because it names a type that's not linked into this binary.
>
>
>> The compiler could check with an enumeration, but then the header
>> defining needed to be included basically everywhere QOM is used, and
>> changed all the time.
>>
>> So QOM went with strings. I can't remember "invalid type name" bugs
>> surviving even basic testing in more than a decade of QOM use.
>
> Yep, at least for static object creation using since we're using the
> pattern "object_new(TYPE_BLAH)" - even if TYPE_BLAH contains a typo,
> it'll be the same typo passed in the .name = TYPE_BLAH of TypeInfo,
> so all will work fine if following normal code patterns.
There's no shortage of qdev_new("mumble"), and even there typos haven't
been a problem.
>> Except for *user-supplied* type names. These need to be validated, we
>> failed to factor out common validation code, and ended up with bugs in
>> some of the copies.
>
> Yep
>
>
>> >> Three cases:
>> >>
>> >> 1. Type name is literal string. No change. This is the most common
>> >> case.
>> >>
>> >> 2. It's not.
>> >>
>> >> 2a. Type name is user-provided. This is rare. We replace
>> >>
>> >> if (... guard ...) {
>> >> ... return failure ...
>> >> }
>> >> obj = object_new(...);
>> >>
>> >> by
>> >>
>> >> obj = object_new_dynamic(..., errp);
>> >> if (!obj) {
>> >> ... return failure ...
>> >> }
>> >>
>> >> This is an improvement.
>> >>
>> >> 2b. It's not. We should replace
>> >>
>> >> obj = object_new(...);
>> >>
>> >> by
>> >>
>> >> obj = object_new_dynamic(..., &error_abort);
>> >>
>> >> Exact same behavior, just wordier, to placate the compiler.
>> >> Tolerable as long as it's relatively rare.
>> >>
>> >> But I'm not sure it's worthwhile. All it really does is helping
>> >> some towards not getting case 2a wrong. But 2a is rare.
>> >
>> > Yes, 2a is fairly rare, but this is amplified by the consequences
>> > of getting it wrong, which are an assert killing your running VM.
>> > My goal was to make it much harder to screw up and trigger an
>> > assert, even if that makes some valid uses more verbose.
>>
>> Has this been a problem in practice? We have thirteen years of
>> experience...
>
> No, but this series came out of Peter's proposal to introduce the
> idea of Singleton classes, which would cause object_new to assert
> in fun new scenarios. Effectively adding a new pre-condition and
> would thus require all places which pass a dynamic type name to
> object_new(), to be updated with a "if singleton..." check. I
> wasn't happy with the idea of adding that precondition without a
> way to enforce that we've not missed checks somewhere in the code.
>
> Of course this pre-condition applies to static object_new calls
> too, but those are less risky as the developer (probably) has the
> mental context that the static object_new call is for a singleton.
>
>> > I don't have a good answer for how to extend compile time validation
>> > to cover non-user specified types that might be modules, without
>> > changnig 'object_new' itself to add "Error **errp" and convert as
>> > many callers as possible to propagate errors. That's a huge pile
>> > of tedious work and in many cases would deteriorate to &error_abort
>> > since some key common use scenarios lack a "Error *errp" to propagate
>> > into.
>>
>> I can offer two ideas.
>>
>> I'll start with devices for reasons that will become apparent in a
>> minute.
>>
>> The first idea is straighforward in conception: since the problem is
>> modules breaking existing design assumptions, unbreak them.
>>
>> Device creation cannot fail, only realize can. Could we delay the
>> problematic failure modes introduced by modules from creation to
>> realize?
>>
>> When creating the real thing fails, create a dummy instead. Of course,
>> the dummy needs to be sufficiently functional to provide for the things
>> we do with devices before realize, such as introspection.
>>
>> Note that we already link information on modules into the binary, so
>> that the binary knows which modules provide a certain object. To enable
>> sufficiently functional dummies, we'd have to link more.
>>
>> The difficulty is "the things we do with devices before realize": do we
>> even know?
>
> Yeah, the idea of a dummy stub until realize is called fills me with
> worry. It feels like something where it would be really easy to make
> a mistake and have code that crashes interacting with an unrealized
> object that doesn't have the struct fields you expect it to have, or
> has the struct fields, but not initialized since no 'init' method
> was run.
>
> A slight refinement of your idea would be to break anything modular
> into 2 distinct objects classes. MyDeviceFacade and MyDeviceImpl.
> Creators of the device always call object_new(TYPE_MY_DEVICE_FACADE),
> and the realize() method would load the module and make thje call
> to object_new(TYPE_MY_DEVICE_IMPL).
>
> Making something currently built-in, into a module, would involve
> a bunch of tedious refactoring work, so I don't much like the
> thought of choosing this as a design approach.
>
>> The other difficulty is that objects don't have realize. User-creatable
>> objects have complete, which is kind of similar. See also "Problem 5:
>> QOM lacks a clear life cycle" in my memo "Dynamic & heterogeneous
>> machines, initial configuration: problems"[*].
>
> It would be nice to have a unified model between object and devices
> for the complete/realize approach, but that's a slight tangent.
Yes, and yes.
>> The second idea is a variation of your idea to provide two interfaces
>> for object creation, where using the wrong one won't compile: a common
>> one that cannot fail, i.e. object_new(), and an uncommon one that can.
>> Let's call that one object_try_new() for now.
>>
>> Your proposed "string literal" as a useful approximation of "cannot
>> fail". Modules defeat that.
>>
>> What if we switch from strings to something more expressive?
>>
>> Step one: replace string type names by symbols
>>
>> Change
>>
>> #define TYPE_FOO "foo"
>>
>> Object *object_new(const char *typename);
>>
>> to something like
>>
>> extern const TypeInfoSafe foo_info;
>> #define TYPE_FOO &foo_info
>>
>> Object *object_new(const TypeInfoSafe *type_info);
>>
>> Step two: different symbols for safe and unsafe types
>>
>> extern const TypeInfoUnsafe bar_info;
>> #define TYPE_BAR &bar_info
>>
>> Object *object_try_new(const TypeInfoUnsafe *type_info);
>>
>> Now you cannot pass bar_info to object_new().
>>
>> For a module-enabled TYPE_BAR, we already have something like
>>
>> module_obj(TYPE_BAR)
>>
>> Make macro module_obj() require its argument to be TypeInfoUnsafe.
>>
>> Voilà, the compiler enforces use of object_try_new() for objects
>> provided by loadable modules.
>>
>> There will be some fallout around computed type names such as
>> ACCEL_OPS_NAME(). Fairly rare, I think.
>>
>> More fallout around passing TYPE_ macros to functions that accept both
>> safe and unsafe types. How common is that?
>
> Perhaps more common than we care to admit. eg most block device drivers
> are safe, except for a few we modularized which are unsafe. Most ui
> frontends would be safe, except for a few we modularized. This pattern
> of "except for a few we modularized" has been repeated all over, and
> conceptually that's not a bad thing, as we wanted to make it easy to
> modularize things incrementally.
It's only a problem if we have functions other than object_new() &
wrappers that now take type name strings. Since their string argument
can't become both TypeInfoSafe and TypeInfoUnsafe, we'd have to split
them into a safe and an unsafe variant just like object_new(), or splice
in a suitable conversion. Do such functions exist? Helpers within qom/
don't really count.
> Looking at our current /usr/bin/qemu-system-XXX binaries, they range in
> size from 6 MB to 30 MB, stripped, ignoring linked libraries. Considering
> work on the qemu-system-any binary that is intended to unify all targets,
> I wouldn't be surprised if it came out at over 100 MB with all devices
> from all targets included.
>
> Is qemu-system-any pushing us to a place where our approach to modules
> is in fact wrong ?
>
> Modularizing piecemeal let us cull the big offenders that pulled in
> huge external libraries.
>
> People still complain QEMU is "too big" and binaries linked to too
> many legacy devices.
>
> With my distro hat on, if we had 'qemu-system-any' would I really
> want to have it as monolithic binary ?
No, and I don't think that's Phil's plan.
> I think I would want to have loadable TCG backends for each target,
> and I would want all the devices for each target to be loadable too.
> eg, so I could have a 'qemu-system-any' RPM with just the core, and
> 'qemu-system-modules-arm', 'qemu-system-modules-x86_64', etc, or
> even more fine grained than that.
>
> IOW, everything is a module by default. Not necccessarily
> 1 object == 1 module, more "N objects == 1 module", but certainly
> with very few objects built-in.
I doubt one module per QOM type makes sense. That's a huuuuge amount
modules, a thicket of dependencies, and way too much dynamic loading and
linking.
My qemu-system-x86_64 links with ~200 shared objects according to ldd.
It sports ~800 QOM types according to qom-list-types.
Pulling in shared objects in the low hundreds is already a dubious idea.
Pushing their number into the thousands feels... unadvisable.
Quite a few QOM types live together with relatives in the same .c. But
even one module per .c instead of one module per QOM type will still
result in an excessive number of modules. Evidence: I count >1700 .c
under hw, and I suspect most of them would become modules.
How can we do better?
Clearly, each target requires a certain set of QOM types. Same for each
machine type. We could simply have one module per target plus one
module per machine type. A homogeneous guest would need one of each.
Since some types are used by more than one target / machine type, we'd
end up with them duplicated in different target / machine type modules.
To avoid that, we'd have to factor them out into common modules the
target / machine type modules can statically require.
Sounds like work, but it should produce a lot fewer modules.
> In such a world, IMHO, it doesn't make sense to have TypeInfoSafe
> and TypeInfoUnsafe, with different object_new/object_try_new
> methods. I think we would have to accept that object_new must
> get an "Error **errp", and possibly even the 'init" method too.
> It would force us to make sure we can propagage into errp in
> all the key places we can't do so today wrt object lifecycles.
With less fine-grained modularization, such as the one I sketched above,
many (most?) instances of object_new() remain safe, because they create
instances of types provided by the same module, or a statically required
module. Remember, shared objects don't load unless their dependencies
also load, which means a successfully loaded module will have all it
statically requires.
> Overall I've talked myself into believing my series here is not
> worthwhile, as it doesn't solve a big enough problem, and it
> needs somethign more ambituous.
Yes, but what exactly is not yet clear to me.
>> >> Maybe module_object_new() and object_new_dynamic() could be fused into a
>> >> single function with a better name.
>> >>
>> >> > With this series, my objections to Peter Xu's singleton series[1]
>> >> > would be largely nullified.
>> >> >
>> >> > [1] https://lists.nongnu.org/archive/html/qemu-devel/2024-10/msg05524.html
>>
>> [*] Message-ID: <87o7d1i7ky.fsf@pond.sub.org>
>> https://lore.kernel.org/qemu-devel/87o7d1i7ky.fsf@pond.sub.org/
>
> With regards,
> Daniel
^ permalink raw reply [flat|nested] 22+ messages in thread