Re: [PATCH for-11.2 v3 15/15] hw/qdev: Prevent devices from being realized more than once

[Philippe Mathieu-Daudé]

Hi Akihiko,

On 21/7/26 10:17, Akihiko Odaki wrote:

qdev currently permits reentrant realization of the same device. It also
permits another realization attempt after a device has been unrealized
or a previous attempt has failed. Either path can invoke
DeviceClass::realize() more than once. Supporting repeated realization
adds complexity to device implementations. It is untested and likely
broken.

Replace the bool DeviceState::realized field with the enum-valued
DeviceState::phase field. The enum has four values:

- initialized
- realizing
- realized
- retired

Excellent.

I have been working on something similar.

I'd start the first patch only including:

DEVICE_PHASE_UNREALIZED (false)
DEVICE_PHASE_REALIZED (true)

Then gradually rename DEVICE_PHASE_REALIZED -> DEVICE_PHASE_CREATED
and add the DEVICE_PHASE_REALIZING and DEVICE_PHASE_RETIRED phases,
so we can discuss them during the review process.

Realization can start only in the initialized phase. It moves the device
to the realizing phase before invoking callbacks, preventing another
realization attempt. Successful realization moves it to the realized
phase; failure after realization has started moves it to the retired
phase. Unrealization also moves a realized device to the retired phase.

So what is the difference between 'initialized' and 'retired'?

The QOM realized property is an internal lifecycle property, not for
end users. Replace it with the enum-valued phase property.

Signed-off-by: Akihiko Odaki <od...@rsg.ci.i.u-tokyo.ac.jp>
---
  qapi/common.json          |  19 ++++++++
  include/hw/core/qdev.h    |  12 ++---
  hw/core/qdev-clock.c      |   4 +-
  hw/core/qdev-properties.c |   4 +-
  hw/core/qdev.c            |  98 ++++++++++++++++++++++++++--------------
  hw/scsi/scsi-bus.c        |   4 +-
  qom/qom-qmp-cmds.c        |   2 +-
  system/qdev-monitor.c     |   5 ++-
  tests/unit/test-qdev.c    | 112 +++++++++++++++++++++++++++++++++++++++++++++-
  9 files changed, 212 insertions(+), 48 deletions(-)

diff --git a/qapi/common.json b/qapi/common.json
index af7e3d618a7c..88a308cbd172 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -7,6 +7,25 @@
  # *****************
  ##

+##

+# @DevicePhase:
+#
+# An enumeration of the device phases
+#
+# @initialized: the initial phase
+#
+# @realizing: the phase during realization
+#
+# @realized: the phase after realization
+#
+# @retired: the terminal phase entered when unrealization begins or
+#           realization fails after starting
+#
+# Since: 11.1
+##
+{ 'enum': 'DevicePhase',
+  'data': [ 'initialized', 'realizing', 'realized', 'retired' ] }
+

@@ -477,10 +477,10 @@ bool qdev_unplug_blocked(DeviceState *dev, Error **errp)
      return false;
  }

-static bool device_get_realized(Object *obj, Error **errp)

+static int device_get_phase(Object *obj, Error **errp)

DevicePhase

  {
      DeviceState *dev = DEVICE(obj);
-    return dev->realized;
+    return dev->phase;
  }

@@ -670,7 +702,6 @@ static void device_initfn(Object *obj)
      }

dev->instance_id_alias = -1;

-    dev->realized = false;

Can we keep the initialization explicit? This helps when navigating
the code base.

      dev->allow_unplug_during_migration = false;

QLIST_INIT(&dev->gpios);