Current migration_completion function is a bit long. Refactor the long
implementation into different subfunctions:
- migration_completion_precopy: completion code related to precopy
- migration_completion_postcopy: completion code related to postcopy
- close_return_path_on_source: rp thread related cleanup on migration
completion. It is named to match with open_return_path_on_source.

This improves readability and is easier for future updates (e.g. add new
subfunctions when completion code related to new features are needed). No
functional changes intended.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
Changelog:
- Merge await_return_path_close_on_source into
  close_return_path_on_source as the later basically just calls the
  previous;
- make migration_completion_postcopy "void" as it doesn't return a
  value.

 migration/migration.c | 174 ++++++++++++++++++++++++------------------
 1 file changed, 98 insertions(+), 76 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 5528acb65e..f1c55d1148 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2048,9 +2048,13 @@ static int open_return_path_on_source(MigrationState *ms,
     return 0;
 }
 
-/* Returns 0 if the RP was ok, otherwise there was an error on the RP */
-static int await_return_path_close_on_source(MigrationState *ms)
+static int close_return_path_on_source(MigrationState *ms)
 {
+    if (!ms->rp_state.rp_thread_created) {
+        return 0;
+    }
+    trace_migration_return_path_end_before();
+
     /*
      * If this is a normal exit then the destination will send a SHUT and the
      * rp_thread will exit, however if there's an error we need to cause
@@ -2068,6 +2072,8 @@ static int await_return_path_close_on_source(MigrationState *ms)
     qemu_thread_join(&ms->rp_state.rp_thread);
     ms->rp_state.rp_thread_created = false;
     trace_await_return_path_close_on_source_close();
+
+    trace_migration_return_path_end_after(ms->rp_state.error);
     return ms->rp_state.error;
 }
 
@@ -2301,66 +2307,107 @@ static int migration_maybe_pause(MigrationState *s,
     return s->state == new_state ? 0 : -EINVAL;
 }
 
-/**
- * migration_completion: Used by migration_thread when there's not much left.
- *   The caller 'breaks' the loop when this returns.
- *
- * @s: Current migration state
- */
-static void migration_completion(MigrationState *s)
+static int migration_completion_precopy(MigrationState *s,
+                                        int *current_active_state)
 {
     int ret;
-    int current_active_state = s->state;
 
-    if (s->state == MIGRATION_STATUS_ACTIVE) {
-        qemu_mutex_lock_iothread();
-        s->downtime_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
-        qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, NULL);
+    qemu_mutex_lock_iothread();
+    s->downtime_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+    qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, NULL);
 
-        s->vm_old_state = runstate_get();
-        global_state_store();
+    s->vm_old_state = runstate_get();
+    global_state_store();
 
-        ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
-        trace_migration_completion_vm_stop(ret);
-        if (ret >= 0) {
-            ret = migration_maybe_pause(s, &current_active_state,
-                                        MIGRATION_STATUS_DEVICE);
-        }
-        if (ret >= 0) {
-            /*
-             * Inactivate disks except in COLO, and track that we
-             * have done so in order to remember to reactivate
-             * them if migration fails or is cancelled.
-             */
-            s->block_inactive = !migrate_colo();
-            migration_rate_set(RATE_LIMIT_DISABLED);
-            ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
-                                                     s->block_inactive);
-        }
+    ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
+    trace_migration_completion_vm_stop(ret);
+    if (ret < 0) {
+        goto out_unlock;
+    }
 
-        qemu_mutex_unlock_iothread();
+    ret = migration_maybe_pause(s, current_active_state,
+                                MIGRATION_STATUS_DEVICE);
+    if (ret < 0) {
+        goto out_unlock;
+    }
 
-        if (ret < 0) {
-            goto fail;
-        }
-    } else if (s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
-        trace_migration_completion_postcopy_end();
+    /*
+     * Inactivate disks except in COLO, and track that we have done so in order
+     * to remember to reactivate them if migration fails or is cancelled.
+     */
+    s->block_inactive = !migrate_colo();
+    migration_rate_set(RATE_LIMIT_DISABLED);
+    ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
+                                             s->block_inactive);
+out_unlock:
+    qemu_mutex_unlock_iothread();
+    return ret;
+}
 
-        qemu_mutex_lock_iothread();
-        qemu_savevm_state_complete_postcopy(s->to_dst_file);
-        qemu_mutex_unlock_iothread();
+static void migration_completion_postcopy(MigrationState *s)
+{
+    trace_migration_completion_postcopy_end();
 
+    qemu_mutex_lock_iothread();
+    qemu_savevm_state_complete_postcopy(s->to_dst_file);
+    qemu_mutex_unlock_iothread();
+
+    /*
+     * Shutdown the postcopy fast path thread.  This is only needed when dest
+     * QEMU binary is old (7.1/7.2).  QEMU 8.0+ doesn't need this.
+     */
+    if (migrate_postcopy_preempt() && s->preempt_pre_7_2) {
+        postcopy_preempt_shutdown_file(s);
+    }
+
+    trace_migration_completion_postcopy_end_after_complete();
+}
+
+static void migration_completion_failed(MigrationState *s,
+                                        int current_active_state)
+{
+    if (s->block_inactive && (s->state == MIGRATION_STATUS_ACTIVE ||
+                              s->state == MIGRATION_STATUS_DEVICE)) {
         /*
-         * Shutdown the postcopy fast path thread.  This is only needed
-         * when dest QEMU binary is old (7.1/7.2).  QEMU 8.0+ doesn't need
-         * this.
+         * If not doing postcopy, vm_start() will be called: let's
+         * regain control on images.
          */
-        if (migrate_postcopy_preempt() && s->preempt_pre_7_2) {
-            postcopy_preempt_shutdown_file(s);
+        Error *local_err = NULL;
+
+        qemu_mutex_lock_iothread();
+        bdrv_activate_all(&local_err);
+        if (local_err) {
+            error_report_err(local_err);
+        } else {
+            s->block_inactive = false;
         }
+        qemu_mutex_unlock_iothread();
+    }
 
-        trace_migration_completion_postcopy_end_after_complete();
+    migrate_set_state(&s->state, current_active_state,
+                      MIGRATION_STATUS_FAILED);
+}
+
+/**
+ * migration_completion: Used by migration_thread when there's not much left.
+ *   The caller 'breaks' the loop when this returns.
+ *
+ * @s: Current migration state
+ */
+static void migration_completion(MigrationState *s)
+{
+    int ret = 0;
+    int current_active_state = s->state;
+
+    if (s->state == MIGRATION_STATUS_ACTIVE) {
+        ret = migration_completion_precopy(s, &current_active_state);
+    } else if (s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
+        migration_completion_postcopy(s);
     } else {
+        ret = -1;
+    }
+
+    if (ret < 0) {
         goto fail;
     }
 
@@ -2370,14 +2417,8 @@ static void migration_completion(MigrationState *s)
      * it will wait for the destination to send it's status in
      * a SHUT command).
      */
-    if (s->rp_state.rp_thread_created) {
-        int rp_error;
-        trace_migration_return_path_end_before();
-        rp_error = await_return_path_close_on_source(s);
-        trace_migration_return_path_end_after(rp_error);
-        if (rp_error) {
-            goto fail;
-        }
+    if (close_return_path_on_source(s) < 0) {
+        goto fail;
     }
 
     if (qemu_file_get_error(s->to_dst_file)) {
@@ -2397,26 +2438,7 @@ static void migration_completion(MigrationState *s)
     return;
 
 fail:
-    if (s->block_inactive && (s->state == MIGRATION_STATUS_ACTIVE ||
-                              s->state == MIGRATION_STATUS_DEVICE)) {
-        /*
-         * If not doing postcopy, vm_start() will be called: let's
-         * regain control on images.
-         */
-        Error *local_err = NULL;
-
-        qemu_mutex_lock_iothread();
-        bdrv_activate_all(&local_err);
-        if (local_err) {
-            error_report_err(local_err);
-        } else {
-            s->block_inactive = false;
-        }
-        qemu_mutex_unlock_iothread();
-    }
-
-    migrate_set_state(&s->state, current_active_state,
-                      MIGRATION_STATUS_FAILED);
+    migration_completion_failed(s, current_active_state);
 }
 
 /**
-- 
2.27.0

Wei Wang <wei.w.wang@intel.com> wrote:
> Current migration_completion function is a bit long. Refactor the long
> implementation into different subfunctions:
> - migration_completion_precopy: completion code related to precopy
> - migration_completion_postcopy: completion code related to postcopy
> - close_return_path_on_source: rp thread related cleanup on migration
> completion. It is named to match with open_return_path_on_source.
>
> This improves readability and is easier for future updates (e.g. add new
> subfunctions when completion code related to new features are needed). No
> functional changes intended.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>

There was some conflict with:

commit d50f5dc075cbb891bfe4a9378600a4871264468a
Author: Fabiano Rosas <farosas@suse.de>
Date:   Mon Sep 18 14:28:20 2023 -0300

    migration: Consolidate return path closing code

(basically the traces and the rp_thread_created check were already on
the tree).

BTW, the diff is uglier than it needs to be.

You can add to your global .gitconfig:

[diff]
        algorithm = patience
        renames = true

commit e2db83d6e73df7619de75093d1477a7f3c638847
Author: Wei Wang <wei.w.wang@intel.com>
Date:   Fri Aug 4 17:30:53 2023 +0800

    migration: refactor migration_completion
    
    Current migration_completion function is a bit long. Refactor the long
    implementation into different subfunctions:
    - migration_completion_precopy: completion code related to precopy
    - migration_completion_postcopy: completion code related to postcopy
    - close_return_path_on_source: rp thread related cleanup on migration
    completion. It is named to match with open_return_path_on_source.
    
    This improves readability and is easier for future updates (e.g. add new
    subfunctions when completion code related to new features are needed). No
    functional changes intended.
    
    Signed-off-by: Wei Wang <wei.w.wang@intel.com>

diff --git a/migration/migration.c b/migration/migration.c
index 1c6c81ad49..99a06832f5 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -99,7 +99,7 @@ static int migration_maybe_pause(MigrationState *s,
                                  int *current_active_state,
                                  int new_state);
 static void migrate_fd_cancel(MigrationState *s);
-static int await_return_path_close_on_source(MigrationState *s);
+static int close_return_path_on_source(MigrationState *s);
 
 static bool migration_needs_multiple_sockets(void)
 {
@@ -1191,7 +1191,7 @@ static void migrate_fd_cleanup(MigrationState *s)
      * We already cleaned up to_dst_file, so errors from the return
      * path might be due to that, ignore them.
      */
-    await_return_path_close_on_source(s);
+    close_return_path_on_source(s);
 
     assert(!migration_is_active(s));
 
@@ -2049,8 +2049,7 @@ static int open_return_path_on_source(MigrationState *ms)
     return 0;
 }
 
-/* Returns 0 if the RP was ok, otherwise there was an error on the RP */
-static int await_return_path_close_on_source(MigrationState *ms)
+static int close_return_path_on_source(MigrationState *ms)
 {
     int ret;
 
@@ -2317,6 +2316,87 @@ static int migration_maybe_pause(MigrationState *s,
     return s->state == new_state ? 0 : -EINVAL;
 }
 
+static int migration_completion_precopy(MigrationState *s,
+                                        int *current_active_state)
+{
+    int ret;
+
+    qemu_mutex_lock_iothread();
+    s->downtime_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+    qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, NULL);
+
+    s->vm_old_state = runstate_get();
+    global_state_store();
+
+    ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
+    trace_migration_completion_vm_stop(ret);
+    if (ret < 0) {
+        goto out_unlock;
+    }
+
+    ret = migration_maybe_pause(s, current_active_state,
+                                MIGRATION_STATUS_DEVICE);
+    if (ret < 0) {
+        goto out_unlock;
+    }
+
+    /*
+     * Inactivate disks except in COLO, and track that we have done so in order
+     * to remember to reactivate them if migration fails or is cancelled.
+     */
+    s->block_inactive = !migrate_colo();
+    migration_rate_set(RATE_LIMIT_DISABLED);
+    ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
+                                             s->block_inactive);
+out_unlock:
+    qemu_mutex_unlock_iothread();
+    return ret;
+}
+
+static void migration_completion_postcopy(MigrationState *s)
+{
+    trace_migration_completion_postcopy_end();
+
+    qemu_mutex_lock_iothread();
+    qemu_savevm_state_complete_postcopy(s->to_dst_file);
+    qemu_mutex_unlock_iothread();
+
+    /*
+     * Shutdown the postcopy fast path thread.  This is only needed when dest
+     * QEMU binary is old (7.1/7.2).  QEMU 8.0+ doesn't need this.
+     */
+    if (migrate_postcopy_preempt() && s->preempt_pre_7_2) {
+        postcopy_preempt_shutdown_file(s);
+    }
+
+    trace_migration_completion_postcopy_end_after_complete();
+}
+
+static void migration_completion_failed(MigrationState *s,
+                                        int current_active_state)
+{
+    if (s->block_inactive && (s->state == MIGRATION_STATUS_ACTIVE ||
+                              s->state == MIGRATION_STATUS_DEVICE)) {
+        /*
+         * If not doing postcopy, vm_start() will be called: let's
+         * regain control on images.
+         */
+        Error *local_err = NULL;
+
+        qemu_mutex_lock_iothread();
+        bdrv_activate_all(&local_err);
+        if (local_err) {
+            error_report_err(local_err);
+        } else {
+            s->block_inactive = false;
+        }
+        qemu_mutex_unlock_iothread();
+    }
+
+    migrate_set_state(&s->state, current_active_state,
+                      MIGRATION_STATUS_FAILED);
+}
+
 /**
  * migration_completion: Used by migration_thread when there's not much left.
  *   The caller 'breaks' the loop when this returns.
@@ -2325,62 +2405,22 @@ static int migration_maybe_pause(MigrationState *s,
  */
 static void migration_completion(MigrationState *s)
 {
-    int ret;
+    int ret = 0;
     int current_active_state = s->state;
 
     if (s->state == MIGRATION_STATUS_ACTIVE) {
-        qemu_mutex_lock_iothread();
-        s->downtime_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
-        qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, NULL);
-
-        s->vm_old_state = runstate_get();
-        global_state_store();
-
-        ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
-        trace_migration_completion_vm_stop(ret);
-        if (ret >= 0) {
-            ret = migration_maybe_pause(s, &current_active_state,
-                                        MIGRATION_STATUS_DEVICE);
-        }
-        if (ret >= 0) {
-            /*
-             * Inactivate disks except in COLO, and track that we
-             * have done so in order to remember to reactivate
-             * them if migration fails or is cancelled.
-             */
-            s->block_inactive = !migrate_colo();
-            migration_rate_set(RATE_LIMIT_DISABLED);
-            ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
-                                                     s->block_inactive);
-        }
-
-        qemu_mutex_unlock_iothread();
-
-        if (ret < 0) {
-            goto fail;
-        }
+        ret = migration_completion_precopy(s, &current_active_state);
     } else if (s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
-        trace_migration_completion_postcopy_end();
-
-        qemu_mutex_lock_iothread();
-        qemu_savevm_state_complete_postcopy(s->to_dst_file);
-        qemu_mutex_unlock_iothread();
-
-        /*
-         * Shutdown the postcopy fast path thread.  This is only needed
-         * when dest QEMU binary is old (7.1/7.2).  QEMU 8.0+ doesn't need
-         * this.
-         */
-        if (migrate_postcopy_preempt() && s->preempt_pre_7_2) {
-            postcopy_preempt_shutdown_file(s);
-        }
-
-        trace_migration_completion_postcopy_end_after_complete();
+        migration_completion_postcopy(s);
     } else {
+        ret = -1;
+    }
+
+    if (ret < 0) {
         goto fail;
     }
 
-    if (await_return_path_close_on_source(s)) {
+    if (close_return_path_on_source(s) < 0) {
         goto fail;
     }
 
@@ -2401,26 +2441,7 @@ static void migration_completion(MigrationState *s)
     return;
 
 fail:
-    if (s->block_inactive && (s->state == MIGRATION_STATUS_ACTIVE ||
-                              s->state == MIGRATION_STATUS_DEVICE)) {
-        /*
-         * If not doing postcopy, vm_start() will be called: let's
-         * regain control on images.
-         */
-        Error *local_err = NULL;
-
-        qemu_mutex_lock_iothread();
-        bdrv_activate_all(&local_err);
-        if (local_err) {
-            error_report_err(local_err);
-        } else {
-            s->block_inactive = false;
-        }
-        qemu_mutex_unlock_iothread();
-    }
-
-    migrate_set_state(&s->state, current_active_state,
-                      MIGRATION_STATUS_FAILED);
+    migration_completion_failed(s, current_active_state);
 }
 
 /**
@@ -2563,7 +2584,7 @@ static MigThrError postcopy_pause(MigrationState *s)
          * path and just wait for the thread to finish. It will be
          * re-created when we resume.
          */
-        await_return_path_close_on_source(s);
+        close_return_path_on_source(s);
 
         migrate_set_state(&s->state, s->state,
                           MIGRATION_STATUS_POSTCOPY_PAUSED);

On Wednesday, October 11, 2023 8:41 PM, Juan Quintela wrote:
> Wei Wang <wei.w.wang@intel.com> wrote:
> > Current migration_completion function is a bit long. Refactor the long
> > implementation into different subfunctions:
> > - migration_completion_precopy: completion code related to precopy
> > - migration_completion_postcopy: completion code related to postcopy
> > - close_return_path_on_source: rp thread related cleanup on migration
> > completion. It is named to match with open_return_path_on_source.

Btw, just a reminder that the above line related to close_return_path_on_source
in the commit log needs to be removed as it's not added by the patch after solving
the conflicts.

On Wednesday, October 11, 2023 8:41 PM, Juan Quintela wrote:
> Wei Wang <wei.w.wang@intel.com> wrote:
> > Current migration_completion function is a bit long. Refactor the long
> > implementation into different subfunctions:
> > - migration_completion_precopy: completion code related to precopy
> > - migration_completion_postcopy: completion code related to postcopy
> > - close_return_path_on_source: rp thread related cleanup on migration
> > completion. It is named to match with open_return_path_on_source.
> >
> > This improves readability and is easier for future updates (e.g. add
> > new subfunctions when completion code related to new features are
> > needed). No functional changes intended.
> >
> > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> 
> There was some conflict with:
> 
> commit d50f5dc075cbb891bfe4a9378600a4871264468a
> Author: Fabiano Rosas <farosas@suse.de>
> Date:   Mon Sep 18 14:28:20 2023 -0300
> 
>     migration: Consolidate return path closing code
> 
> (basically the traces and the rp_thread_created check were already on the
> tree).
> 
> BTW, the diff is uglier than it needs to be.
> 
> You can add to your global .gitconfig:
> 
> [diff]
>         algorithm = patience
>         renames = true

Yeah, this generates a nicer diff, thanks.
I'll rebase and resend it.

"Wang, Wei W" <wei.w.wang@intel.com> wrote:
> On Wednesday, October 11, 2023 8:41 PM, Juan Quintela wrote:
>> Wei Wang <wei.w.wang@intel.com> wrote:
>> > Current migration_completion function is a bit long. Refactor the long
>> > implementation into different subfunctions:
>> > - migration_completion_precopy: completion code related to precopy
>> > - migration_completion_postcopy: completion code related to postcopy
>> > - close_return_path_on_source: rp thread related cleanup on migration
>> > completion. It is named to match with open_return_path_on_source.
>> >
>> > This improves readability and is easier for future updates (e.g. add
>> > new subfunctions when completion code related to new features are
>> > needed). No functional changes intended.
>> >
>> > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> 
>> There was some conflict with:
>> 
>> commit d50f5dc075cbb891bfe4a9378600a4871264468a
>> Author: Fabiano Rosas <farosas@suse.de>
>> Date:   Mon Sep 18 14:28:20 2023 -0300
>> 
>>     migration: Consolidate return path closing code
>> 
>> (basically the traces and the rp_thread_created check were already on the
>> tree).
>> 
>> BTW, the diff is uglier than it needs to be.
>> 
>> You can add to your global .gitconfig:
>> 
>> [diff]
>>         algorithm = patience
>>         renames = true
>
> Yeah, this generates a nicer diff, thanks.
> I'll rebase and resend it.

Already on the pull request.

I have to fix the conflict, but it has the same changes that yours as
far as I can see.

Later, Juan.

On Thursday, October 12, 2023 4:32 AM, Juan Quintela wrote:
> > Yeah, this generates a nicer diff, thanks.
> > I'll rebase and resend it.
> 
> Already on the pull request.
> 
> I have to fix the conflict, but it has the same changes that yours as far as I can
> see.

Yes, just need to remove the conflict part, other changes seem to be the same as before.

Wei Wang <wei.w.wang@intel.com> wrote:
> Current migration_completion function is a bit long. Refactor the long
> implementation into different subfunctions:
> - migration_completion_precopy: completion code related to precopy
> - migration_completion_postcopy: completion code related to postcopy
> - close_return_path_on_source: rp thread related cleanup on migration
> completion. It is named to match with open_return_path_on_source.
>
> This improves readability and is easier for future updates (e.g. add new
> subfunctions when completion code related to new features are needed). No
> functional changes intended.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

queued.

On Fri, Aug 04, 2023 at 05:30:53PM +0800,
Wei Wang <wei.w.wang@intel.com> wrote:

> Current migration_completion function is a bit long. Refactor the long
> implementation into different subfunctions:
> - migration_completion_precopy: completion code related to precopy
> - migration_completion_postcopy: completion code related to postcopy
> - close_return_path_on_source: rp thread related cleanup on migration
> completion. It is named to match with open_return_path_on_source.
> 
> This improves readability and is easier for future updates (e.g. add new
> subfunctions when completion code related to new features are needed). No
> functional changes intended.
> 
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
> Changelog:
> - Merge await_return_path_close_on_source into
>   close_return_path_on_source as the later basically just calls the
>   previous;
> - make migration_completion_postcopy "void" as it doesn't return a
>   value.

Reviewed-by: Isaku Yamahata <isaku.yamahata@intel.com>
-- 
Isaku Yamahata <isaku.yamahata@gmail.com>

On Fri, Aug 04, 2023 at 05:30:53PM +0800, Wei Wang wrote:
> Current migration_completion function is a bit long. Refactor the long
> implementation into different subfunctions:
> - migration_completion_precopy: completion code related to precopy
> - migration_completion_postcopy: completion code related to postcopy
> - close_return_path_on_source: rp thread related cleanup on migration
> completion. It is named to match with open_return_path_on_source.
> 
> This improves readability and is easier for future updates (e.g. add new
> subfunctions when completion code related to new features are needed). No
> functional changes intended.
> 
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

On Friday, August 4, 2023 9:37 PM, Peter Xu wrote:
Fri, Aug 04, 2023 at 05:30:53PM +0800, Wei Wang wrote:
> > Current migration_completion function is a bit long. Refactor the long
> > implementation into different subfunctions:
> > - migration_completion_precopy: completion code related to precopy
> > - migration_completion_postcopy: completion code related to postcopy
> > - close_return_path_on_source: rp thread related cleanup on migration
> > completion. It is named to match with open_return_path_on_source.
> >
> > This improves readability and is easier for future updates (e.g. add
> > new subfunctions when completion code related to new features are
> > needed). No functional changes intended.
> >
> > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> 

Hi Juan,
Do you think this refactoring would be good to merge or have any more comments?

Thanks,
Wei