RE: [PATCH v1] migration: refactor migration_completion

[Wang, Wei W]

On Tuesday, July 18, 2023 1:44 PM, Isaku Yamahata wrote:
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -2058,6 +2058,21 @@ static int
> await_return_path_close_on_source(MigrationState *ms)
> >      return ms->rp_state.error;
> >  }
> >
> > +static int close_return_path_on_source(MigrationState *ms) {
> > +    int ret;
> > +
> > +    if (!ms->rp_state.rp_thread_created) {
> > +        return 0;
> > +    }
> > +
> > +    trace_migration_return_path_end_before();
> > +    ret = await_return_path_close_on_source(ms);
> > +    trace_migration_return_path_end_after(ret);
> > +
> > +    return ret;
> > +}
> > +
> 
> There is only one caller, migration_completion().  We can consolidate two
> functions, await_return_path_close_on_source() and
> close_return_path_on_source(), into single function.

Sounds good, thanks.

> > +static int 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();
> > +
> > +    return 0;
> 
> Always return 0?  Make it void.

OK.

> > +static void migration_completion(MigrationState *s) {
> > +    int ret = -1;
> > +    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) {
> > +        ret = migration_completion_postcopy(s);
> 
> Here we can set ret = 0.

Yes, after migration_completion_postcopy is made "void".