Re: [PATCH 02/14] migration: Add Error** argument to .load_setup() handler

[Cédric Le Goater]

On 2/8/24 05:30, Peter Xu wrote:

On Wed, Feb 07, 2024 at 02:33:35PM +0100, Cédric Le Goater wrote:

diff --git a/migration/ram.c b/migration/ram.c
index 
136c237f4079f68d4e578cf1c72eec2efc815bc8..8dac9bac2fe8b8c19e102c771a7ef6e976252906
 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3498,7 +3498,7 @@ void colo_release_ram_cache(void)
   * @f: QEMUFile where to receive the data
   * @opaque: RAMState pointer

Another one may need touch up..

   */
-static int ram_load_setup(QEMUFile *f, void *opaque)
+static int ram_load_setup(QEMUFile *f, void *opaque, Error **errp)
  {
      xbzrle_load_setup();
      ramblock_recv_map_init();
diff --git a/migration/savevm.c b/migration/savevm.c
index 
f2ae799bad13e631bccf733a34c3a8fd22e8dd48..990f4249a26d28117ee365d8b20fc5bbca0d43d6
 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2737,7 +2737,7 @@ static void 
qemu_loadvm_state_switchover_ack_needed(MigrationIncomingState *mis)
      trace_loadvm_state_switchover_ack_needed(mis->switchover_ack_pending_num);
  }

-static int qemu_loadvm_state_setup(QEMUFile *f)

+static int qemu_loadvm_state_setup(QEMUFile *f, Error **errp)
  {
      SaveStateEntry *se;
      int ret;
@@ -2753,10 +2753,11 @@ static int qemu_loadvm_state_setup(QEMUFile *f)
              }
          }

- ret = se->ops->load_setup(f, se->opaque);

+        ret = se->ops->load_setup(f, se->opaque, errp);
          if (ret < 0) {
+            error_prepend(errp, "Load state of device %s failed: ",
+                          se->idstr);
              qemu_file_set_error(f, ret);

Do we also want to switch to _set_error_obj()?

yes. possible.

Or even use migrate_set_error()

It seems so and may be even remove it completely.

What we could do first is add an Errp ** argument to qemu_loadvm_state()
which would improve qmp_xen_load_devices_state() and load_snapshot().
It is less obvious for process_incoming_migration_co().

(the latter may apply to previous patch too if it works)?

It seems safe to use migrate_set_error for both migration_thread() and
bg_migration_thread() because migration_detect_error() is called after
calling qemu_savevm_state_setup().

However, qemu_savevm_state() relies only on qemu_file_get_error() and
there would be a problem there I think.

Thanks,

C.

-            error_report("Load state of device %s failed", se->idstr);
              return ret;
          }
      }
@@ -2937,7 +2938,8 @@ int qemu_loadvm_state(QEMUFile *f)
          return ret;
      }

- if (qemu_loadvm_state_setup(f) != 0) {

+    if (qemu_loadvm_state_setup(f, &local_err) != 0) {
+        error_report_err(local_err);
          return -EINVAL;
      }

--

2.43.0