Cleanup bg_migration_thread() function on error handling.  First of all,
early_fail is almost only used to say if BQL is taken.  Since we already
have separate jumping labels, we don't really need it, hence removed.

Also, since local_err is around, making sure every failure path will set a
proper error string for the failure, then propagate to MigrationState.error.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 2cbeebc9ba..ddf6faab46 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3616,7 +3616,6 @@ static void *bg_migration_thread(void *opaque)
     int64_t setup_start;
     MigThrError thr_error;
     QEMUFile *fb;
-    bool early_fail = true;
     Error *local_err = NULL;
     int ret;
 
@@ -3662,10 +3661,7 @@ static void *bg_migration_thread(void *opaque)
      * devices to unplug. This to preserve migration state transitions.
      */
     if (ret) {
-        migrate_error_propagate(s, local_err);
-        migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
-                          MIGRATION_STATUS_FAILED);
-        goto fail_setup;
+        goto fail;
     }
 
     s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
@@ -3675,11 +3671,13 @@ static void *bg_migration_thread(void *opaque)
     bql_lock();
 
     if (migration_stop_vm(s, RUN_STATE_PAUSED)) {
-        goto fail;
+        error_setg(&local_err, "Failed to stop the VM");
+        goto fail_with_bql;
     }
 
     if (qemu_savevm_state_non_iterable(fb)) {
-        goto fail;
+        error_setg(&local_err, "Failed to save non-iterable devices");
+        goto fail_with_bql;
     }
 
     qemu_savevm_state_end_precopy(s, fb);
@@ -3692,9 +3690,9 @@ static void *bg_migration_thread(void *opaque)
 
     /* Now initialize UFFD context and start tracking RAM writes */
     if (ram_write_tracking_start()) {
-        goto fail;
+        error_setg(&local_err, "Failed to start write tracking");
+        goto fail_with_bql;
     }
-    early_fail = false;
 
     /*
      * Start VM from BH handler to avoid write-fault lock here.
@@ -3726,21 +3724,22 @@ static void *bg_migration_thread(void *opaque)
     }
 
     trace_migration_thread_after_loop();
+    goto done;
+
+fail_with_bql:
+    bql_unlock();
 
 fail:
-    if (early_fail) {
-        migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
-                MIGRATION_STATUS_FAILED);
-        bql_unlock();
-    }
+    /* local_err is guaranteed to be set when reaching here */
+    migrate_error_propagate(s, local_err);
+    migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
+                      MIGRATION_STATUS_FAILED);
 
-fail_setup:
+done:
     bg_migration_iteration_finish(s);
-
     qemu_fclose(fb);
     object_unref(OBJECT(s));
     rcu_unregister_thread();
-
     return NULL;
 }
 
-- 
2.50.1

Peter Xu <peterx@redhat.com> writes:

> Cleanup bg_migration_thread() function on error handling.  First of all,
> early_fail is almost only used to say if BQL is taken.  Since we already
> have separate jumping labels, we don't really need it, hence removed.
>
> Also, since local_err is around, making sure every failure path will set a
> proper error string for the failure, then propagate to MigrationState.error.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>