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

[Anthony Liguori]

On 10/17/2011 10:12 AM, Juan Quintela wrote:
> 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.

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;