Re: [PATCH v6 4/5] migration: process_incoming_migration_co(): rework error reporting

[Vladimir Sementsov-Ogievskiy]

On 30.04.24 12:16, Philippe Mathieu-Daudé wrote:

On 30/4/24 10:56, Vladimir Sementsov-Ogievskiy wrote:

Unify error reporting in the function. This simplifies the following
commit, which will not-exit-on-error behavior variant to the function.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  migration/migration.c | 23 +--
  1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index b307a4bc59..a9599838e6 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -735,14 +735,16 @@ static void process_incoming_migration_bh(void *opaque)
  static void coroutine_fn
  process_incoming_migration_co(void *opaque)
  {
+    MigrationState *s = migrate_get_current();


(see below)


  MigrationIncomingState *mis = migration_incoming_get_current();
  PostcopyState ps;
  int ret;
+    Error *local_err = NULL;
  assert(mis->from_src_file);
  if (compress_threads_load_setup(mis->from_src_file)) {
-    error_report("Failed to setup decompress threads");
+    error_setg(&local_err, "Failed to setup decompress threads");
  goto fail;
  }
@@ -779,19 +781,12 @@ process_incoming_migration_co(void *opaque)
  }
  if (ret < 0) {
-    MigrationState *s = migrate_get_current();
-
-    if (migrate_has_error(s)) {
-    WITH_QEMU_LOCK_GUARD(&s->error_mutex) {
-    error_report_err(s->error);
-    s->error = NULL;
-    }
-    }
-    error_report("load of migration failed: %s", strerror(-ret));
+    error_setg(&local_err, "load of migration failed: %s", strerror(-ret));
  goto fail;
  }
  if (colo_incoming_co() < 0) {
+    error_setg(&local_err, "colo incoming failed");
  goto fail;
  }
@@ -800,8 +795,16 @@ process_incoming_migration_co(void *opaque)
  fail:


Maybe just assign @s in error path here?

    s = migrate_get_current();


I'd keep as is. If continue improving the function, I'd better split the logic to 
seperate function with classic "Error **errp" argument. And keep reporting 
error in caller.




  migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
    MIGRATION_STATUS_FAILED);
+    migrate_set_error(s, local_err);
+    error_free(local_err);
+
  migration_incoming_state_destroy();
+    WITH_QEMU_LOCK_GUARD(&s->error_mutex) {
+    error_report_err(s->error);
+    s->error = NULL;
+    }
+
  exit(EXIT_FAILURE);
  }


Reviewed-by: Philippe Mathieu-Daudé 



--
Best regards,
Vladimir

Re: [PATCH v6 4/5] migration: process_incoming_migration_co(): rework error reporting

[Philippe Mathieu-Daudé]

On 30/4/24 10:56, Vladimir Sementsov-Ogievskiy wrote:

Unify error reporting in the function. This simplifies the following
commit, which will not-exit-on-error behavior variant to the function.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  migration/migration.c | 23 +--
  1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index b307a4bc59..a9599838e6 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -735,14 +735,16 @@ static void process_incoming_migration_bh(void *opaque)
  static void coroutine_fn
  process_incoming_migration_co(void *opaque)
  {
+MigrationState *s = migrate_get_current();


(see below)


  MigrationIncomingState *mis = migration_incoming_get_current();
  PostcopyState ps;
  int ret;
+Error *local_err = NULL;
  
  assert(mis->from_src_file);
  
  if (compress_threads_load_setup(mis->from_src_file)) {

-error_report("Failed to setup decompress threads");
+error_setg(&local_err, "Failed to setup decompress threads");
  goto fail;
  }
  
@@ -779,19 +781,12 @@ process_incoming_migration_co(void *opaque)

  }
  
  if (ret < 0) {

-MigrationState *s = migrate_get_current();
-
-if (migrate_has_error(s)) {
-WITH_QEMU_LOCK_GUARD(&s->error_mutex) {
-error_report_err(s->error);
-s->error = NULL;
-}
-}
-error_report("load of migration failed: %s", strerror(-ret));
+error_setg(&local_err, "load of migration failed: %s", strerror(-ret));
  goto fail;
  }
  
  if (colo_incoming_co() < 0) {

+error_setg(&local_err, "colo incoming failed");
  goto fail;
  }
  
@@ -800,8 +795,16 @@ process_incoming_migration_co(void *opaque)

  fail:


Maybe just assign @s in error path here?

   s = migrate_get_current();


  migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
MIGRATION_STATUS_FAILED);
+migrate_set_error(s, local_err);
+error_free(local_err);
+
  migration_incoming_state_destroy();
  
+WITH_QEMU_LOCK_GUARD(&s->error_mutex) {

+error_report_err(s->error);
+s->error = NULL;
+}
+
  exit(EXIT_FAILURE);
  }
  


Reviewed-by: Philippe Mathieu-Daudé