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

[Juan Quintela]

Anthony Liguori <address@hidden> 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<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);
>> -    }
>> -
>> +    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.


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;