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

[Anthony Liguori <anthony@codemonkey.ws>]

On 10/17/2011 10:12 AM, Juan Quintela wrote:
> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>> 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<quintela@redhat.com>
>>> ---
>>>    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);
>>> -    }
>>> -
>>> +    g_free(current_migration);
>>
>> migrate_fd_cleanup() is no longer being called.
>
> It was never called.
>
>> Regards,
>>
>> Anthony Liguori
>>
>>>        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) {
>
> The first thing that we look is that MIG_STATE_ACTIVE, and we call
> migrate_fd_cleanup() on that case.

It was meant to cancel existing migration but there was a later patch that 
didn't allow the implicit cancel anymore.

So ignore my comments here.

Regards,

Anthony Liguori

>
>
> See the beggining of do_migrate().  If we are in MIG_STATE_ACTIVE, we
> just don't continue. the function.  Notice that you complained about
> comments being bad on the other patches, and in this very case, it was
> explicitely stated on the comment O:-)
>
> int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
> {
>      MigrationState *s = NULL;
>      const char *p;
>      int detach = qdict_get_try_bool(qdict, "detach", 0);
>      int blk = qdict_get_try_bool(qdict, "blk", 0);
>      int inc = qdict_get_try_bool(qdict, "inc", 0);
>      const char *uri = qdict_get_str(qdict, "uri");
>
>      if (current_migration&&
>          current_migration->get_status(current_migration) == MIG_STATE_ACTIVE) {
>          monitor_printf(mon, "migration already in progress\n");
>          return -1;
>
>          ************ We stop here ****
>          call to ->release() is below this.
>      }
>
>      if (qemu_savevm_state_blocked(mon)) {
>          return -1;
>      }
>
>
>
>>> -        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;
>