[v2,0/8] Require error handling for dynamically created objects

Message

Daniel P. Berrangé Nov. 11, 2024, 3:55 p.m. UTC

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. It could easily be an abstract
type. As a result, many code paths have added a manual check ahead
of time

   if (object_class_is_abstract(typename)) {
      error_setg(errp, ....)
   }

...except for where we forget to do this, such as qdev_new().

Overall 'object_new' is a bad design because it is inherantly
unsafe to call with unvalidated typenames.

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.

This is more tractable than adding 'Error **errp' to 'object_new'
as only a handful of places use a dynamic type 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

Changed in v2:

 * Removed "RFC" tag
 * Converted code in all non-x86_64 targets
 * Converted qdev_new to same pattern as object_new
 * Ensured test suites work now

Daniel P. Berrangé (8):
  qom: refactor checking abstract property when creating instances
  qom: allow failure of object_new_with_class
  qom: introduce object_new_dynamic()
  convert code to object_new_dynamic() where appropriate
  qom: enforce use of static, const string with object_new()
  qom: introduce qdev_new_dynamic()
  convert code to qdev_new_dynamic() where appropriate
  hw: enforce use of static, const string with qdev_new()

 accel/accel-user.c                    |  4 +-
 chardev/char.c                        |  6 ++-
 hw/arm/aspeed.c                       |  6 +--
 hw/arm/exynos4210.c                   |  3 +-
 hw/arm/highbank.c                     |  2 +-
 hw/arm/integratorcp.c                 |  2 +-
 hw/arm/mps3r.c                        |  3 +-
 hw/arm/npcm7xx_boards.c               |  2 +-
 hw/arm/realview.c                     |  3 +-
 hw/arm/sbsa-ref.c                     |  7 +--
 hw/arm/versatilepb.c                  |  2 +-
 hw/arm/vexpress.c                     |  4 +-
 hw/arm/virt.c                         | 10 ++--
 hw/arm/xilinx_zynq.c                  |  3 +-
 hw/audio/soundhw.c                    |  2 +-
 hw/block/xen-block.c                  |  7 ++-
 hw/core/bus.c                         |  2 +-
 hw/core/cpu-common.c                  |  2 +-
 hw/core/qdev.c                        | 24 +++++++--
 hw/core/sysbus.c                      |  2 +-
 hw/i2c/core.c                         |  2 +-
 hw/i386/x86-common.c                  |  5 +-
 hw/i386/xen/xen-pvh.c                 |  2 +-
 hw/intc/xics.c                        |  5 +-
 hw/isa/isa-bus.c                      |  4 +-
 hw/mips/cps.c                         |  3 +-
 hw/pci-host/pnv_phb.c                 |  5 +-
 hw/pci/pci.c                          |  2 +-
 hw/ppc/e500.c                         |  2 +-
 hw/ppc/pnv.c                          |  6 +--
 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            | 10 +++-
 hw/scsi/scsi-bus.c                    |  5 +-
 hw/sparc/leon3.c                      |  2 +-
 hw/sparc/sun4m.c                      |  2 +-
 hw/sparc64/sparc64.c                  |  2 +-
 hw/ssi/ssi.c                          |  2 +-
 hw/vfio/common.c                      |  6 ++-
 hw/vfio/container.c                   |  7 ++-
 include/hw/qdev-core.h                | 78 ++++++++++++++++++++++++++-
 include/hw/usb.h                      |  4 +-
 include/qom/object.h                  | 48 +++++++++++++++--
 net/net.c                             |  7 +--
 qom/object.c                          | 44 ++++++++++-----
 qom/object_interfaces.c               |  7 ++-
 qom/qom-qmp-cmds.c                    | 16 +++---
 system/qdev-monitor.c                 |  5 +-
 system/vl.c                           |  6 ++-
 target/arm/arm-qmp-cmds.c             |  5 +-
 target/i386/cpu-apic.c                |  8 ++-
 target/i386/cpu-sysemu.c              | 11 ++--
 target/i386/cpu.c                     |  4 +-
 target/loongarch/loongarch-qmp-cmds.c |  5 +-
 target/mips/cpu.c                     |  2 +-
 target/riscv/riscv-qmp-cmds.c         |  5 +-
 target/s390x/cpu_models_sysemu.c      |  7 ++-
 target/xtensa/cpu.c                   |  2 +-
 tests/unit/check-qom-interface.c      |  3 +-
 tests/unit/test-smp-parse.c           | 20 +++----
 62 files changed, 353 insertions(+), 116 deletions(-)