Re: [Qemu-devel] [PATCH 02/11] migration: qemu_savevm_state_cleanup() in cleanup

[Juan Quintela]

Peter Xu <address@hidden> wrote:
> Moving existing callers all into migrate_fd_cleanup().  It simplifies
> migration_thread() a bit.
>
> Signed-off-by: Peter Xu <address@hidden>

Reviewed-by: Juan Quintela <address@hidden>

I am trying to see if we can call migrate_fd_cleanup() twice.  As far as
I can see, we are not doing it.  But, and it is a big but, we are not
checking that we are not calling qemu_savevm_state_cleanup() twice.  If
that happens, we can get double frees and similar.

I put the reviewed-by anyways, because I *think* that we are doing it
right now, and otherwise, we should make sure that we are not calling it
twice, not papering over it.

Once here, I have notice that we call block_cleanup_parameters() in
*three* places.  We call notifier_list_notify() on two of this places (I
can't see any good reason *why* we don't call the notifier for
migrate_fd_cancel).

So, still more review/cleanups to do on this arera.

Later, Juan.

> ---
>  migration/migration.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 0ee4b4c27c..edbda43246 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1077,6 +1077,8 @@ static void migrate_fd_cleanup(void *opaque)
>      qemu_bh_delete(s->cleanup_bh);
>      s->cleanup_bh = NULL;
>  
> +    qemu_savevm_state_cleanup();
> +
>      if (s->to_dst_file) {
>          Error *local_err = NULL;
>  
> @@ -2290,13 +2292,6 @@ static void *migration_thread(void *opaque)
>      end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>  
>      qemu_mutex_lock_iothread();
> -    /*
> -     * The resource has been allocated by migration will be reused in COLO
> -     * process, so don't release them.
> -     */
> -    if (!enable_colo) {
> -        qemu_savevm_state_cleanup();
> -    }
>      if (s->state == MIGRATION_STATUS_COMPLETED) {
>          uint64_t transferred_bytes = qemu_ftell(s->to_dst_file);
>          s->total_time = end_time - s->total_time;
> @@ -2312,7 +2307,6 @@ static void *migration_thread(void *opaque)
>          if (s->state == MIGRATION_STATUS_ACTIVE) {
>              assert(enable_colo);
>              migrate_start_colo_process(s);
> -            qemu_savevm_state_cleanup();
>              /*
>              * Fixme: we will run VM in COLO no matter its old running state.
>              * After exited COLO, we will keep running.