Re: [PATCH v4 13/25] memory: Add Error** argument to .log_global_start() handler

[Cédric Le Goater]

On 3/15/24 12:18, Peter Xu wrote:

@@ -3009,13 +3045,16 @@ static void listener_add_address_space(MemoryListener 
*listener,
  {
      FlatView *view;
      FlatRange *fr;
+    Error *local_err = NULL;

if (listener->begin) {

          listener->begin(listener);
      }
      if (global_dirty_tracking) {
          if (listener->log_global_start) {
-            listener->log_global_start(listener);
+            if (!listener->log_global_start(listener, &local_err)) {
+                error_report_err(local_err);
+            }

IMHO we should assert here instead of error report.  We have this to guard
hot-plug during migration so I think the assert is justified:

qdev_device_add_from_qdict():

     if (!migration_is_idle()) {
         error_setg(errp, "device_add not allowed while migrating");
         return NULL;
     }

If it really happens it's a bug, as listener_add_address_space() will still
keep the rest things around even if the hook failed.  It'll start to be a
total mess..

It seems that adding a region listener while logging is active has been
supported from the beginning, commit 7664e80c8470 ("memory: add API for
observing  updates to the physical memory map"). Can it happen ? if not
we could simply remove the  log_global_start() call.

Thanks,

C.