Re: [Qemu-devel] [PATCH v3 2/4] migration: fix spice migration

["Michael R. Hines" <mrhines@linux.vnet.ibm.com>]

On 07/29/2013 11:01 AM, Stefan Hajnoczi wrote:
> On Mon, Jul 29, 2013 at 10:46:50AM -0400, Michael R. Hines wrote:
>> On 07/29/2013 09:01 AM, Stefan Hajnoczi wrote:
>>> Commit 29ae8a4133082e16970c9d4be09f4b6a15034617 ("rdma: introduce
>>> MIG_STATE_NONE and change MIG_STATE_SETUP state transition") changed the
>>> state transitions during migration setup.
>>>
>>> Spice used to be notified with MIG_STATE_ACTIVE and it detected this
>>> using migration_is_active().  Spice is now notified with
>>> MIG_STATE_SETUP and migration_is_active() no longer works.
>>>
>>> Replace migration_is_active() with migration_in_setup() to fix spice
>>> migration.
>>>
>>> Cc: Michael R. Hines <mrhines@us.ibm.com>
>>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>   include/migration/migration.h | 2 +-
>>>   migration.c                   | 4 ++--
>>>   ui/spice-core.c               | 2 +-
>>>   3 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/migration/migration.h b/include/migration/migration.h
>>> index 08c772d..140e6b4 100644
>>> --- a/include/migration/migration.h
>>> +++ b/include/migration/migration.h
>>> @@ -90,7 +90,7 @@ int migrate_fd_close(MigrationState *s);
>>>
>>>   void add_migration_state_change_notifier(Notifier *notify);
>>>   void remove_migration_state_change_notifier(Notifier *notify);
>>> -bool migration_is_active(MigrationState *);
>>> +bool migration_in_setup(MigrationState *);
>>>   bool migration_has_finished(MigrationState *);
>>>   bool migration_has_failed(MigrationState *);
>>>   MigrationState *migrate_get_current(void);
>>> diff --git a/migration.c b/migration.c
>>> index a5ed26b..9fc7294 100644
>>> --- a/migration.c
>>> +++ b/migration.c
>>> @@ -338,9 +338,9 @@ void remove_migration_state_change_notifier(Notifier *notify)
>>>       notifier_remove(notify);
>>>   }
>>>
>>> -bool migration_is_active(MigrationState *s)
>>> +bool migration_in_setup(MigrationState *s)
>>>   {
>>> -    return s->state == MIG_STATE_ACTIVE;
>>> +    return s->state == MIG_STATE_SETUP;
>>>   }
>>>
>>>   bool migration_has_finished(MigrationState *s)
>>> diff --git a/ui/spice-core.c b/ui/spice-core.c
>>> index f308fd9..033fd89 100644
>>> --- a/ui/spice-core.c
>>> +++ b/ui/spice-core.c
>>> @@ -563,7 +563,7 @@ static void migration_state_notifier(Notifier *notifier, void *data)
>>>   {
>>>       MigrationState *s = data;
>>>
>>> -    if (migration_is_active(s)) {
>>> +    if (migration_in_setup(s)) {
>>>           spice_server_migrate_start(spice_server);
>>>       } else if (migration_has_finished(s)) {
>>>           spice_server_migrate_end(spice_server, true);
>> Do we really have to replace this function? Can we just add a new function?
> Yes, migration_is_active() is deadcode since there are no callers.
>
> There is also no way to receive a migration state change callback with
> ACTIVE.  The notifiers are not called on the SETUP -> ACTIVE transition
> because the migration thread does cmpxchg only (it doesn't invoke
> notifiers).
>
> If someone would like it again in the future they will first need to
> decide how to notify when the migration thread transitions states.
> Leaving this function around would be misleading since it cannot be used
> correctly at the moment.
>
> Stefan
>
Acknowledged.

- Michael