Re: [PATCH 1/2] migration: move wait-unplug loop to its own function

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Laurent Vivier (lvivier@redhat.com) wrote:
> The loop is used in migration_thread() and bg_migration_thread(),
> so we can move it to its own function and call it from these both places.
> 
> Moreover, in migration_thread() we have a wrong state transition from
> SETUP to ACTIVE while state could be WAIT_UNPLUG. This is correctly
> managed in bg_migration_thread() so use this code instead.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/migration.c | 54 +++++++++++++++++++++----------------------
>  1 file changed, 26 insertions(+), 28 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 4228635d1880..3e92c405a2b6 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3664,6 +3664,28 @@ bool migration_rate_limit(void)
>      return urgent;
>  }
>  
> +/*
> + * if failover devices are present, wait they are completely
> + * unplugged
> + */
> +
> +static void qemu_savevm_wait_unplug(MigrationState *s, int old_state,
> +                                    int new_state)
> +{
> +    if (qemu_savevm_state_guest_unplug_pending()) {
> +        migrate_set_state(&s->state, old_state, MIGRATION_STATUS_WAIT_UNPLUG);
> +
> +        while (s->state == MIGRATION_STATUS_WAIT_UNPLUG &&
> +               qemu_savevm_state_guest_unplug_pending()) {
> +            qemu_sem_timedwait(&s->wait_unplug_sem, 250);
> +        }
> +
> +        migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG, new_state);
> +    } else {
> +        migrate_set_state(&s->state, old_state, new_state);
> +    }
> +}
> +
>  /*
>   * Master migration thread on the source VM.
>   * It drives the migration and pumps the data down the outgoing channel.
> @@ -3710,22 +3732,10 @@ static void *migration_thread(void *opaque)
>  
>      qemu_savevm_state_setup(s->to_dst_file);
>  
> -    if (qemu_savevm_state_guest_unplug_pending()) {
> -        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> -                          MIGRATION_STATUS_WAIT_UNPLUG);
> -
> -        while (s->state == MIGRATION_STATUS_WAIT_UNPLUG &&
> -               qemu_savevm_state_guest_unplug_pending()) {
> -            qemu_sem_timedwait(&s->wait_unplug_sem, 250);
> -        }
> -
> -        migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG,
> -                MIGRATION_STATUS_ACTIVE);
> -    }
> +    qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
> +                               MIGRATION_STATUS_ACTIVE);
>  
>      s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
> -    migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> -                      MIGRATION_STATUS_ACTIVE);
>  
>      trace_migration_thread_setup_complete();
>  
> @@ -3833,21 +3843,9 @@ static void *bg_migration_thread(void *opaque)
>      qemu_savevm_state_header(s->to_dst_file);
>      qemu_savevm_state_setup(s->to_dst_file);
>  
> -    if (qemu_savevm_state_guest_unplug_pending()) {
> -        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> -                          MIGRATION_STATUS_WAIT_UNPLUG);
> -
> -        while (s->state == MIGRATION_STATUS_WAIT_UNPLUG &&
> -               qemu_savevm_state_guest_unplug_pending()) {
> -            qemu_sem_timedwait(&s->wait_unplug_sem, 250);
> -        }
> +    qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
> +                               MIGRATION_STATUS_ACTIVE);
>  
> -        migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG,
> -                          MIGRATION_STATUS_ACTIVE);
> -    } else {
> -        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> -                MIGRATION_STATUS_ACTIVE);
> -    }
>      s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
>  
>      trace_migration_thread_setup_complete();
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK