Re: [PATCH for-9.1 v5 08/14] migration: Add Error** argument to .load_setup() handler

[Cédric Le Goater]

On 3/20/24 09:02, Markus Armbruster wrote:

Cédric Le Goater <c...@redhat.com> writes:

This will be useful to report errors at a higher level, mostly in VFIO
today.

Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org>
Reviewed-by: Peter Xu <pet...@redhat.com>
Signed-off-by: Cédric Le Goater <c...@redhat.com>
---

[...]

diff --git a/migration/savevm.c b/migration/savevm.c
index 
535ad5a32d67057dd172ce25d561a66a07172e97..8f42999a15d1685957de9ed517d6bc9ba49c3f11
 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2747,8 +2747,9 @@ 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)
  {
+    ERRP_GUARD(); /* error_prepend use */

The comment is correct, but prone to go stale.  No other use of
ERRP_GUARD() is commented.  Suggest to drop it.

OK. I found it interesting, for me at least. Will drop in the VFIO
patches also.

Thanks,

C.

      SaveStateEntry *se;
      int ret;

@@ -2763,10 +2764,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);
-            error_report("Load state of device %s failed", se->idstr);
              return ret;
          }
      }
@@ -2947,7 +2949,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;
      }