Re: [Qemu-devel] [PATCH 25/36] migration: Our release callback was just free

[Anthony Liguori]

On 10/11/2011 05:00 AM, Juan Quintela wrote:
> We called it from a single place, and always with state !=
> MIG_STATE_ACTIVE.  Just remove the whole callback.  For users of the
> notifier, notice that this is exactly the case where they don't care,
> we are just freeing the state from previous failed migration (it can't
> be a sucessful one, otherwise we would not be running on that machine
> in the first place).
>
> Signed-off-by: Juan Quintela<address@hidden>
> ---
>   migration.c |   19 +------------------
>   migration.h |    1 -
>   2 files changed, 1 insertions(+), 19 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index a8e936e..689464d 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -123,10 +123,7 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject 
> **ret_data)
>           goto free_migrate_state;
>       }
>
> -    if (current_migration) {
> -        current_migration->release(current_migration);
> -    }

So ->release() calls migrate_fd_release() which if (s->state == 
MIG_STATE_ACTIVE) changes the state to MIG_STATE_CANCELLED and calls 
migrate_fd_cleanup().

This path is triggered *if* you start a migration while another is active.

Now you just free() the old migration and never bother cancelling it (and 
calling migrate_fd_cleanup).

Regards,

Anthony Liguori

> -
> +    g_free(current_migration);
>       current_migration = s;
>       notifier_list_notify(&migration_state_notifiers, NULL);
>       return 0;
> @@ -416,19 +413,6 @@ static void migrate_fd_cancel(MigrationState *s)
>       migrate_fd_cleanup(s);
>   }
>
> -static void migrate_fd_release(MigrationState *s)
> -{
> -
> -    DPRINTF("releasing state\n");
> -
> -    if (s->state == MIG_STATE_ACTIVE) {
> -        s->state = MIG_STATE_CANCELLED;
> -        notifier_list_notify(&migration_state_notifiers, NULL);
> -        migrate_fd_cleanup(s);
> -    }
> -    g_free(s);
> -}
> -
>   static void migrate_fd_wait_for_unfreeze(void *opaque)
>   {
>       MigrationState *s = opaque;
> @@ -511,7 +495,6 @@ static MigrationState *migrate_new(Monitor *mon, int64_t 
> bandwidth_limit,
>
>       s->cancel = migrate_fd_cancel;
>       s->get_status = migrate_fd_get_status;
> -    s->release = migrate_fd_release;
>       s->blk = blk;
>       s->shared = inc;
>       s->mon = NULL;
> diff --git a/migration.h b/migration.h
> index 3165140..1cdb539 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -40,7 +40,6 @@ struct MigrationState
>       int (*write)(MigrationState *s, const void *buff, size_t size);
>       void (*cancel)(MigrationState *s);
>       int (*get_status)(MigrationState *s);
> -    void (*release)(MigrationState *s);
>       void *opaque;
>       int blk;
>       int shared;