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

[Stefan Hajnoczi <stefanha@gmail.com>]

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