On 2025/07/17 9:37, Arun Menon wrote:
This is an incremental step in converting vmstate loading
code to report error via Error objects instead of directly
printing it to console/monitor.
It is ensured that loadvm_process_enable_colo() must report an error
in errp, in case of failure.

Signed-off-by: Arun Menon <arme...@redhat.com>
---
  include/migration/colo.h |  2 +-
  migration/migration.c    | 12 ++++++------
  migration/ram.c          |  8 ++++----
  migration/ram.h          |  2 +-
  migration/savevm.c       | 25 ++++++++++++-------------
  5 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/include/migration/colo.h b/include/migration/colo.h
index 
43222ef5ae6adc3f7d8aa6a48bef79af33d09208..d4fe422e4d335d3bef4f860f56400fcd73287a0e
 100644
--- a/include/migration/colo.h
+++ b/include/migration/colo.h
@@ -25,7 +25,7 @@ void migrate_start_colo_process(MigrationState *s);
  bool migration_in_colo_state(void);
/* loadvm */
-int migration_incoming_enable_colo(void);
+int migration_incoming_enable_colo(Error **errp);
  void migration_incoming_disable_colo(void);
  bool migration_incoming_colo_enabled(void);
  bool migration_incoming_in_colo_state(void);
diff --git a/migration/migration.c b/migration/migration.c
index 
10c216d25dec01f206eacad2edd24d21f00e614c..326487882c8d41e2f89f99f69df0d9d4d42705e4
 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -623,22 +623,22 @@ void migration_incoming_disable_colo(void)
      migration_colo_enabled = false;
  }
-int migration_incoming_enable_colo(void)
+int migration_incoming_enable_colo(Error **errp)
  {
  #ifndef CONFIG_REPLICATION
-    error_report("ENABLE_COLO command come in migration stream, but the "
-                 "replication module is not built in");
+    error_setg(errp, "ENABLE_COLO command come in migration stream, but the "
+               "replication module is not built in");
      return -ENOTSUP;
  #endif
if (!migrate_colo()) {
-        error_report("ENABLE_COLO command come in migration stream, but x-colo 
"
-                     "capability is not set");
+        error_setg(errp, "ENABLE_COLO command come in migration stream"
+                   ", but x-colo capability is not set");
          return -EINVAL;
      }
if (ram_block_discard_disable(true)) {
-        error_report("COLO: cannot disable RAM discard");
+        error_setg(errp, "COLO: cannot disable RAM discard");
          return -EBUSY;
      }
      migration_colo_enabled = true;
diff --git a/migration/ram.c b/migration/ram.c
index 
8223183132dc0f558f45fbae3f4f832845730bd3..607c979cc15a3d321e5e3e380ac7613d80d86fc9
 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3576,7 +3576,7 @@ static void colo_init_ram_state(void)
   * memory of the secondary VM, it is need to hold the global lock
   * to call this helper.
   */
-int colo_init_ram_cache(void)
+int colo_init_ram_cache(Error **errp)
  {
      RAMBlock *block;
@@ -3585,9 +3585,9 @@ int colo_init_ram_cache(void)
              block->colo_cache = qemu_anon_ram_alloc(block->used_length,
                                                      NULL, false, false);
              if (!block->colo_cache) {
-                error_report("%s: Can't alloc memory for COLO cache of block 
%s,"
-                             "size 0x" RAM_ADDR_FMT, __func__, block->idstr,
-                             block->used_length);
+                error_setg(errp, "%s: Can't alloc memory for COLO cache of "
+                           "block %s, size 0x" RAM_ADDR_FMT, __func__,
+                           block->idstr, block->used_length);
                  RAMBLOCK_FOREACH_NOT_IGNORED(block) {
                      if (block->colo_cache) {
                          qemu_anon_ram_free(block->colo_cache, 
block->used_length);
diff --git a/migration/ram.h b/migration/ram.h
index 
275709a99187f9429ccb4111e05281ec268ba0db..24cd0bf585762cfa1e86834dc03c6baeea2f0627
 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -109,7 +109,7 @@ void ramblock_set_file_bmap_atomic(RAMBlock *block, 
ram_addr_t offset,
                                     bool set);
/* ram cache */
-int colo_init_ram_cache(void);
+int colo_init_ram_cache(Error **errp);
  void colo_flush_ram_cache(void);
  void colo_release_ram_cache(void);
  void colo_incoming_start_dirty_log(void);
diff --git a/migration/savevm.c b/migration/savevm.c
index 
1cbc44a5314043a403d983511066cf137681a18d..755ba7e4504d377a4649da191ad9875d9fd66f69
 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2510,15 +2510,19 @@ static int 
loadvm_handle_recv_bitmap(MigrationIncomingState *mis,
      return 0;
  }
-static int loadvm_process_enable_colo(MigrationIncomingState *mis)
+static int loadvm_process_enable_colo(MigrationIncomingState *mis,
+                                      Error **errp)
  {
-    int ret = migration_incoming_enable_colo();
+    int ret;
- if (!ret) {
-        ret = colo_init_ram_cache();
-        if (ret) {
-            migration_incoming_disable_colo();
-        }
+    if (migration_incoming_enable_colo(errp) < 0) {
+        return -1;

Returning -1 here while colo_init_ram_cache() returns -errno results in an inconsistent semantics of this function's return value.

Ideally, I think this function (and other similar functions) should stop returning -errno and instead return a bool value to prevent callers to make any assumption based on the return value other than success/failure. But that could be done later; let it return -errno until then.

Reply via email to