[Qemu-devel] [PATCH 00/22] Refactor and cleaup migration code

[Qemu-devel] [PATCH 00/22] Refactor and cleaup migration code

[Juan Quintela]

This series:
- Fold MigrationState into FdMigrationState (and then rename)
- Factorize migration statec creation in a single place
- Make use of MIG_STATE_*, setup through helpers and make them local
- remove relase & cancel callbacks (where used only one in same
  file than defined)
- get_status() is no more, just access directly to .state
- current_migration use cleanup, and make variable static
- max_throotle is gone, now inside current_migration
- change get_migration_status() to migration_has_finished()
  and actualize single user.

Please review.

Later, Juan.

Juan Quintela (22):
  migration: Make *start_outgoing_migration return FdMigrationState
  migration: Use FdMigrationState instead of MigrationState when
    possible
  migration: Fold MigrationState into FdMigrationState
  migration: Rename FdMigrationState MigrationState
  migration: Refactor MigrationState creation
  migration: Make all posible migration functions static
  migration: move migrate_create_state to do_migrate
  migration: Check that migration is active before cancel it
  migration: Introduce MIG_STATE_NONE
  migration: Refactor and simplify error checking in
    migrate_fd_put_ready
  migration: Introduce migrate_fd_completed() for consistenncy
  migration: Use migrate_fd_error() in last place that set status to
    ERROR
  migration: Our release callback was just free
  migration: Remove get_status() accessor
  migration: Remove migration cancel() callback
  migration: Move exported functions to the end of the file
  migration: use global variable directly
  migration: another case of global variable assigned to local one
  migration: convert current_migration from pointer to struct
  migration: Use bandwidth_limit directly
  migration: Export a function that tells if the migration has finished
    correctly
  migration: Make state definitions local

 migration-exec.c |   39 +----
 migration-fd.c   |   42 ++-----
 migration-tcp.c  |   41 ++----
 migration-unix.c |   40 ++----
 migration.c      |  399 ++++++++++++++++++++++++++----------------------------
 migration.h      |   85 ++----------
 ui/spice-core.c  |    4 +-
 7 files changed, 238 insertions(+), 412 deletions(-)

-- 
1.7.4

[Qemu-devel] [PATCH 01/22] migration: Make *start_outgoing_migration return FdMigrationState

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration-exec.c |    4 ++--
 migration-fd.c   |    4 ++--
 migration-tcp.c  |    4 ++--
 migration-unix.c |    4 ++--
 migration.c      |    4 ++--
 migration.h      |    8 ++++----
 6 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/migration-exec.c b/migration-exec.c
index 14718dd..b49a475 100644
--- a/migration-exec.c
+++ b/migration-exec.c
@@ -62,7 +62,7 @@ static int exec_close(FdMigrationState *s)
     return ret;
 }

-MigrationState *exec_start_outgoing_migration(Monitor *mon,
+FdMigrationState *exec_start_outgoing_migration(Monitor *mon,
                                               const char *command,
 					      int64_t bandwidth_limit,
 					      int detach,
@@ -109,7 +109,7 @@ MigrationState *exec_start_outgoing_migration(Monitor *mon,
     }

     migrate_fd_connect(s);
-    return &s->mig_state;
+    return s;

 err_after_open:
     pclose(f);
diff --git a/migration-fd.c b/migration-fd.c
index 6d14505..bd5e8a9 100644
--- a/migration-fd.c
+++ b/migration-fd.c
@@ -51,7 +51,7 @@ static int fd_close(FdMigrationState *s)
     return 0;
 }

-MigrationState *fd_start_outgoing_migration(Monitor *mon,
+FdMigrationState *fd_start_outgoing_migration(Monitor *mon,
 					    const char *fdname,
 					    int64_t bandwidth_limit,
 					    int detach,
@@ -92,7 +92,7 @@ MigrationState *fd_start_outgoing_migration(Monitor *mon,
     }

     migrate_fd_connect(s);
-    return &s->mig_state;
+    return s;

 err_after_open:
     close(s->fd);
diff --git a/migration-tcp.c b/migration-tcp.c
index b55f419..355bc37 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -76,7 +76,7 @@ static void tcp_wait_for_connect(void *opaque)
     }
 }

-MigrationState *tcp_start_outgoing_migration(Monitor *mon,
+FdMigrationState *tcp_start_outgoing_migration(Monitor *mon,
                                              const char *host_port,
                                              int64_t bandwidth_limit,
                                              int detach,
@@ -132,7 +132,7 @@ MigrationState *tcp_start_outgoing_migration(Monitor *mon,
     } else if (ret >= 0)
         migrate_fd_connect(s);

-    return &s->mig_state;
+    return s;
 }

 static void tcp_accept_incoming_migration(void *opaque)
diff --git a/migration-unix.c b/migration-unix.c
index 57232c0..b9b0dbf 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -75,7 +75,7 @@ static void unix_wait_for_connect(void *opaque)
     }
 }

-MigrationState *unix_start_outgoing_migration(Monitor *mon,
+FdMigrationState *unix_start_outgoing_migration(Monitor *mon,
                                               const char *path,
 					      int64_t bandwidth_limit,
 					      int detach,
@@ -133,7 +133,7 @@ MigrationState *unix_start_outgoing_migration(Monitor *mon,
     if (ret >= 0)
         migrate_fd_connect(s);

-    return &s->mig_state;
+    return s;

 err_after_open:
     close(s->fd);
diff --git a/migration.c b/migration.c
index af3a1f2..f9aaadc 100644
--- a/migration.c
+++ b/migration.c
@@ -78,7 +78,7 @@ void process_incoming_migration(QEMUFile *f)

 int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
-    MigrationState *s = NULL;
+    FdMigrationState *s = NULL;
     const char *p;
     int detach = qdict_get_try_bool(qdict, "detach", 0);
     int blk = qdict_get_try_bool(qdict, "blk", 0);
@@ -123,7 +123,7 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
         current_migration->release(current_migration);
     }

-    current_migration = s;
+    current_migration = &s->mig_state;
     notifier_list_notify(&migration_state_notifiers);
     return 0;
 }
diff --git a/migration.h b/migration.h
index 2170792..db0e45a 100644
--- a/migration.h
+++ b/migration.h
@@ -72,7 +72,7 @@ void do_info_migrate(Monitor *mon, QObject **ret_data);

 int exec_start_incoming_migration(const char *host_port);

-MigrationState *exec_start_outgoing_migration(Monitor *mon,
+FdMigrationState *exec_start_outgoing_migration(Monitor *mon,
                                               const char *host_port,
 					      int64_t bandwidth_limit,
 					      int detach,
@@ -81,7 +81,7 @@ MigrationState *exec_start_outgoing_migration(Monitor *mon,

 int tcp_start_incoming_migration(const char *host_port);

-MigrationState *tcp_start_outgoing_migration(Monitor *mon,
+FdMigrationState *tcp_start_outgoing_migration(Monitor *mon,
                                              const char *host_port,
 					     int64_t bandwidth_limit,
 					     int detach,
@@ -90,7 +90,7 @@ MigrationState *tcp_start_outgoing_migration(Monitor *mon,

 int unix_start_incoming_migration(const char *path);

-MigrationState *unix_start_outgoing_migration(Monitor *mon,
+FdMigrationState *unix_start_outgoing_migration(Monitor *mon,
                                               const char *path,
 					      int64_t bandwidth_limit,
 					      int detach,
@@ -99,7 +99,7 @@ MigrationState *unix_start_outgoing_migration(Monitor *mon,

 int fd_start_incoming_migration(const char *path);

-MigrationState *fd_start_outgoing_migration(Monitor *mon,
+FdMigrationState *fd_start_outgoing_migration(Monitor *mon,
 					    const char *fdname,
 					    int64_t bandwidth_limit,
 					    int detach,
-- 
1.7.4

[Qemu-devel] [PATCH 02/22] migration: Use FdMigrationState instead of MigrationState when possible

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |   31 ++++++++++++++-----------------
 migration.h |   16 ++++++++--------
 2 files changed, 22 insertions(+), 25 deletions(-)

diff --git a/migration.c b/migration.c
index f9aaadc..3a371a3 100644
--- a/migration.c
+++ b/migration.c
@@ -34,7 +34,7 @@
 /* Migration speed throttling */
 static int64_t max_throttle = (32 << 20);

-static MigrationState *current_migration;
+static FdMigrationState *current_migration;

 static NotifierList migration_state_notifiers =
     NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
@@ -86,7 +86,7 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
     const char *uri = qdict_get_str(qdict, "uri");

     if (current_migration &&
-        current_migration->get_status(current_migration) == MIG_STATE_ACTIVE) {
+        current_migration->mig_state.get_status(current_migration) == MIG_STATE_ACTIVE) {
         monitor_printf(mon, "migration already in progress\n");
         return -1;
     }
@@ -120,20 +120,20 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
     }

     if (current_migration) {
-        current_migration->release(current_migration);
+        current_migration->mig_state.release(current_migration);
     }

-    current_migration = &s->mig_state;
+    current_migration = s;
     notifier_list_notify(&migration_state_notifiers);
     return 0;
 }

 int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
-    MigrationState *s = current_migration;
+    FdMigrationState *s = current_migration;

     if (s)
-        s->cancel(s);
+        s->mig_state.cancel(s);

     return 0;
 }
@@ -149,7 +149,7 @@ int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
     }
     max_throttle = d;

-    s = migrate_to_fms(current_migration);
+    s = current_migration;
     if (s && s->file) {
         qemu_file_set_rate_limit(s->file, max_throttle);
     }
@@ -227,10 +227,11 @@ static void migrate_put_status(QDict *qdict, const char *name,
 void do_info_migrate(Monitor *mon, QObject **ret_data)
 {
     QDict *qdict;
-    MigrationState *s = current_migration;

-    if (s) {
-        switch (s->get_status(s)) {
+    if (current_migration) {
+        MigrationState *s = &current_migration->mig_state;
+
+        switch (s->get_status(current_migration)) {
         case MIG_STATE_ACTIVE:
             qdict = qdict_new();
             qdict_put(qdict, "status", qstring_from_str("active"));
@@ -399,16 +400,13 @@ void migrate_fd_put_ready(void *opaque)
     }
 }

-int migrate_fd_get_status(MigrationState *mig_state)
+int migrate_fd_get_status(FdMigrationState *s)
 {
-    FdMigrationState *s = migrate_to_fms(mig_state);
     return s->state;
 }

-void migrate_fd_cancel(MigrationState *mig_state)
+void migrate_fd_cancel(FdMigrationState *s)
 {
-    FdMigrationState *s = migrate_to_fms(mig_state);
-
     if (s->state != MIG_STATE_ACTIVE)
         return;

@@ -421,9 +419,8 @@ void migrate_fd_cancel(MigrationState *mig_state)
     migrate_fd_cleanup(s);
 }

-void migrate_fd_release(MigrationState *mig_state)
+void migrate_fd_release(FdMigrationState *s)
 {
-    FdMigrationState *s = migrate_to_fms(mig_state);

     DPRINTF("releasing state\n");
    
diff --git a/migration.h b/migration.h
index db0e45a..f49a9e2 100644
--- a/migration.h
+++ b/migration.h
@@ -25,18 +25,18 @@

 typedef struct MigrationState MigrationState;

+typedef struct FdMigrationState FdMigrationState;
+
 struct MigrationState
 {
     /* FIXME: add more accessors to print migration info */
-    void (*cancel)(MigrationState *s);
-    int (*get_status)(MigrationState *s);
-    void (*release)(MigrationState *s);
+    void (*cancel)(FdMigrationState *s);
+    int (*get_status)(FdMigrationState *s);
+    void (*release)(FdMigrationState *s);
     int blk;
     int shared;
 };

-typedef struct FdMigrationState FdMigrationState;
-
 struct FdMigrationState
 {
     MigrationState mig_state;
@@ -120,11 +120,11 @@ void migrate_fd_connect(FdMigrationState *s);

 void migrate_fd_put_ready(void *opaque);

-int migrate_fd_get_status(MigrationState *mig_state);
+int migrate_fd_get_status(FdMigrationState *mig_state);

-void migrate_fd_cancel(MigrationState *mig_state);
+void migrate_fd_cancel(FdMigrationState *mig_state);

-void migrate_fd_release(MigrationState *mig_state);
+void migrate_fd_release(FdMigrationState *mig_state);

 void migrate_fd_wait_for_unfreeze(void *opaque);

-- 
1.7.4

[Qemu-devel] [PATCH 03/22] migration: Fold MigrationState into FdMigrationState

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration-exec.c |   10 +++++-----
 migration-fd.c   |   10 +++++-----
 migration-tcp.c  |   10 +++++-----
 migration-unix.c |   10 +++++-----
 migration.c      |   11 +++++------
 migration.h      |   23 +++++------------------
 6 files changed, 30 insertions(+), 44 deletions(-)

diff --git a/migration-exec.c b/migration-exec.c
index b49a475..02b0667 100644
--- a/migration-exec.c
+++ b/migration-exec.c
@@ -93,12 +93,12 @@ FdMigrationState *exec_start_outgoing_migration(Monitor *mon,
     s->close = exec_close;
     s->get_error = file_errno;
     s->write = file_write;
-    s->mig_state.cancel = migrate_fd_cancel;
-    s->mig_state.get_status = migrate_fd_get_status;
-    s->mig_state.release = migrate_fd_release;
+    s->cancel = migrate_fd_cancel;
+    s->get_status = migrate_fd_get_status;
+    s->release = migrate_fd_release;

-    s->mig_state.blk = blk;
-    s->mig_state.shared = inc;
+    s->blk = blk;
+    s->shared = inc;

     s->state = MIG_STATE_ACTIVE;
     s->mon = NULL;
diff --git a/migration-fd.c b/migration-fd.c
index bd5e8a9..ccba86b 100644
--- a/migration-fd.c
+++ b/migration-fd.c
@@ -76,12 +76,12 @@ FdMigrationState *fd_start_outgoing_migration(Monitor *mon,
     s->get_error = fd_errno;
     s->write = fd_write;
     s->close = fd_close;
-    s->mig_state.cancel = migrate_fd_cancel;
-    s->mig_state.get_status = migrate_fd_get_status;
-    s->mig_state.release = migrate_fd_release;
+    s->cancel = migrate_fd_cancel;
+    s->get_status = migrate_fd_get_status;
+    s->release = migrate_fd_release;

-    s->mig_state.blk = blk;
-    s->mig_state.shared = inc;
+    s->blk = blk;
+    s->shared = inc;

     s->state = MIG_STATE_ACTIVE;
     s->mon = NULL;
diff --git a/migration-tcp.c b/migration-tcp.c
index 355bc37..02b01ed 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -95,12 +95,12 @@ FdMigrationState *tcp_start_outgoing_migration(Monitor *mon,
     s->get_error = socket_errno;
     s->write = socket_write;
     s->close = tcp_close;
-    s->mig_state.cancel = migrate_fd_cancel;
-    s->mig_state.get_status = migrate_fd_get_status;
-    s->mig_state.release = migrate_fd_release;
+    s->cancel = migrate_fd_cancel;
+    s->get_status = migrate_fd_get_status;
+    s->release = migrate_fd_release;

-    s->mig_state.blk = blk;
-    s->mig_state.shared = inc;
+    s->blk = blk;
+    s->shared = inc;

     s->state = MIG_STATE_ACTIVE;
     s->mon = NULL;
diff --git a/migration-unix.c b/migration-unix.c
index b9b0dbf..fb73f46 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -94,12 +94,12 @@ FdMigrationState *unix_start_outgoing_migration(Monitor *mon,
     s->get_error = unix_errno;
     s->write = unix_write;
     s->close = unix_close;
-    s->mig_state.cancel = migrate_fd_cancel;
-    s->mig_state.get_status = migrate_fd_get_status;
-    s->mig_state.release = migrate_fd_release;
+    s->cancel = migrate_fd_cancel;
+    s->get_status = migrate_fd_get_status;
+    s->release = migrate_fd_release;

-    s->mig_state.blk = blk;
-    s->mig_state.shared = inc;
+    s->blk = blk;
+    s->shared = inc;

     s->state = MIG_STATE_ACTIVE;
     s->mon = NULL;
diff --git a/migration.c b/migration.c
index 3a371a3..dd4cdab 100644
--- a/migration.c
+++ b/migration.c
@@ -86,7 +86,7 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
     const char *uri = qdict_get_str(qdict, "uri");

     if (current_migration &&
-        current_migration->mig_state.get_status(current_migration) == MIG_STATE_ACTIVE) {
+        current_migration->get_status(current_migration) == MIG_STATE_ACTIVE) {
         monitor_printf(mon, "migration already in progress\n");
         return -1;
     }
@@ -120,7 +120,7 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
     }

     if (current_migration) {
-        current_migration->mig_state.release(current_migration);
+        current_migration->release(current_migration);
     }

     current_migration = s;
@@ -133,7 +133,7 @@ int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
     FdMigrationState *s = current_migration;

     if (s)
-        s->mig_state.cancel(s);
+        s->cancel(s);

     return 0;
 }
@@ -229,7 +229,7 @@ void do_info_migrate(Monitor *mon, QObject **ret_data)
     QDict *qdict;

     if (current_migration) {
-        MigrationState *s = &current_migration->mig_state;
+        FdMigrationState *s = current_migration;

         switch (s->get_status(current_migration)) {
         case MIG_STATE_ACTIVE:
@@ -353,8 +353,7 @@ void migrate_fd_connect(FdMigrationState *s)
                                       migrate_fd_close);

     DPRINTF("beginning savevm\n");
-    ret = qemu_savevm_state_begin(s->mon, s->file, s->mig_state.blk,
-                                  s->mig_state.shared);
+    ret = qemu_savevm_state_begin(s->mon, s->file, s->blk, s->shared);
     if (ret < 0) {
         DPRINTF("failed, %d\n", ret);
         migrate_fd_error(s);
diff --git a/migration.h b/migration.h
index f49a9e2..93441e4 100644
--- a/migration.h
+++ b/migration.h
@@ -23,23 +23,10 @@
 #define MIG_STATE_CANCELLED	1
 #define MIG_STATE_ACTIVE	2

-typedef struct MigrationState MigrationState;
-
 typedef struct FdMigrationState FdMigrationState;

-struct MigrationState
-{
-    /* FIXME: add more accessors to print migration info */
-    void (*cancel)(FdMigrationState *s);
-    int (*get_status)(FdMigrationState *s);
-    void (*release)(FdMigrationState *s);
-    int blk;
-    int shared;
-};
-
 struct FdMigrationState
 {
-    MigrationState mig_state;
     int64_t bandwidth_limit;
     QEMUFile *file;
     int fd;
@@ -48,7 +35,12 @@ struct FdMigrationState
     int (*get_error)(struct FdMigrationState*);
     int (*close)(struct FdMigrationState*);
     int (*write)(struct FdMigrationState*, const void *, size_t);
+    void (*cancel)(FdMigrationState *s);
+    int (*get_status)(FdMigrationState *s);
+    void (*release)(FdMigrationState *s);
     void *opaque;
+    int blk;
+    int shared;
 };

 void process_incoming_migration(QEMUFile *f);
@@ -130,11 +122,6 @@ void migrate_fd_wait_for_unfreeze(void *opaque);

 int migrate_fd_close(void *opaque);

-static inline FdMigrationState *migrate_to_fms(MigrationState *mig_state)
-{
-    return container_of(mig_state, FdMigrationState, mig_state);
-}
-
 void add_migration_state_change_notifier(Notifier *notify);
 void remove_migration_state_change_notifier(Notifier *notify);
 int get_migration_state(void);
-- 
1.7.4

[Qemu-devel] [PATCH 04/22] migration: Rename FdMigrationState MigrationState

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration-exec.c |   10 +++++-----
 migration-fd.c   |   10 +++++-----
 migration-tcp.c  |   12 ++++++------
 migration-unix.c |   12 ++++++------
 migration.c      |   34 +++++++++++++++++-----------------
 migration.h      |   38 +++++++++++++++++++-------------------
 6 files changed, 58 insertions(+), 58 deletions(-)

diff --git a/migration-exec.c b/migration-exec.c
index 02b0667..e91e1e2 100644
--- a/migration-exec.c
+++ b/migration-exec.c
@@ -33,17 +33,17 @@
     do { } while (0)
 #endif

-static int file_errno(FdMigrationState *s)
+static int file_errno(MigrationState *s)
 {
     return errno;
 }

-static int file_write(FdMigrationState *s, const void * buf, size_t size)
+static int file_write(MigrationState *s, const void * buf, size_t size)
 {
     return write(s->fd, buf, size);
 }

-static int exec_close(FdMigrationState *s)
+static int exec_close(MigrationState *s)
 {
     int ret = 0;
     DPRINTF("exec_close\n");
@@ -62,14 +62,14 @@ static int exec_close(FdMigrationState *s)
     return ret;
 }

-FdMigrationState *exec_start_outgoing_migration(Monitor *mon,
+MigrationState *exec_start_outgoing_migration(Monitor *mon,
                                               const char *command,
 					      int64_t bandwidth_limit,
 					      int detach,
 					      int blk,
 					      int inc)
 {
-    FdMigrationState *s;
+    MigrationState *s;
     FILE *f;

     s = qemu_mallocz(sizeof(*s));
diff --git a/migration-fd.c b/migration-fd.c
index ccba86b..4073000 100644
--- a/migration-fd.c
+++ b/migration-fd.c
@@ -31,17 +31,17 @@
     do { } while (0)
 #endif

-static int fd_errno(FdMigrationState *s)
+static int fd_errno(MigrationState *s)
 {
     return errno;
 }

-static int fd_write(FdMigrationState *s, const void * buf, size_t size)
+static int fd_write(MigrationState *s, const void * buf, size_t size)
 {
     return write(s->fd, buf, size);
 }

-static int fd_close(FdMigrationState *s)
+static int fd_close(MigrationState *s)
 {
     DPRINTF("fd_close\n");
     if (s->fd != -1) {
@@ -51,14 +51,14 @@ static int fd_close(FdMigrationState *s)
     return 0;
 }

-FdMigrationState *fd_start_outgoing_migration(Monitor *mon,
+MigrationState *fd_start_outgoing_migration(Monitor *mon,
 					    const char *fdname,
 					    int64_t bandwidth_limit,
 					    int detach,
 					    int blk,
 					    int inc)
 {
-    FdMigrationState *s;
+    MigrationState *s;

     s = qemu_mallocz(sizeof(*s));

diff --git a/migration-tcp.c b/migration-tcp.c
index 02b01ed..e8fa46c 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -29,17 +29,17 @@
     do { } while (0)
 #endif

-static int socket_errno(FdMigrationState *s)
+static int socket_errno(MigrationState *s)
 {
     return socket_error();
 }

-static int socket_write(FdMigrationState *s, const void * buf, size_t size)
+static int socket_write(MigrationState *s, const void * buf, size_t size)
 {
     return send(s->fd, buf, size, 0);
 }

-static int tcp_close(FdMigrationState *s)
+static int tcp_close(MigrationState *s)
 {
     DPRINTF("tcp_close\n");
     if (s->fd != -1) {
@@ -52,7 +52,7 @@ static int tcp_close(FdMigrationState *s)

 static void tcp_wait_for_connect(void *opaque)
 {
-    FdMigrationState *s = opaque;
+    MigrationState *s = opaque;
     int val, ret;
     socklen_t valsize = sizeof(val);

@@ -76,7 +76,7 @@ static void tcp_wait_for_connect(void *opaque)
     }
 }

-FdMigrationState *tcp_start_outgoing_migration(Monitor *mon,
+MigrationState *tcp_start_outgoing_migration(Monitor *mon,
                                              const char *host_port,
                                              int64_t bandwidth_limit,
                                              int detach,
@@ -84,7 +84,7 @@ FdMigrationState *tcp_start_outgoing_migration(Monitor *mon,
 					     int inc)
 {
     struct sockaddr_in addr;
-    FdMigrationState *s;
+    MigrationState *s;
     int ret;

     if (parse_host_port(&addr, host_port) < 0)
diff --git a/migration-unix.c b/migration-unix.c
index fb73f46..9b74401 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -29,17 +29,17 @@
     do { } while (0)
 #endif

-static int unix_errno(FdMigrationState *s)
+static int unix_errno(MigrationState *s)
 {
     return errno;
 }

-static int unix_write(FdMigrationState *s, const void * buf, size_t size)
+static int unix_write(MigrationState *s, const void * buf, size_t size)
 {
     return write(s->fd, buf, size);
 }

-static int unix_close(FdMigrationState *s)
+static int unix_close(MigrationState *s)
 {
     DPRINTF("unix_close\n");
     if (s->fd != -1) {
@@ -51,7 +51,7 @@ static int unix_close(FdMigrationState *s)

 static void unix_wait_for_connect(void *opaque)
 {
-    FdMigrationState *s = opaque;
+    MigrationState *s = opaque;
     int val, ret;
     socklen_t valsize = sizeof(val);

@@ -75,14 +75,14 @@ static void unix_wait_for_connect(void *opaque)
     }
 }

-FdMigrationState *unix_start_outgoing_migration(Monitor *mon,
+MigrationState *unix_start_outgoing_migration(Monitor *mon,
                                               const char *path,
 					      int64_t bandwidth_limit,
 					      int detach,
 					      int blk,
 					      int inc)
 {
-    FdMigrationState *s;
+    MigrationState *s;
     struct sockaddr_un addr;
     int ret;

diff --git a/migration.c b/migration.c
index dd4cdab..9ad45d6 100644
--- a/migration.c
+++ b/migration.c
@@ -34,7 +34,7 @@
 /* Migration speed throttling */
 static int64_t max_throttle = (32 << 20);

-static FdMigrationState *current_migration;
+static MigrationState *current_migration;

 static NotifierList migration_state_notifiers =
     NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
@@ -78,7 +78,7 @@ void process_incoming_migration(QEMUFile *f)

 int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
-    FdMigrationState *s = NULL;
+    MigrationState *s = NULL;
     const char *p;
     int detach = qdict_get_try_bool(qdict, "detach", 0);
     int blk = qdict_get_try_bool(qdict, "blk", 0);
@@ -130,7 +130,7 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)

 int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
-    FdMigrationState *s = current_migration;
+    MigrationState *s = current_migration;

     if (s)
         s->cancel(s);
@@ -141,7 +141,7 @@ int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
 int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     int64_t d;
-    FdMigrationState *s;
+    MigrationState *s;

     d = qdict_get_int(qdict, "value");
     if (d < 0) {
@@ -229,7 +229,7 @@ void do_info_migrate(Monitor *mon, QObject **ret_data)
     QDict *qdict;

     if (current_migration) {
-        FdMigrationState *s = current_migration;
+        MigrationState *s = current_migration;

         switch (s->get_status(current_migration)) {
         case MIG_STATE_ACTIVE:
@@ -262,7 +262,7 @@ void do_info_migrate(Monitor *mon, QObject **ret_data)

 /* shared migration helpers */

-void migrate_fd_monitor_suspend(FdMigrationState *s, Monitor *mon)
+void migrate_fd_monitor_suspend(MigrationState *s, Monitor *mon)
 {
     s->mon = mon;
     if (monitor_suspend(mon) == 0) {
@@ -273,7 +273,7 @@ void migrate_fd_monitor_suspend(FdMigrationState *s, Monitor *mon)
     }
 }

-void migrate_fd_error(FdMigrationState *s)
+void migrate_fd_error(MigrationState *s)
 {
     DPRINTF("setting error state\n");
     s->state = MIG_STATE_ERROR;
@@ -281,7 +281,7 @@ void migrate_fd_error(FdMigrationState *s)
     migrate_fd_cleanup(s);
 }

-int migrate_fd_cleanup(FdMigrationState *s)
+int migrate_fd_cleanup(MigrationState *s)
 {
     int ret = 0;

@@ -310,7 +310,7 @@ int migrate_fd_cleanup(FdMigrationState *s)

 void migrate_fd_put_notify(void *opaque)
 {
-    FdMigrationState *s = opaque;
+    MigrationState *s = opaque;

     qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
     qemu_file_put_notify(s->file);
@@ -318,7 +318,7 @@ void migrate_fd_put_notify(void *opaque)

 ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size)
 {
-    FdMigrationState *s = opaque;
+    MigrationState *s = opaque;
     ssize_t ret;

     do {
@@ -341,7 +341,7 @@ ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size)
     return ret;
 }

-void migrate_fd_connect(FdMigrationState *s)
+void migrate_fd_connect(MigrationState *s)
 {
     int ret;

@@ -365,7 +365,7 @@ void migrate_fd_connect(FdMigrationState *s)

 void migrate_fd_put_ready(void *opaque)
 {
-    FdMigrationState *s = opaque;
+    MigrationState *s = opaque;

     if (s->state != MIG_STATE_ACTIVE) {
         DPRINTF("put_ready returning because of non-active state\n");
@@ -399,12 +399,12 @@ void migrate_fd_put_ready(void *opaque)
     }
 }

-int migrate_fd_get_status(FdMigrationState *s)
+int migrate_fd_get_status(MigrationState *s)
 {
     return s->state;
 }

-void migrate_fd_cancel(FdMigrationState *s)
+void migrate_fd_cancel(MigrationState *s)
 {
     if (s->state != MIG_STATE_ACTIVE)
         return;
@@ -418,7 +418,7 @@ void migrate_fd_cancel(FdMigrationState *s)
     migrate_fd_cleanup(s);
 }

-void migrate_fd_release(FdMigrationState *s)
+void migrate_fd_release(MigrationState *s)
 {

     DPRINTF("releasing state\n");
@@ -433,7 +433,7 @@ void migrate_fd_release(FdMigrationState *s)

 void migrate_fd_wait_for_unfreeze(void *opaque)
 {
-    FdMigrationState *s = opaque;
+    MigrationState *s = opaque;
     int ret;

     DPRINTF("wait for unfreeze\n");
@@ -452,7 +452,7 @@ void migrate_fd_wait_for_unfreeze(void *opaque)

 int migrate_fd_close(void *opaque)
 {
-    FdMigrationState *s = opaque;
+    MigrationState *s = opaque;

     qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
     return s->close(s);
diff --git a/migration.h b/migration.h
index 93441e4..7b34d16 100644
--- a/migration.h
+++ b/migration.h
@@ -23,21 +23,21 @@
 #define MIG_STATE_CANCELLED	1
 #define MIG_STATE_ACTIVE	2

-typedef struct FdMigrationState FdMigrationState;
+typedef struct MigrationState MigrationState;

-struct FdMigrationState
+struct MigrationState
 {
     int64_t bandwidth_limit;
     QEMUFile *file;
     int fd;
     Monitor *mon;
     int state;
-    int (*get_error)(struct FdMigrationState*);
-    int (*close)(struct FdMigrationState*);
-    int (*write)(struct FdMigrationState*, const void *, size_t);
-    void (*cancel)(FdMigrationState *s);
-    int (*get_status)(FdMigrationState *s);
-    void (*release)(FdMigrationState *s);
+    int (*get_error)(MigrationState*);
+    int (*close)(MigrationState*);
+    int (*write)(MigrationState*, const void *, size_t);
+    void (*cancel)(MigrationState *s);
+    int (*get_status)(MigrationState *s);
+    void (*release)(MigrationState *s);
     void *opaque;
     int blk;
     int shared;
@@ -64,7 +64,7 @@ void do_info_migrate(Monitor *mon, QObject **ret_data);

 int exec_start_incoming_migration(const char *host_port);

-FdMigrationState *exec_start_outgoing_migration(Monitor *mon,
+MigrationState *exec_start_outgoing_migration(Monitor *mon,
                                               const char *host_port,
 					      int64_t bandwidth_limit,
 					      int detach,
@@ -73,7 +73,7 @@ FdMigrationState *exec_start_outgoing_migration(Monitor *mon,

 int tcp_start_incoming_migration(const char *host_port);

-FdMigrationState *tcp_start_outgoing_migration(Monitor *mon,
+MigrationState *tcp_start_outgoing_migration(Monitor *mon,
                                              const char *host_port,
 					     int64_t bandwidth_limit,
 					     int detach,
@@ -82,7 +82,7 @@ FdMigrationState *tcp_start_outgoing_migration(Monitor *mon,

 int unix_start_incoming_migration(const char *path);

-FdMigrationState *unix_start_outgoing_migration(Monitor *mon,
+MigrationState *unix_start_outgoing_migration(Monitor *mon,
                                               const char *path,
 					      int64_t bandwidth_limit,
 					      int detach,
@@ -91,32 +91,32 @@ FdMigrationState *unix_start_outgoing_migration(Monitor *mon,

 int fd_start_incoming_migration(const char *path);

-FdMigrationState *fd_start_outgoing_migration(Monitor *mon,
+MigrationState *fd_start_outgoing_migration(Monitor *mon,
 					    const char *fdname,
 					    int64_t bandwidth_limit,
 					    int detach,
 					    int blk,
 					    int inc);

-void migrate_fd_monitor_suspend(FdMigrationState *s, Monitor *mon);
+void migrate_fd_monitor_suspend(MigrationState *s, Monitor *mon);

-void migrate_fd_error(FdMigrationState *s);
+void migrate_fd_error(MigrationState *s);

-int migrate_fd_cleanup(FdMigrationState *s);
+int migrate_fd_cleanup(MigrationState *s);

 void migrate_fd_put_notify(void *opaque);

 ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size);

-void migrate_fd_connect(FdMigrationState *s);
+void migrate_fd_connect(MigrationState *s);

 void migrate_fd_put_ready(void *opaque);

-int migrate_fd_get_status(FdMigrationState *mig_state);
+int migrate_fd_get_status(MigrationState *mig_state);

-void migrate_fd_cancel(FdMigrationState *mig_state);
+void migrate_fd_cancel(MigrationState *mig_state);

-void migrate_fd_release(FdMigrationState *mig_state);
+void migrate_fd_release(MigrationState *mig_state);

 void migrate_fd_wait_for_unfreeze(void *opaque);

-- 
1.7.4

[Qemu-devel] [PATCH 05/22] migration: Refactor MigrationState creation

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration-exec.c |   16 +---------------
 migration-fd.c   |   16 +---------------
 migration-tcp.c  |   15 +--------------
 migration-unix.c |   15 +--------------
 migration.c      |   29 +++++++++++++++++++++++++----
 migration.h      |   11 +++--------
 6 files changed, 32 insertions(+), 70 deletions(-)

diff --git a/migration-exec.c b/migration-exec.c
index e91e1e2..67dec27 100644
--- a/migration-exec.c
+++ b/migration-exec.c
@@ -72,7 +72,7 @@ MigrationState *exec_start_outgoing_migration(Monitor *mon,
     MigrationState *s;
     FILE *f;

-    s = qemu_mallocz(sizeof(*s));
+    s = migrate_create_state(mon, bandwidth_limit, detach, blk, inc);

     f = popen(command, "w");
     if (f == NULL) {
@@ -93,20 +93,6 @@ MigrationState *exec_start_outgoing_migration(Monitor *mon,
     s->close = exec_close;
     s->get_error = file_errno;
     s->write = file_write;
-    s->cancel = migrate_fd_cancel;
-    s->get_status = migrate_fd_get_status;
-    s->release = migrate_fd_release;
-
-    s->blk = blk;
-    s->shared = inc;
-
-    s->state = MIG_STATE_ACTIVE;
-    s->mon = NULL;
-    s->bandwidth_limit = bandwidth_limit;
-
-    if (!detach) {
-        migrate_fd_monitor_suspend(s, mon);
-    }

     migrate_fd_connect(s);
     return s;
diff --git a/migration-fd.c b/migration-fd.c
index 4073000..1deedd7 100644
--- a/migration-fd.c
+++ b/migration-fd.c
@@ -60,7 +60,7 @@ MigrationState *fd_start_outgoing_migration(Monitor *mon,
 {
     MigrationState *s;

-    s = qemu_mallocz(sizeof(*s));
+    s = migrate_create_state(mon, bandwidth_limit, detach, blk, inc);

     s->fd = monitor_get_fd(mon, fdname);
     if (s->fd == -1) {
@@ -76,20 +76,6 @@ MigrationState *fd_start_outgoing_migration(Monitor *mon,
     s->get_error = fd_errno;
     s->write = fd_write;
     s->close = fd_close;
-    s->cancel = migrate_fd_cancel;
-    s->get_status = migrate_fd_get_status;
-    s->release = migrate_fd_release;
-
-    s->blk = blk;
-    s->shared = inc;
-
-    s->state = MIG_STATE_ACTIVE;
-    s->mon = NULL;
-    s->bandwidth_limit = bandwidth_limit;
-
-    if (!detach) {
-        migrate_fd_monitor_suspend(s, mon);
-    }

     migrate_fd_connect(s);
     return s;
diff --git a/migration-tcp.c b/migration-tcp.c
index e8fa46c..c1c7bc3 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -90,21 +90,12 @@ MigrationState *tcp_start_outgoing_migration(Monitor *mon,
     if (parse_host_port(&addr, host_port) < 0)
         return NULL;

-    s = qemu_mallocz(sizeof(*s));
+    s = migrate_create_state(mon, bandwidth_limit, detach, blk, inc);

     s->get_error = socket_errno;
     s->write = socket_write;
     s->close = tcp_close;
-    s->cancel = migrate_fd_cancel;
-    s->get_status = migrate_fd_get_status;
-    s->release = migrate_fd_release;

-    s->blk = blk;
-    s->shared = inc;
-
-    s->state = MIG_STATE_ACTIVE;
-    s->mon = NULL;
-    s->bandwidth_limit = bandwidth_limit;
     s->fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (s->fd == -1) {
         qemu_free(s);
@@ -113,10 +104,6 @@ MigrationState *tcp_start_outgoing_migration(Monitor *mon,

     socket_set_nonblock(s->fd);

-    if (!detach) {
-        migrate_fd_monitor_suspend(s, mon);
-    }
-
     do {
         ret = connect(s->fd, (struct sockaddr *)&addr, sizeof(addr));
         if (ret == -1)
diff --git a/migration-unix.c b/migration-unix.c
index 9b74401..94995af 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -89,21 +89,12 @@ MigrationState *unix_start_outgoing_migration(Monitor *mon,
     addr.sun_family = AF_UNIX;
     snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", path);

-    s = qemu_mallocz(sizeof(*s));
+    s = migrate_create_state(mon, bandwidth_limit, detach, blk, inc);

     s->get_error = unix_errno;
     s->write = unix_write;
     s->close = unix_close;
-    s->cancel = migrate_fd_cancel;
-    s->get_status = migrate_fd_get_status;
-    s->release = migrate_fd_release;

-    s->blk = blk;
-    s->shared = inc;
-
-    s->state = MIG_STATE_ACTIVE;
-    s->mon = NULL;
-    s->bandwidth_limit = bandwidth_limit;
     s->fd = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
     if (s->fd < 0) {
         DPRINTF("Unable to open socket");
@@ -126,10 +117,6 @@ MigrationState *unix_start_outgoing_migration(Monitor *mon,
         goto err_after_open;
     }

-    if (!detach) {
-        migrate_fd_monitor_suspend(s, mon);
-    }
-
     if (ret >= 0)
         migrate_fd_connect(s);

diff --git a/migration.c b/migration.c
index 9ad45d6..e773806 100644
--- a/migration.c
+++ b/migration.c
@@ -262,7 +262,7 @@ void do_info_migrate(Monitor *mon, QObject **ret_data)

 /* shared migration helpers */

-void migrate_fd_monitor_suspend(MigrationState *s, Monitor *mon)
+static void migrate_fd_monitor_suspend(MigrationState *s, Monitor *mon)
 {
     s->mon = mon;
     if (monitor_suspend(mon) == 0) {
@@ -399,12 +399,12 @@ void migrate_fd_put_ready(void *opaque)
     }
 }

-int migrate_fd_get_status(MigrationState *s)
+static int migrate_fd_get_status(MigrationState *s)
 {
     return s->state;
 }

-void migrate_fd_cancel(MigrationState *s)
+static void migrate_fd_cancel(MigrationState *s)
 {
     if (s->state != MIG_STATE_ACTIVE)
         return;
@@ -418,7 +418,7 @@ void migrate_fd_cancel(MigrationState *s)
     migrate_fd_cleanup(s);
 }

-void migrate_fd_release(MigrationState *s)
+static void migrate_fd_release(MigrationState *s)
 {

     DPRINTF("releasing state\n");
@@ -476,3 +476,24 @@ int get_migration_state(void)
         return MIG_STATE_ERROR;
     }
 }
+
+MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limit,
+                                     int detach, int blk, int inc)
+{
+    MigrationState *s = qemu_mallocz(sizeof(*s));
+
+    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;
+    s->bandwidth_limit = bandwidth_limit;
+    s->state = MIG_STATE_ACTIVE;
+
+    if (!detach) {
+        migrate_fd_monitor_suspend(s, mon);
+    }
+
+    return s;
+}
diff --git a/migration.h b/migration.h
index 7b34d16..0178414 100644
--- a/migration.h
+++ b/migration.h
@@ -98,8 +98,6 @@ MigrationState *fd_start_outgoing_migration(Monitor *mon,
 					    int blk,
 					    int inc);

-void migrate_fd_monitor_suspend(MigrationState *s, Monitor *mon);
-
 void migrate_fd_error(MigrationState *s);

 int migrate_fd_cleanup(MigrationState *s);
@@ -112,16 +110,13 @@ void migrate_fd_connect(MigrationState *s);

 void migrate_fd_put_ready(void *opaque);

-int migrate_fd_get_status(MigrationState *mig_state);
-
-void migrate_fd_cancel(MigrationState *mig_state);
-
-void migrate_fd_release(MigrationState *mig_state);
-
 void migrate_fd_wait_for_unfreeze(void *opaque);

 int migrate_fd_close(void *opaque);

+MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limit,
+                                     int detach, int blk, int inc);
+
 void add_migration_state_change_notifier(Notifier *notify);
 void remove_migration_state_change_notifier(Notifier *notify);
 int get_migration_state(void);
-- 
1.7.4

[Qemu-devel] [PATCH 06/22] migration: Make all posible migration functions static

[Juan Quintela]

I have to move two functions postions to avoid forward declarations

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |   72 +++++++++++++++++++++++++++++-----------------------------
 migration.h |   12 ---------
 2 files changed, 36 insertions(+), 48 deletions(-)

diff --git a/migration.c b/migration.c
index e773806..1853380 100644
--- a/migration.c
+++ b/migration.c
@@ -273,15 +273,7 @@ static void migrate_fd_monitor_suspend(MigrationState *s, Monitor *mon)
     }
 }

-void migrate_fd_error(MigrationState *s)
-{
-    DPRINTF("setting error state\n");
-    s->state = MIG_STATE_ERROR;
-    notifier_list_notify(&migration_state_notifiers);
-    migrate_fd_cleanup(s);
-}
-
-int migrate_fd_cleanup(MigrationState *s)
+static int migrate_fd_cleanup(MigrationState *s)
 {
     int ret = 0;

@@ -308,7 +300,15 @@ int migrate_fd_cleanup(MigrationState *s)
     return ret;
 }

-void migrate_fd_put_notify(void *opaque)
+void migrate_fd_error(MigrationState *s)
+{
+    DPRINTF("setting error state\n");
+    s->state = MIG_STATE_ERROR;
+    notifier_list_notify(&migration_state_notifiers);
+    migrate_fd_cleanup(s);
+}
+
+static void migrate_fd_put_notify(void *opaque)
 {
     MigrationState *s = opaque;

@@ -316,7 +316,7 @@ void migrate_fd_put_notify(void *opaque)
     qemu_file_put_notify(s->file);
 }

-ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size)
+static ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size)
 {
     MigrationState *s = opaque;
     ssize_t ret;
@@ -341,29 +341,7 @@ ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size)
     return ret;
 }

-void migrate_fd_connect(MigrationState *s)
-{
-    int ret;
-
-    s->file = qemu_fopen_ops_buffered(s,
-                                      s->bandwidth_limit,
-                                      migrate_fd_put_buffer,
-                                      migrate_fd_put_ready,
-                                      migrate_fd_wait_for_unfreeze,
-                                      migrate_fd_close);
-
-    DPRINTF("beginning savevm\n");
-    ret = qemu_savevm_state_begin(s->mon, s->file, s->blk, s->shared);
-    if (ret < 0) {
-        DPRINTF("failed, %d\n", ret);
-        migrate_fd_error(s);
-        return;
-    }
-    
-    migrate_fd_put_ready(s);
-}
-
-void migrate_fd_put_ready(void *opaque)
+static void migrate_fd_put_ready(void *opaque)
 {
     MigrationState *s = opaque;

@@ -431,7 +409,7 @@ static void migrate_fd_release(MigrationState *s)
     qemu_free(s);
 }

-void migrate_fd_wait_for_unfreeze(void *opaque)
+static void migrate_fd_wait_for_unfreeze(void *opaque)
 {
     MigrationState *s = opaque;
     int ret;
@@ -450,7 +428,7 @@ void migrate_fd_wait_for_unfreeze(void *opaque)
     } while (ret == -1 && (s->get_error(s)) == EINTR);
 }

-int migrate_fd_close(void *opaque)
+static int migrate_fd_close(void *opaque)
 {
     MigrationState *s = opaque;

@@ -477,6 +455,28 @@ int get_migration_state(void)
     }
 }

+void migrate_fd_connect(MigrationState *s)
+{
+    int ret;
+
+    s->file = qemu_fopen_ops_buffered(s,
+                                      s->bandwidth_limit,
+                                      migrate_fd_put_buffer,
+                                      migrate_fd_put_ready,
+                                      migrate_fd_wait_for_unfreeze,
+                                      migrate_fd_close);
+
+    DPRINTF("beginning savevm\n");
+    ret = qemu_savevm_state_begin(s->mon, s->file, s->blk, s->shared);
+    if (ret < 0) {
+        DPRINTF("failed, %d\n", ret);
+        migrate_fd_error(s);
+        return;
+    }
+
+    migrate_fd_put_ready(s);
+}
+
 MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limit,
                                      int detach, int blk, int inc)
 {
diff --git a/migration.h b/migration.h
index 0178414..048ee46 100644
--- a/migration.h
+++ b/migration.h
@@ -100,20 +100,8 @@ MigrationState *fd_start_outgoing_migration(Monitor *mon,

 void migrate_fd_error(MigrationState *s);

-int migrate_fd_cleanup(MigrationState *s);
-
-void migrate_fd_put_notify(void *opaque);
-
-ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size);
-
 void migrate_fd_connect(MigrationState *s);

-void migrate_fd_put_ready(void *opaque);
-
-void migrate_fd_wait_for_unfreeze(void *opaque);
-
-int migrate_fd_close(void *opaque);
-
 MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limit,
                                      int detach, int blk, int inc);

-- 
1.7.4

[Qemu-devel] [PATCH 07/22] migration: move migrate_create_state to do_migrate

[Juan Quintela]

Once there, remove all parameters that don't need to be passed to
*start_outgoing_migration() functions

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration-exec.c |   19 +++++--------------
 migration-fd.c   |   22 ++++++----------------
 migration-tcp.c  |   20 ++++++--------------
 migration-unix.c |   19 +++++--------------
 migration.c      |   32 +++++++++++++++++++-------------
 migration.h      |   31 ++++---------------------------
 6 files changed, 45 insertions(+), 98 deletions(-)

diff --git a/migration-exec.c b/migration-exec.c
index 67dec27..ed0798b 100644
--- a/migration-exec.c
+++ b/migration-exec.c
@@ -62,22 +62,14 @@ static int exec_close(MigrationState *s)
     return ret;
 }

-MigrationState *exec_start_outgoing_migration(Monitor *mon,
-                                              const char *command,
-					      int64_t bandwidth_limit,
-					      int detach,
-					      int blk,
-					      int inc)
+int exec_start_outgoing_migration(MigrationState *s, const char *command)
 {
-    MigrationState *s;
     FILE *f;

-    s = migrate_create_state(mon, bandwidth_limit, detach, blk, inc);
-
     f = popen(command, "w");
     if (f == NULL) {
         DPRINTF("Unable to popen exec target\n");
-        goto err_after_alloc;
+        goto err_after_popen;
     }

     s->fd = fileno(f);
@@ -95,13 +87,12 @@ MigrationState *exec_start_outgoing_migration(Monitor *mon,
     s->write = file_write;

     migrate_fd_connect(s);
-    return s;
+    return 0;

 err_after_open:
     pclose(f);
-err_after_alloc:
-    qemu_free(s);
-    return NULL;
+err_after_popen:
+    return -1;
 }

 static void exec_accept_incoming_migration(void *opaque)
diff --git a/migration-fd.c b/migration-fd.c
index 1deedd7..e2dd61f 100644
--- a/migration-fd.c
+++ b/migration-fd.c
@@ -51,21 +51,12 @@ static int fd_close(MigrationState *s)
     return 0;
 }

-MigrationState *fd_start_outgoing_migration(Monitor *mon,
-					    const char *fdname,
-					    int64_t bandwidth_limit,
-					    int detach,
-					    int blk,
-					    int inc)
+int fd_start_outgoing_migration(MigrationState *s, const char *fdname)
 {
-    MigrationState *s;
-
-    s = migrate_create_state(mon, bandwidth_limit, detach, blk, inc);
-
-    s->fd = monitor_get_fd(mon, fdname);
+    s->fd = monitor_get_fd(s->mon, fdname);
     if (s->fd == -1) {
         DPRINTF("fd_migration: invalid file descriptor identifier\n");
-        goto err_after_alloc;
+        goto err_after_get_fd;
     }

     if (fcntl(s->fd, F_SETFL, O_NONBLOCK) == -1) {
@@ -78,13 +69,12 @@ MigrationState *fd_start_outgoing_migration(Monitor *mon,
     s->close = fd_close;

     migrate_fd_connect(s);
-    return s;
+    return 0;

 err_after_open:
     close(s->fd);
-err_after_alloc:
-    qemu_free(s);
-    return NULL;
+err_after_get_fd:
+    return -1;
 }

 static void fd_accept_incoming_migration(void *opaque)
diff --git a/migration-tcp.c b/migration-tcp.c
index c1c7bc3..ac69681 100644
--- a/migration-tcp.c
+++ b/migration-tcp.c
@@ -76,21 +76,14 @@ static void tcp_wait_for_connect(void *opaque)
     }
 }

-MigrationState *tcp_start_outgoing_migration(Monitor *mon,
-                                             const char *host_port,
-                                             int64_t bandwidth_limit,
-                                             int detach,
-					     int blk,
-					     int inc)
+int tcp_start_outgoing_migration(MigrationState *s, const char *host_port)
 {
     struct sockaddr_in addr;
-    MigrationState *s;
     int ret;

-    if (parse_host_port(&addr, host_port) < 0)
-        return NULL;
-
-    s = migrate_create_state(mon, bandwidth_limit, detach, blk, inc);
+    ret = parse_host_port(&addr, host_port);
+    if (ret < 0)
+        return ret;

     s->get_error = socket_errno;
     s->write = socket_write;
@@ -98,8 +91,7 @@ MigrationState *tcp_start_outgoing_migration(Monitor *mon,

     s->fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (s->fd == -1) {
-        qemu_free(s);
-        return NULL;
+        return -1;
     }

     socket_set_nonblock(s->fd);
@@ -119,7 +111,7 @@ MigrationState *tcp_start_outgoing_migration(Monitor *mon,
     } else if (ret >= 0)
         migrate_fd_connect(s);

-    return s;
+    return 0;
 }

 static void tcp_accept_incoming_migration(void *opaque)
diff --git a/migration-unix.c b/migration-unix.c
index 94995af..d562456 100644
--- a/migration-unix.c
+++ b/migration-unix.c
@@ -75,22 +75,14 @@ static void unix_wait_for_connect(void *opaque)
     }
 }

-MigrationState *unix_start_outgoing_migration(Monitor *mon,
-                                              const char *path,
-					      int64_t bandwidth_limit,
-					      int detach,
-					      int blk,
-					      int inc)
+int unix_start_outgoing_migration(MigrationState *s, const char *path)
 {
-    MigrationState *s;
     struct sockaddr_un addr;
     int ret;

     addr.sun_family = AF_UNIX;
     snprintf(addr.sun_path, sizeof(addr.sun_path), "%s", path);

-    s = migrate_create_state(mon, bandwidth_limit, detach, blk, inc);
-
     s->get_error = unix_errno;
     s->write = unix_write;
     s->close = unix_close;
@@ -98,7 +90,7 @@ MigrationState *unix_start_outgoing_migration(Monitor *mon,
     s->fd = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
     if (s->fd < 0) {
         DPRINTF("Unable to open socket");
-        goto err_after_alloc;
+        goto err_after_socket;
     }

     socket_set_nonblock(s->fd);
@@ -120,14 +112,13 @@ MigrationState *unix_start_outgoing_migration(Monitor *mon,
     if (ret >= 0)
         migrate_fd_connect(s);

-    return s;
+    return 0;

 err_after_open:
     close(s->fd);

-err_after_alloc:
-    qemu_free(s);
-    return NULL;
+err_after_socket:
+    return -1;
 }

 static void unix_accept_incoming_migration(void *opaque)
diff --git a/migration.c b/migration.c
index 1853380..397a0b9 100644
--- a/migration.c
+++ b/migration.c
@@ -76,6 +76,9 @@ void process_incoming_migration(QEMUFile *f)
         vm_start();
 }

+static MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limit,
+                                            int detach, int blk, int inc);
+
 int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     MigrationState *s = NULL;
@@ -84,6 +87,7 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
     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");
+    int ret;

     if (current_migration &&
         current_migration->get_status(current_migration) == MIG_STATE_ACTIVE) {
@@ -95,28 +99,27 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
         return -1;
     }

+    s = migrate_create_state(mon, max_throttle, detach, blk, inc);
+
     if (strstart(uri, "tcp:", &p)) {
-        s = tcp_start_outgoing_migration(mon, p, max_throttle, detach,
-                                         blk, inc);
+        ret = tcp_start_outgoing_migration(s, p);
 #if !defined(WIN32)
     } else if (strstart(uri, "exec:", &p)) {
-        s = exec_start_outgoing_migration(mon, p, max_throttle, detach,
-                                          blk, inc);
+        ret = exec_start_outgoing_migration(s, p);
     } else if (strstart(uri, "unix:", &p)) {
-        s = unix_start_outgoing_migration(mon, p, max_throttle, detach,
-                                          blk, inc);
+        ret = unix_start_outgoing_migration(s, p);
     } else if (strstart(uri, "fd:", &p)) {
-        s = fd_start_outgoing_migration(mon, p, max_throttle, detach, 
-                                        blk, inc);
+        ret = fd_start_outgoing_migration(s, p);
 #endif
     } else {
         monitor_printf(mon, "unknown migration protocol: %s\n", uri);
-        return -1;
+        ret  = -EINVAL;
+        goto free_migrate_state;
     }

-    if (s == NULL) {
+    if (ret < 0) {
         monitor_printf(mon, "migration failed\n");
-        return -1;
+        goto free_migrate_state;
     }

     if (current_migration) {
@@ -126,6 +129,9 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
     current_migration = s;
     notifier_list_notify(&migration_state_notifiers);
     return 0;
+free_migrate_state:
+    qemu_free(s);
+    return -1;
 }

 int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
@@ -477,8 +483,8 @@ void migrate_fd_connect(MigrationState *s)
     migrate_fd_put_ready(s);
 }

-MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limit,
-                                     int detach, int blk, int inc)
+static MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limit,
+                                            int detach, int blk, int inc)
 {
     MigrationState *s = qemu_mallocz(sizeof(*s));

diff --git a/migration.h b/migration.h
index 048ee46..7d28dd3 100644
--- a/migration.h
+++ b/migration.h
@@ -64,47 +64,24 @@ void do_info_migrate(Monitor *mon, QObject **ret_data);

 int exec_start_incoming_migration(const char *host_port);

-MigrationState *exec_start_outgoing_migration(Monitor *mon,
-                                              const char *host_port,
-					      int64_t bandwidth_limit,
-					      int detach,
-					      int blk,
-					      int inc);
+int exec_start_outgoing_migration(MigrationState *s, const char *host_port);

 int tcp_start_incoming_migration(const char *host_port);

-MigrationState *tcp_start_outgoing_migration(Monitor *mon,
-                                             const char *host_port,
-					     int64_t bandwidth_limit,
-					     int detach,
-					     int blk,
-					     int inc);
+int tcp_start_outgoing_migration(MigrationState *s, const char *host_port);

 int unix_start_incoming_migration(const char *path);

-MigrationState *unix_start_outgoing_migration(Monitor *mon,
-                                              const char *path,
-					      int64_t bandwidth_limit,
-					      int detach,
-					      int blk,
-					      int inc);
+int unix_start_outgoing_migration(MigrationState *s, const char *path);

 int fd_start_incoming_migration(const char *path);

-MigrationState *fd_start_outgoing_migration(Monitor *mon,
-					    const char *fdname,
-					    int64_t bandwidth_limit,
-					    int detach,
-					    int blk,
-					    int inc);
+int fd_start_outgoing_migration(MigrationState *s, const char *fdname);

 void migrate_fd_error(MigrationState *s);

 void migrate_fd_connect(MigrationState *s);

-MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limit,
-                                     int detach, int blk, int inc);
-
 void add_migration_state_change_notifier(Notifier *notify);
 void remove_migration_state_change_notifier(Notifier *notify);
 int get_migration_state(void);
-- 
1.7.4

[Qemu-devel] [PATCH 08/22] migration: Check that migration is active before cancel it

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/migration.c b/migration.c
index 397a0b9..55f58c8 100644
--- a/migration.c
+++ b/migration.c
@@ -138,7 +138,7 @@ int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     MigrationState *s = current_migration;

-    if (s)
+    if (s && s->get_status(s) == MIG_STATE_ACTIVE)
         s->cancel(s);

     return 0;
-- 
1.7.4

[Qemu-devel] [PATCH 09/22] migration: Introduce MIG_STATE_NONE

[Juan Quintela]

Use MIG_STATE_ACTIVE only when migration has really started

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |    6 +++++-
 migration.h |    3 ++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/migration.c b/migration.c
index 55f58c8..f015e02 100644
--- a/migration.c
+++ b/migration.c
@@ -238,6 +238,9 @@ void do_info_migrate(Monitor *mon, QObject **ret_data)
         MigrationState *s = current_migration;

         switch (s->get_status(current_migration)) {
+        case MIG_STATE_NONE:
+            /* no migration has happened ever */
+            break;
         case MIG_STATE_ACTIVE:
             qdict = qdict_new();
             qdict_put(qdict, "status", qstring_from_str("active"));
@@ -465,6 +468,7 @@ void migrate_fd_connect(MigrationState *s)
 {
     int ret;

+    s->state = MIG_STATE_ACTIVE;
     s->file = qemu_fopen_ops_buffered(s,
                                       s->bandwidth_limit,
                                       migrate_fd_put_buffer,
@@ -495,7 +499,7 @@ static MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limi
     s->shared = inc;
     s->mon = NULL;
     s->bandwidth_limit = bandwidth_limit;
-    s->state = MIG_STATE_ACTIVE;
+    s->state = MIG_STATE_NONE;

     if (!detach) {
         migrate_fd_monitor_suspend(s, mon);
diff --git a/migration.h b/migration.h
index 7d28dd3..3df2293 100644
--- a/migration.h
+++ b/migration.h
@@ -19,9 +19,10 @@
 #include "notify.h"

 #define MIG_STATE_ERROR		-1
-#define MIG_STATE_COMPLETED	0
+#define MIG_STATE_NONE		0
 #define MIG_STATE_CANCELLED	1
 #define MIG_STATE_ACTIVE	2
+#define MIG_STATE_COMPLETED	3

 typedef struct MigrationState MigrationState;

-- 
1.7.4

[Qemu-devel] [PATCH 10/22] migration: Refactor and simplify error checking in migrate_fd_put_ready

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |   20 +++++++++-----------
 1 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/migration.c b/migration.c
index f015e02..641df9f 100644
--- a/migration.c
+++ b/migration.c
@@ -361,28 +361,26 @@ static void migrate_fd_put_ready(void *opaque)

     DPRINTF("iterate\n");
     if (qemu_savevm_state_iterate(s->mon, s->file) == 1) {
-        int state;
         int old_vm_running = vm_running;

         DPRINTF("done iterating\n");
         vm_stop(VMSTOP_MIGRATE);

-        if ((qemu_savevm_state_complete(s->mon, s->file)) < 0) {
-            if (old_vm_running) {
-                vm_start();
-            }
-            state = MIG_STATE_ERROR;
+        if (qemu_savevm_state_complete(s->mon, s->file) < 0) {
+            migrate_fd_error(s);
         } else {
-            state = MIG_STATE_COMPLETED;
+            if (migrate_fd_cleanup(s) < 0) {
+                migrate_fd_error(s);
+            } else {
+                s->state = MIG_STATE_COMPLETED;
+                notifier_list_notify(&migration_state_notifiers);
+            }
         }
-        if (migrate_fd_cleanup(s) < 0) {
+        if (s->get_status(s) != MIG_STATE_COMPLETED) {
             if (old_vm_running) {
                 vm_start();
             }
-            state = MIG_STATE_ERROR;
         }
-        s->state = state;
-        notifier_list_notify(&migration_state_notifiers);
     }
 }

-- 
1.7.4

[Qemu-devel] [PATCH 11/22] migration: Introduce migrate_fd_completed() for consistenncy

[Juan Quintela]

This function is a bit different of the others that change the state,
in the sense that if migrate_fd_cleanup() returns an error, it set the
status to error, not completed.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/migration.c b/migration.c
index 641df9f..ab98664 100644
--- a/migration.c
+++ b/migration.c
@@ -317,6 +317,17 @@ void migrate_fd_error(MigrationState *s)
     migrate_fd_cleanup(s);
 }

+static void migrate_fd_completed(MigrationState *s)
+{
+    DPRINTF("setting completed state\n");
+    if (migrate_fd_cleanup(s) < 0) {
+        s->state = MIG_STATE_ERROR;
+    } else {
+        s->state = MIG_STATE_COMPLETED;
+    }
+    notifier_list_notify(&migration_state_notifiers);
+}
+
 static void migrate_fd_put_notify(void *opaque)
 {
     MigrationState *s = opaque;
@@ -369,12 +380,7 @@ static void migrate_fd_put_ready(void *opaque)
         if (qemu_savevm_state_complete(s->mon, s->file) < 0) {
             migrate_fd_error(s);
         } else {
-            if (migrate_fd_cleanup(s) < 0) {
-                migrate_fd_error(s);
-            } else {
-                s->state = MIG_STATE_COMPLETED;
-                notifier_list_notify(&migration_state_notifiers);
-            }
+            migrate_fd_completed(s);
         }
         if (s->get_status(s) != MIG_STATE_COMPLETED) {
             if (old_vm_running) {
-- 
1.7.4

[Qemu-devel] [PATCH 12/22] migration: Use migrate_fd_error() in last place that set status to ERROR

[Juan Quintela]

We are also calling to migrate_fd_cleanup(), but notice that it is the
right thing to do.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/migration.c b/migration.c
index ab98664..3983257 100644
--- a/migration.c
+++ b/migration.c
@@ -351,11 +351,7 @@ static ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size
     if (ret == -EAGAIN) {
         qemu_set_fd_handler2(s->fd, NULL, NULL, migrate_fd_put_notify, s);
     } else if (ret < 0) {
-        if (s->mon) {
-            monitor_resume(s->mon);
-        }
-        s->state = MIG_STATE_ERROR;
-        notifier_list_notify(&migration_state_notifiers);
+        migrate_fd_error(s);
     }

     return ret;
-- 
1.7.4

[Qemu-devel] [PATCH 13/22] migration: Our release callback was just free

[Juan Quintela]

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 3983257..dfe6a96 100644
--- a/migration.c
+++ b/migration.c
@@ -122,10 +122,7 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
         goto free_migrate_state;
     }

-    if (current_migration) {
-        current_migration->release(current_migration);
-    }
-
+    qemu_free(current_migration);
     current_migration = s;
     notifier_list_notify(&migration_state_notifiers);
     return 0;
@@ -405,19 +402,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) {
-        s->state = MIG_STATE_CANCELLED;
-        notifier_list_notify(&migration_state_notifiers);
-        migrate_fd_cleanup(s);
-    }
-    qemu_free(s);
-}
-
 static void migrate_fd_wait_for_unfreeze(void *opaque)
 {
     MigrationState *s = opaque;
@@ -494,7 +478,6 @@ static MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limi

     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 3df2293..5455d8b 100644
--- a/migration.h
+++ b/migration.h
@@ -38,7 +38,6 @@ struct MigrationState
     int (*write)(MigrationState*, const void *, size_t);
     void (*cancel)(MigrationState *s);
     int (*get_status)(MigrationState *s);
-    void (*release)(MigrationState *s);
     void *opaque;
     int blk;
     int shared;
-- 
1.7.4

[Qemu-devel] [PATCH 14/22] migration: Remove get_status() accessor

[Juan Quintela]

It is only used inside migration.c, and fields on that struct are
accessed all around the place on that file.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |   16 +++++-----------
 migration.h |    1 -
 2 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/migration.c b/migration.c
index dfe6a96..2b873fa 100644
--- a/migration.c
+++ b/migration.c
@@ -90,7 +90,7 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
     int ret;

     if (current_migration &&
-        current_migration->get_status(current_migration) == MIG_STATE_ACTIVE) {
+        current_migration->state == MIG_STATE_ACTIVE) {
         monitor_printf(mon, "migration already in progress\n");
         return -1;
     }
@@ -135,7 +135,7 @@ int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     MigrationState *s = current_migration;

-    if (s && s->get_status(s) == MIG_STATE_ACTIVE)
+    if (s && s->state == MIG_STATE_ACTIVE)
         s->cancel(s);

     return 0;
@@ -234,7 +234,7 @@ void do_info_migrate(Monitor *mon, QObject **ret_data)
     if (current_migration) {
         MigrationState *s = current_migration;

-        switch (s->get_status(current_migration)) {
+        switch (s->state) {
         case MIG_STATE_NONE:
             /* no migration has happened ever */
             break;
@@ -375,7 +375,7 @@ static void migrate_fd_put_ready(void *opaque)
         } else {
             migrate_fd_completed(s);
         }
-        if (s->get_status(s) != MIG_STATE_COMPLETED) {
+        if (s->state != MIG_STATE_COMPLETED) {
             if (old_vm_running) {
                 vm_start();
             }
@@ -383,11 +383,6 @@ static void migrate_fd_put_ready(void *opaque)
     }
 }

-static int migrate_fd_get_status(MigrationState *s)
-{
-    return s->state;
-}
-
 static void migrate_fd_cancel(MigrationState *s)
 {
     if (s->state != MIG_STATE_ACTIVE)
@@ -442,7 +437,7 @@ void remove_migration_state_change_notifier(Notifier *notify)
 int get_migration_state(void)
 {
     if (current_migration) {
-        return migrate_fd_get_status(current_migration);
+        return current_migration->state;
     } else {
         return MIG_STATE_ERROR;
     }
@@ -477,7 +472,6 @@ static MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limi
     MigrationState *s = qemu_mallocz(sizeof(*s));

     s->cancel = migrate_fd_cancel;
-    s->get_status = migrate_fd_get_status;
     s->blk = blk;
     s->shared = inc;
     s->mon = NULL;
diff --git a/migration.h b/migration.h
index 5455d8b..58a6e06 100644
--- a/migration.h
+++ b/migration.h
@@ -37,7 +37,6 @@ struct MigrationState
     int (*close)(MigrationState*);
     int (*write)(MigrationState*, const void *, size_t);
     void (*cancel)(MigrationState *s);
-    int (*get_status)(MigrationState *s);
     void *opaque;
     int blk;
     int shared;
-- 
1.7.4

[Qemu-devel] [PATCH 15/22] migration: Remove migration cancel() callback

[Juan Quintela]

It is used only in one place

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |    9 ++++-----
 migration.h |    1 -
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/migration.c b/migration.c
index 2b873fa..92bff01 100644
--- a/migration.c
+++ b/migration.c
@@ -131,12 +131,12 @@ free_migrate_state:
     return -1;
 }

+static void migrate_fd_cancel(MigrationState *s);
+
 int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
-    MigrationState *s = current_migration;
-
-    if (s && s->state == MIG_STATE_ACTIVE)
-        s->cancel(s);
+    if (current_migration)
+        migrate_fd_cancel(current_migration);

     return 0;
 }
@@ -471,7 +471,6 @@ static MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limi
 {
     MigrationState *s = qemu_mallocz(sizeof(*s));

-    s->cancel = migrate_fd_cancel;
     s->blk = blk;
     s->shared = inc;
     s->mon = NULL;
diff --git a/migration.h b/migration.h
index 58a6e06..6477b51 100644
--- a/migration.h
+++ b/migration.h
@@ -36,7 +36,6 @@ struct MigrationState
     int (*get_error)(MigrationState*);
     int (*close)(MigrationState*);
     int (*write)(MigrationState*, const void *, size_t);
-    void (*cancel)(MigrationState *s);
     void *opaque;
     int blk;
     int shared;
-- 
1.7.4

[Qemu-devel] [PATCH 16/22] migration: Move exported functions to the end of the file

[Juan Quintela]

This means we can remove the two forward declarations.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |  188 +++++++++++++++++++++++++++++------------------------------
 1 files changed, 92 insertions(+), 96 deletions(-)

diff --git a/migration.c b/migration.c
index 92bff01..d7dfe1e 100644
--- a/migration.c
+++ b/migration.c
@@ -76,90 +76,6 @@ void process_incoming_migration(QEMUFile *f)
         vm_start();
 }

-static MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limit,
-                                            int detach, int blk, int inc);
-
-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");
-    int ret;
-
-    if (current_migration &&
-        current_migration->state == MIG_STATE_ACTIVE) {
-        monitor_printf(mon, "migration already in progress\n");
-        return -1;
-    }
-
-    if (qemu_savevm_state_blocked(mon)) {
-        return -1;
-    }
-
-    s = migrate_create_state(mon, max_throttle, detach, blk, inc);
-
-    if (strstart(uri, "tcp:", &p)) {
-        ret = tcp_start_outgoing_migration(s, p);
-#if !defined(WIN32)
-    } else if (strstart(uri, "exec:", &p)) {
-        ret = exec_start_outgoing_migration(s, p);
-    } else if (strstart(uri, "unix:", &p)) {
-        ret = unix_start_outgoing_migration(s, p);
-    } else if (strstart(uri, "fd:", &p)) {
-        ret = fd_start_outgoing_migration(s, p);
-#endif
-    } else {
-        monitor_printf(mon, "unknown migration protocol: %s\n", uri);
-        ret  = -EINVAL;
-        goto free_migrate_state;
-    }
-
-    if (ret < 0) {
-        monitor_printf(mon, "migration failed\n");
-        goto free_migrate_state;
-    }
-
-    qemu_free(current_migration);
-    current_migration = s;
-    notifier_list_notify(&migration_state_notifiers);
-    return 0;
-free_migrate_state:
-    qemu_free(s);
-    return -1;
-}
-
-static void migrate_fd_cancel(MigrationState *s);
-
-int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
-{
-    if (current_migration)
-        migrate_fd_cancel(current_migration);
-
-    return 0;
-}
-
-int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
-{
-    int64_t d;
-    MigrationState *s;
-
-    d = qdict_get_int(qdict, "value");
-    if (d < 0) {
-        d = 0;
-    }
-    max_throttle = d;
-
-    s = current_migration;
-    if (s && s->file) {
-        qemu_file_set_rate_limit(s->file, max_throttle);
-    }
-
-    return 0;
-}
-
 /* amount of nanoseconds we are willing to wait for migration to be down.
  * the choice of nanoseconds is because it is the maximum resolution that
  * get_clock() can achieve. It is an internal measure. All user-visible
@@ -171,18 +87,6 @@ uint64_t migrate_max_downtime(void)
     return max_downtime;
 }

-int do_migrate_set_downtime(Monitor *mon, const QDict *qdict,
-                            QObject **ret_data)
-{
-    double d;
-
-    d = qdict_get_double(qdict, "value") * 1e9;
-    d = MAX(0, MIN(UINT64_MAX, d));
-    max_downtime = (uint64_t)d;
-
-    return 0;
-}
-
 static void migrate_print_status(Monitor *mon, const char *name,
                                  const QDict *status_dict)
 {
@@ -483,3 +387,95 @@ static MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limi

     return s;
 }
+
+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");
+    int ret;
+
+    if (current_migration &&
+        current_migration->state == MIG_STATE_ACTIVE) {
+        monitor_printf(mon, "migration already in progress\n");
+        return -1;
+    }
+
+    if (qemu_savevm_state_blocked(mon)) {
+        return -1;
+    }
+
+    s = migrate_create_state(mon, max_throttle, detach, blk, inc);
+
+    if (strstart(uri, "tcp:", &p)) {
+        ret = tcp_start_outgoing_migration(s, p);
+#if !defined(WIN32)
+    } else if (strstart(uri, "exec:", &p)) {
+        ret = exec_start_outgoing_migration(s, p);
+    } else if (strstart(uri, "unix:", &p)) {
+        ret = unix_start_outgoing_migration(s, p);
+    } else if (strstart(uri, "fd:", &p)) {
+        ret = fd_start_outgoing_migration(s, p);
+#endif
+    } else {
+        monitor_printf(mon, "unknown migration protocol: %s\n", uri);
+        ret  = -EINVAL;
+        goto free_migrate_state;
+    }
+
+    if (ret < 0) {
+        monitor_printf(mon, "migration failed\n");
+        goto free_migrate_state;
+    }
+
+    qemu_free(current_migration);
+    current_migration = s;
+    notifier_list_notify(&migration_state_notifiers);
+    return 0;
+free_migrate_state:
+    qemu_free(s);
+    return -1;
+}
+
+int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    if (current_migration)
+        migrate_fd_cancel(current_migration);
+
+    return 0;
+}
+
+int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    int64_t d;
+    MigrationState *s;
+
+    d = qdict_get_int(qdict, "value");
+    if (d < 0) {
+        d = 0;
+    }
+    max_throttle = d;
+
+    s = current_migration;
+    if (s && s->file) {
+        qemu_file_set_rate_limit(s->file, max_throttle);
+    }
+
+    return 0;
+}
+
+int do_migrate_set_downtime(Monitor *mon, const QDict *qdict,
+                            QObject **ret_data)
+{
+    double d;
+
+    d = qdict_get_double(qdict, "value") * 1e9;
+    d = MAX(0, MIN(UINT64_MAX, d));
+    max_downtime = (uint64_t)d;
+
+    return 0;
+}
+
-- 
1.7.4

[Qemu-devel] [PATCH 17/22] migration: use global variable directly

[Juan Quintela]

We are setting a pointer to a local variable in the previous line, just use
the global variable directly.  We remove the ->file test because it is already
done inside qemu_file_set_rate_limit() function.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/migration.c b/migration.c
index d7dfe1e..accc6e4 100644
--- a/migration.c
+++ b/migration.c
@@ -451,7 +451,6 @@ int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
 int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     int64_t d;
-    MigrationState *s;

     d = qdict_get_int(qdict, "value");
     if (d < 0) {
@@ -459,9 +458,8 @@ int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
     }
     max_throttle = d;

-    s = current_migration;
-    if (s && s->file) {
-        qemu_file_set_rate_limit(s->file, max_throttle);
+    if (current_migration) {
+        qemu_file_set_rate_limit(current_migration->file, max_throttle);
     }

     return 0;
-- 
1.7.4

[Qemu-devel] [PATCH 18/22] migration: another case of global variable assigned to local one

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/migration.c b/migration.c
index accc6e4..4014330 100644
--- a/migration.c
+++ b/migration.c
@@ -136,9 +136,8 @@ void do_info_migrate(Monitor *mon, QObject **ret_data)
     QDict *qdict;

     if (current_migration) {
-        MigrationState *s = current_migration;

-        switch (s->state) {
+        switch (current_migration->state) {
         case MIG_STATE_NONE:
             /* no migration has happened ever */
             break;
-- 
1.7.4

[Qemu-devel] [PATCH 19/22] migration: convert current_migration from pointer to struct

[Juan Quintela]

This cleans up a lot the code as we don't have to check anymore if
the variable is NULL or not.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |  119 ++++++++++++++++++++++++----------------------------------
 1 files changed, 49 insertions(+), 70 deletions(-)

diff --git a/migration.c b/migration.c
index 4014330..7b1e679 100644
--- a/migration.c
+++ b/migration.c
@@ -34,7 +34,9 @@
 /* Migration speed throttling */
 static int64_t max_throttle = (32 << 20);

-static MigrationState *current_migration;
+static MigrationState current_migration = {
+    .state = MIG_STATE_NONE,
+};

 static NotifierList migration_state_notifiers =
     NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
@@ -135,37 +137,34 @@ void do_info_migrate(Monitor *mon, QObject **ret_data)
 {
     QDict *qdict;

-    if (current_migration) {
-
-        switch (current_migration->state) {
-        case MIG_STATE_NONE:
-            /* no migration has happened ever */
-            break;
-        case MIG_STATE_ACTIVE:
-            qdict = qdict_new();
-            qdict_put(qdict, "status", qstring_from_str("active"));
-
-            migrate_put_status(qdict, "ram", ram_bytes_transferred(),
-                               ram_bytes_remaining(), ram_bytes_total());
-
-            if (blk_mig_active()) {
-                migrate_put_status(qdict, "disk", blk_mig_bytes_transferred(),
-                                   blk_mig_bytes_remaining(),
-                                   blk_mig_bytes_total());
-            }
-
-            *ret_data = QOBJECT(qdict);
-            break;
-        case MIG_STATE_COMPLETED:
-            *ret_data = qobject_from_jsonf("{ 'status': 'completed' }");
-            break;
-        case MIG_STATE_ERROR:
-            *ret_data = qobject_from_jsonf("{ 'status': 'failed' }");
-            break;
-        case MIG_STATE_CANCELLED:
-            *ret_data = qobject_from_jsonf("{ 'status': 'cancelled' }");
-            break;
+    switch (current_migration.state) {
+    case MIG_STATE_NONE:
+        /* no migration has happened ever */
+        break;
+    case MIG_STATE_ACTIVE:
+        qdict = qdict_new();
+        qdict_put(qdict, "status", qstring_from_str("active"));
+
+        migrate_put_status(qdict, "ram", ram_bytes_transferred(),
+                           ram_bytes_remaining(), ram_bytes_total());
+
+        if (blk_mig_active()) {
+            migrate_put_status(qdict, "disk", blk_mig_bytes_transferred(),
+                               blk_mig_bytes_remaining(),
+                               blk_mig_bytes_total());
         }
+
+        *ret_data = QOBJECT(qdict);
+        break;
+    case MIG_STATE_COMPLETED:
+        *ret_data = qobject_from_jsonf("{ 'status': 'completed' }");
+        break;
+    case MIG_STATE_ERROR:
+        *ret_data = qobject_from_jsonf("{ 'status': 'failed' }");
+        break;
+    case MIG_STATE_CANCELLED:
+        *ret_data = qobject_from_jsonf("{ 'status': 'cancelled' }");
+        break;
     }
 }

@@ -339,11 +338,7 @@ void remove_migration_state_change_notifier(Notifier *notify)

 int get_migration_state(void)
 {
-    if (current_migration) {
-        return current_migration->state;
-    } else {
-        return MIG_STATE_ERROR;
-    }
+    return current_migration.state;
 }

 void migrate_fd_connect(MigrationState *s)
@@ -369,27 +364,22 @@ void migrate_fd_connect(MigrationState *s)
     migrate_fd_put_ready(s);
 }

-static MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limit,
-                                            int detach, int blk, int inc)
+static void migrate_init_state(Monitor *mon, int64_t bandwidth_limit,
+                               int detach, int blk, int inc)
 {
-    MigrationState *s = qemu_mallocz(sizeof(*s));
-
-    s->blk = blk;
-    s->shared = inc;
-    s->mon = NULL;
-    s->bandwidth_limit = bandwidth_limit;
-    s->state = MIG_STATE_NONE;
+    current_migration.blk = blk;
+    current_migration.shared = inc;
+    current_migration.mon = NULL;
+    current_migration.bandwidth_limit = bandwidth_limit;
+    current_migration.state = MIG_STATE_NONE;

     if (!detach) {
-        migrate_fd_monitor_suspend(s, mon);
+        migrate_fd_monitor_suspend(&current_migration, mon);
     }
-
-    return s;
 }

 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);
@@ -397,8 +387,7 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
     const char *uri = qdict_get_str(qdict, "uri");
     int ret;

-    if (current_migration &&
-        current_migration->state == MIG_STATE_ACTIVE) {
+    if (current_migration.state == MIG_STATE_ACTIVE) {
         monitor_printf(mon, "migration already in progress\n");
         return -1;
     }
@@ -407,42 +396,35 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
         return -1;
     }

-    s = migrate_create_state(mon, max_throttle, detach, blk, inc);
+    migrate_init_state(mon, max_throttle, detach, blk, inc);

     if (strstart(uri, "tcp:", &p)) {
-        ret = tcp_start_outgoing_migration(s, p);
+        ret = tcp_start_outgoing_migration(&current_migration, p);
 #if !defined(WIN32)
     } else if (strstart(uri, "exec:", &p)) {
-        ret = exec_start_outgoing_migration(s, p);
+        ret = exec_start_outgoing_migration(&current_migration, p);
     } else if (strstart(uri, "unix:", &p)) {
-        ret = unix_start_outgoing_migration(s, p);
+        ret = unix_start_outgoing_migration(&current_migration, p);
     } else if (strstart(uri, "fd:", &p)) {
-        ret = fd_start_outgoing_migration(s, p);
+        ret = fd_start_outgoing_migration(&current_migration, p);
 #endif
     } else {
         monitor_printf(mon, "unknown migration protocol: %s\n", uri);
-        ret  = -EINVAL;
-        goto free_migrate_state;
+        return -EINVAL;
     }

     if (ret < 0) {
         monitor_printf(mon, "migration failed\n");
-        goto free_migrate_state;
+        return ret;
     }

-    qemu_free(current_migration);
-    current_migration = s;
     notifier_list_notify(&migration_state_notifiers);
     return 0;
-free_migrate_state:
-    qemu_free(s);
-    return -1;
 }

 int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
-    if (current_migration)
-        migrate_fd_cancel(current_migration);
+    migrate_fd_cancel(&current_migration);

     return 0;
 }
@@ -457,10 +439,7 @@ int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
     }
     max_throttle = d;

-    if (current_migration) {
-        qemu_file_set_rate_limit(current_migration->file, max_throttle);
-    }
-
+    qemu_file_set_rate_limit(current_migration.file, max_throttle);
     return 0;
 }

-- 
1.7.4

[Qemu-devel] [PATCH 20/22] migration: Use bandwidth_limit directly

[Juan Quintela]

Now that current_migration is static, there is no reason for max_throotle
variable.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/migration.c b/migration.c
index 7b1e679..312a029 100644
--- a/migration.c
+++ b/migration.c
@@ -31,11 +31,10 @@
     do { } while (0)
 #endif

-/* Migration speed throttling */
-static int64_t max_throttle = (32 << 20);
-
 static MigrationState current_migration = {
     .state = MIG_STATE_NONE,
+     /* Migration speed throttling */
+    .bandwidth_limit = (32 << 20),
 };

 static NotifierList migration_state_notifiers =
@@ -364,13 +363,11 @@ void migrate_fd_connect(MigrationState *s)
     migrate_fd_put_ready(s);
 }

-static void migrate_init_state(Monitor *mon, int64_t bandwidth_limit,
-                               int detach, int blk, int inc)
+static void migrate_init_state(Monitor *mon, int detach, int blk, int inc)
 {
     current_migration.blk = blk;
     current_migration.shared = inc;
     current_migration.mon = NULL;
-    current_migration.bandwidth_limit = bandwidth_limit;
     current_migration.state = MIG_STATE_NONE;

     if (!detach) {
@@ -396,7 +393,7 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
         return -1;
     }

-    migrate_init_state(mon, max_throttle, detach, blk, inc);
+    migrate_init_state(mon, detach, blk, inc);

     if (strstart(uri, "tcp:", &p)) {
         ret = tcp_start_outgoing_migration(&current_migration, p);
@@ -437,9 +434,10 @@ int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
     if (d < 0) {
         d = 0;
     }
-    max_throttle = d;
+    current_migration.bandwidth_limit = d;

-    qemu_file_set_rate_limit(current_migration.file, max_throttle);
+    qemu_file_set_rate_limit(current_migration.file,
+                             current_migration.bandwidth_limit);
     return 0;
 }

-- 
1.7.4

[Qemu-devel] [PATCH 21/22] migration: Export a function that tells if the migration has finished correctly

[Juan Quintela]

This will allows us to hide the status values.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c     |    4 ++--
 migration.h     |    2 +-
 ui/spice-core.c |    4 +---
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/migration.c b/migration.c
index 312a029..383ebaf 100644
--- a/migration.c
+++ b/migration.c
@@ -335,9 +335,9 @@ void remove_migration_state_change_notifier(Notifier *notify)
     notifier_list_remove(&migration_state_notifiers, notify);
 }

-int get_migration_state(void)
+bool migration_has_finished(void)
 {
-    return current_migration.state;
+    return current_migration.state == MIG_STATE_COMPLETED;
 }

 void migrate_fd_connect(MigrationState *s)
diff --git a/migration.h b/migration.h
index 6477b51..9457807 100644
--- a/migration.h
+++ b/migration.h
@@ -82,6 +82,6 @@ void migrate_fd_connect(MigrationState *s);

 void add_migration_state_change_notifier(Notifier *notify);
 void remove_migration_state_change_notifier(Notifier *notify);
-int get_migration_state(void);
+bool migration_has_finished(void);

 #endif
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 1aa1a5e..997603d 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -422,9 +422,7 @@ void do_info_spice(Monitor *mon, QObject **ret_data)

 static void migration_state_notifier(Notifier *notifier)
 {
-    int state = get_migration_state();
-
-    if (state == MIG_STATE_COMPLETED) {
+    if (migration_has_finished()) {
 #if SPICE_SERVER_VERSION >= 0x000701 /* 0.7.1 */
         spice_server_migrate_switch(spice_server);
 #endif
-- 
1.7.4

[Qemu-devel] [PATCH 22/22] migration: Make state definitions local

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration.c |    6 ++++++
 migration.h |    6 ------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/migration.c b/migration.c
index 383ebaf..90fc2a0 100644
--- a/migration.c
+++ b/migration.c
@@ -31,6 +31,12 @@
     do { } while (0)
 #endif

+#define MIG_STATE_ERROR		-1
+#define MIG_STATE_NONE		0
+#define MIG_STATE_CANCELLED	1
+#define MIG_STATE_ACTIVE	2
+#define MIG_STATE_COMPLETED	3
+
 static MigrationState current_migration = {
     .state = MIG_STATE_NONE,
      /* Migration speed throttling */
diff --git a/migration.h b/migration.h
index 9457807..493fbe5 100644
--- a/migration.h
+++ b/migration.h
@@ -18,12 +18,6 @@
 #include "qemu-common.h"
 #include "notify.h"

-#define MIG_STATE_ERROR		-1
-#define MIG_STATE_NONE		0
-#define MIG_STATE_CANCELLED	1
-#define MIG_STATE_ACTIVE	2
-#define MIG_STATE_COMPLETED	3
-
 typedef struct MigrationState MigrationState;

 struct MigrationState
-- 
1.7.4

Re: [Qemu-devel] [PATCH 03/22] migration: Fold MigrationState into FdMigrationState

[Yoshiaki Tamura]

2011/2/23 Juan Quintela <quintela@redhat.com>:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration-exec.c |   10 +++++-----
>  migration-fd.c   |   10 +++++-----
>  migration-tcp.c  |   10 +++++-----
>  migration-unix.c |   10 +++++-----
>  migration.c      |   11 +++++------
>  migration.h      |   23 +++++------------------
>  6 files changed, 30 insertions(+), 44 deletions(-)
>
> diff --git a/migration-exec.c b/migration-exec.c
> index b49a475..02b0667 100644
> --- a/migration-exec.c
> +++ b/migration-exec.c
> @@ -93,12 +93,12 @@ FdMigrationState *exec_start_outgoing_migration(Monitor *mon,
>     s->close = exec_close;
>     s->get_error = file_errno;
>     s->write = file_write;
> -    s->mig_state.cancel = migrate_fd_cancel;
> -    s->mig_state.get_status = migrate_fd_get_status;
> -    s->mig_state.release = migrate_fd_release;
> +    s->cancel = migrate_fd_cancel;
> +    s->get_status = migrate_fd_get_status;
> +    s->release = migrate_fd_release;
>
> -    s->mig_state.blk = blk;
> -    s->mig_state.shared = inc;
> +    s->blk = blk;
> +    s->shared = inc;
>
>     s->state = MIG_STATE_ACTIVE;
>     s->mon = NULL;
> diff --git a/migration-fd.c b/migration-fd.c
> index bd5e8a9..ccba86b 100644
> --- a/migration-fd.c
> +++ b/migration-fd.c
> @@ -76,12 +76,12 @@ FdMigrationState *fd_start_outgoing_migration(Monitor *mon,
>     s->get_error = fd_errno;
>     s->write = fd_write;
>     s->close = fd_close;
> -    s->mig_state.cancel = migrate_fd_cancel;
> -    s->mig_state.get_status = migrate_fd_get_status;
> -    s->mig_state.release = migrate_fd_release;
> +    s->cancel = migrate_fd_cancel;
> +    s->get_status = migrate_fd_get_status;
> +    s->release = migrate_fd_release;
>
> -    s->mig_state.blk = blk;
> -    s->mig_state.shared = inc;
> +    s->blk = blk;
> +    s->shared = inc;
>
>     s->state = MIG_STATE_ACTIVE;
>     s->mon = NULL;
> diff --git a/migration-tcp.c b/migration-tcp.c
> index 355bc37..02b01ed 100644
> --- a/migration-tcp.c
> +++ b/migration-tcp.c
> @@ -95,12 +95,12 @@ FdMigrationState *tcp_start_outgoing_migration(Monitor *mon,
>     s->get_error = socket_errno;
>     s->write = socket_write;
>     s->close = tcp_close;
> -    s->mig_state.cancel = migrate_fd_cancel;
> -    s->mig_state.get_status = migrate_fd_get_status;
> -    s->mig_state.release = migrate_fd_release;
> +    s->cancel = migrate_fd_cancel;
> +    s->get_status = migrate_fd_get_status;
> +    s->release = migrate_fd_release;
>
> -    s->mig_state.blk = blk;
> -    s->mig_state.shared = inc;
> +    s->blk = blk;
> +    s->shared = inc;
>
>     s->state = MIG_STATE_ACTIVE;
>     s->mon = NULL;
> diff --git a/migration-unix.c b/migration-unix.c
> index b9b0dbf..fb73f46 100644
> --- a/migration-unix.c
> +++ b/migration-unix.c
> @@ -94,12 +94,12 @@ FdMigrationState *unix_start_outgoing_migration(Monitor *mon,
>     s->get_error = unix_errno;
>     s->write = unix_write;
>     s->close = unix_close;
> -    s->mig_state.cancel = migrate_fd_cancel;
> -    s->mig_state.get_status = migrate_fd_get_status;
> -    s->mig_state.release = migrate_fd_release;
> +    s->cancel = migrate_fd_cancel;
> +    s->get_status = migrate_fd_get_status;
> +    s->release = migrate_fd_release;
>
> -    s->mig_state.blk = blk;
> -    s->mig_state.shared = inc;
> +    s->blk = blk;
> +    s->shared = inc;
>
>     s->state = MIG_STATE_ACTIVE;
>     s->mon = NULL;
> diff --git a/migration.c b/migration.c
> index 3a371a3..dd4cdab 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -86,7 +86,7 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
>     const char *uri = qdict_get_str(qdict, "uri");
>
>     if (current_migration &&
> -        current_migration->mig_state.get_status(current_migration) == MIG_STATE_ACTIVE) {
> +        current_migration->get_status(current_migration) == MIG_STATE_ACTIVE) {
>         monitor_printf(mon, "migration already in progress\n");
>         return -1;
>     }
> @@ -120,7 +120,7 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
>     }
>
>     if (current_migration) {
> -        current_migration->mig_state.release(current_migration);
> +        current_migration->release(current_migration);
>     }
>
>     current_migration = s;
> @@ -133,7 +133,7 @@ int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
>     FdMigrationState *s = current_migration;
>
>     if (s)
> -        s->mig_state.cancel(s);
> +        s->cancel(s);
>
>     return 0;
>  }
> @@ -229,7 +229,7 @@ void do_info_migrate(Monitor *mon, QObject **ret_data)
>     QDict *qdict;
>
>     if (current_migration) {
> -        MigrationState *s = &current_migration->mig_state;
> +        FdMigrationState *s = current_migration;
>
>         switch (s->get_status(current_migration)) {
>         case MIG_STATE_ACTIVE:
> @@ -353,8 +353,7 @@ void migrate_fd_connect(FdMigrationState *s)
>                                       migrate_fd_close);
>
>     DPRINTF("beginning savevm\n");
> -    ret = qemu_savevm_state_begin(s->mon, s->file, s->mig_state.blk,
> -                                  s->mig_state.shared);
> +    ret = qemu_savevm_state_begin(s->mon, s->file, s->blk, s->shared);
>     if (ret < 0) {
>         DPRINTF("failed, %d\n", ret);
>         migrate_fd_error(s);
> diff --git a/migration.h b/migration.h
> index f49a9e2..93441e4 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -23,23 +23,10 @@
>  #define MIG_STATE_CANCELLED    1
>  #define MIG_STATE_ACTIVE       2
>
> -typedef struct MigrationState MigrationState;
> -
>  typedef struct FdMigrationState FdMigrationState;
>
> -struct MigrationState
> -{
> -    /* FIXME: add more accessors to print migration info */
> -    void (*cancel)(FdMigrationState *s);
> -    int (*get_status)(FdMigrationState *s);
> -    void (*release)(FdMigrationState *s);
> -    int blk;
> -    int shared;
> -};
> -
>  struct FdMigrationState
>  {
> -    MigrationState mig_state;
>     int64_t bandwidth_limit;
>     QEMUFile *file;
>     int fd;
> @@ -48,7 +35,12 @@ struct FdMigrationState
>     int (*get_error)(struct FdMigrationState*);
>     int (*close)(struct FdMigrationState*);
>     int (*write)(struct FdMigrationState*, const void *, size_t);
> +    void (*cancel)(FdMigrationState *s);
> +    int (*get_status)(FdMigrationState *s);
> +    void (*release)(FdMigrationState *s);
>     void *opaque;
> +    int blk;
> +    int shared;
>  };

Although I don't have objections for folding MigrationState into
FdMigrationState, it would be good to ask why the original author
split it intentionally.

Thanks,

Yoshi

>
>  void process_incoming_migration(QEMUFile *f);
> @@ -130,11 +122,6 @@ void migrate_fd_wait_for_unfreeze(void *opaque);
>
>  int migrate_fd_close(void *opaque);
>
> -static inline FdMigrationState *migrate_to_fms(MigrationState *mig_state)
> -{
> -    return container_of(mig_state, FdMigrationState, mig_state);
> -}
> -
>  void add_migration_state_change_notifier(Notifier *notify);
>  void remove_migration_state_change_notifier(Notifier *notify);
>  int get_migration_state(void);
> --
> 1.7.4
>
>
>

Re: [Qemu-devel] [PATCH 17/22] migration: use global variable directly

[Yoshiaki Tamura]

2011/2/23 Juan Quintela <quintela@redhat.com>:
> We are setting a pointer to a local variable in the previous line, just use
> the global variable directly.  We remove the ->file test because it is already
> done inside qemu_file_set_rate_limit() function.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index d7dfe1e..accc6e4 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -451,7 +451,6 @@ int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  {
>     int64_t d;
> -    MigrationState *s;
>
>     d = qdict_get_int(qdict, "value");
>     if (d < 0) {
> @@ -459,9 +458,8 @@ int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
>     }
>     max_throttle = d;
>
> -    s = current_migration;
> -    if (s && s->file) {
> -        qemu_file_set_rate_limit(s->file, max_throttle);
> +    if (current_migration) {
> +        qemu_file_set_rate_limit(current_migration->file, max_throttle);
>     }
>
>     return 0;

Looks good to me.

Yoshi

> --
> 1.7.4
>
>
>

Re: [Qemu-devel] [PATCH 12/22] migration: Use migrate_fd_error() in last place that set status to ERROR

[Yoshiaki Tamura]

2011/2/23 Juan Quintela <quintela@redhat.com>:
> We are also calling to migrate_fd_cleanup(), but notice that it is the
> right thing to do.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration.c |    6 +-----
>  1 files changed, 1 insertions(+), 5 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index ab98664..3983257 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -351,11 +351,7 @@ static ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size
>     if (ret == -EAGAIN) {
>         qemu_set_fd_handler2(s->fd, NULL, NULL, migrate_fd_put_notify, s);
>     } else if (ret < 0) {
> -        if (s->mon) {
> -            monitor_resume(s->mon);
> -        }
> -        s->state = MIG_STATE_ERROR;
> -        notifier_list_notify(&migration_state_notifiers);
> +        migrate_fd_error(s);
>     }

Are you sure about this?  migrate_fd_error may call qemu_fclose
through migrate_fd_cleanup, but the caller of
migrate_fd_put_buffer gets called by buffered_file that sits
under qemu file.  In my previous posting,

http://permalink.gmane.org/gmane.comp.emulators.qemu/94688

I thought migrate_fd_put_buffer should just return error, and let
the original caller (migrate_fd_put_notify or any) to actually call
migrate_fd_error.

Thanks,

Yoshi

>
>     return ret;
> --
> 1.7.4
>
>
>

Re: [Qemu-devel] [PATCH 08/22] migration: Check that migration is active before cancel it

[Yoshiaki Tamura]

2011/2/23 Juan Quintela <quintela@redhat.com>:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index 397a0b9..55f58c8 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -138,7 +138,7 @@ int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  {
>     MigrationState *s = current_migration;
>
> -    if (s)
> +    if (s && s->get_status(s) == MIG_STATE_ACTIVE)
>         s->cancel(s);
>
>     return 0;

Why don't you remove *s again?

Yoshi

> --
> 1.7.4
>
>
>

Re: [Qemu-devel] [PATCH 22/22] migration: Make state definitions local

[Yoshiaki Tamura]

2011/2/23 Juan Quintela <quintela@redhat.com>:
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration.c |    6 ++++++
>  migration.h |    6 ------
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index 383ebaf..90fc2a0 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -31,6 +31,12 @@
>     do { } while (0)
>  #endif
>
> +#define MIG_STATE_ERROR                -1
> +#define MIG_STATE_NONE         0
> +#define MIG_STATE_CANCELLED    1
> +#define MIG_STATE_ACTIVE       2
> +#define MIG_STATE_COMPLETED    3
> +
>  static MigrationState current_migration = {
>     .state = MIG_STATE_NONE,
>      /* Migration speed throttling */
> diff --git a/migration.h b/migration.h
> index 9457807..493fbe5 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -18,12 +18,6 @@
>  #include "qemu-common.h"
>  #include "notify.h"
>
> -#define MIG_STATE_ERROR                -1
> -#define MIG_STATE_NONE         0
> -#define MIG_STATE_CANCELLED    1
> -#define MIG_STATE_ACTIVE       2
> -#define MIG_STATE_COMPLETED    3
> -

Although you're right, I would prefer to keep it so that somebody
outside of migration may understand the status in the future if
there are no harms.

Yoshi

>  typedef struct MigrationState MigrationState;
>
>  struct MigrationState
> --
> 1.7.4
>
>
>

Re: [Qemu-devel] [PATCH 14/22] migration: Remove get_status() accessor

[Yoshiaki Tamura]

2011/2/23 Juan Quintela <quintela@redhat.com>:
> It is only used inside migration.c, and fields on that struct are
> accessed all around the place on that file.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration.c |   16 +++++-----------
>  migration.h |    1 -
>  2 files changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index dfe6a96..2b873fa 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -90,7 +90,7 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
>     int ret;
>
>     if (current_migration &&
> -        current_migration->get_status(current_migration) == MIG_STATE_ACTIVE) {
> +        current_migration->state == MIG_STATE_ACTIVE) {
>         monitor_printf(mon, "migration already in progress\n");
>         return -1;
>     }
> @@ -135,7 +135,7 @@ int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  {
>     MigrationState *s = current_migration;
>
> -    if (s && s->get_status(s) == MIG_STATE_ACTIVE)
> +    if (s && s->state == MIG_STATE_ACTIVE)
>         s->cancel(s);
>
>     return 0;
> @@ -234,7 +234,7 @@ void do_info_migrate(Monitor *mon, QObject **ret_data)
>     if (current_migration) {
>         MigrationState *s = current_migration;
>
> -        switch (s->get_status(current_migration)) {
> +        switch (s->state) {
>         case MIG_STATE_NONE:
>             /* no migration has happened ever */
>             break;
> @@ -375,7 +375,7 @@ static void migrate_fd_put_ready(void *opaque)
>         } else {
>             migrate_fd_completed(s);
>         }
> -        if (s->get_status(s) != MIG_STATE_COMPLETED) {
> +        if (s->state != MIG_STATE_COMPLETED) {
>             if (old_vm_running) {
>                 vm_start();
>             }
> @@ -383,11 +383,6 @@ static void migrate_fd_put_ready(void *opaque)
>     }
>  }
>
> -static int migrate_fd_get_status(MigrationState *s)
> -{
> -    return s->state;
> -}
> -
>  static void migrate_fd_cancel(MigrationState *s)
>  {
>     if (s->state != MIG_STATE_ACTIVE)
> @@ -442,7 +437,7 @@ void remove_migration_state_change_notifier(Notifier *notify)
>  int get_migration_state(void)
>  {
>     if (current_migration) {
> -        return migrate_fd_get_status(current_migration);
> +        return current_migration->state;
>     } else {
>         return MIG_STATE_ERROR;
>     }
> @@ -477,7 +472,6 @@ static MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limi
>     MigrationState *s = qemu_mallocz(sizeof(*s));
>
>     s->cancel = migrate_fd_cancel;
> -    s->get_status = migrate_fd_get_status;
>     s->blk = blk;
>     s->shared = inc;
>     s->mon = NULL;
> diff --git a/migration.h b/migration.h
> index 5455d8b..58a6e06 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -37,7 +37,6 @@ struct MigrationState
>     int (*close)(MigrationState*);
>     int (*write)(MigrationState*, const void *, size_t);
>     void (*cancel)(MigrationState *s);
> -    int (*get_status)(MigrationState *s);
>     void *opaque;
>     int blk;
>     int shared;

I agree to access s->state directly inside of migration.c, but I
disagree to remove get_status() accessor right away.  We don't
have strong motivations for doing that AFAIK.

Yoshi

> --
> 1.7.4
>
>
>

Re: [Qemu-devel] [PATCH 21/22] migration: Export a function that tells if the migration has finished correctly

[Yoshiaki Tamura]

2011/2/23 Juan Quintela <quintela@redhat.com>:
> This will allows us to hide the status values.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration.c     |    4 ++--
>  migration.h     |    2 +-
>  ui/spice-core.c |    4 +---
>  3 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index 312a029..383ebaf 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -335,9 +335,9 @@ void remove_migration_state_change_notifier(Notifier *notify)
>     notifier_list_remove(&migration_state_notifiers, notify);
>  }
>
> -int get_migration_state(void)
> +bool migration_has_finished(void)
>  {
> -    return current_migration.state;
> +    return current_migration.state == MIG_STATE_COMPLETED;
>  }
>
>  void migrate_fd_connect(MigrationState *s)
> diff --git a/migration.h b/migration.h
> index 6477b51..9457807 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -82,6 +82,6 @@ void migrate_fd_connect(MigrationState *s);
>
>  void add_migration_state_change_notifier(Notifier *notify);
>  void remove_migration_state_change_notifier(Notifier *notify);
> -int get_migration_state(void);
> +bool migration_has_finished(void);
>
>  #endif
> diff --git a/ui/spice-core.c b/ui/spice-core.c
> index 1aa1a5e..997603d 100644
> --- a/ui/spice-core.c
> +++ b/ui/spice-core.c
> @@ -422,9 +422,7 @@ void do_info_spice(Monitor *mon, QObject **ret_data)
>
>  static void migration_state_notifier(Notifier *notifier)
>  {
> -    int state = get_migration_state();
> -
> -    if (state == MIG_STATE_COMPLETED) {
> +    if (migration_has_finished()) {
>  #if SPICE_SERVER_VERSION >= 0x000701 /* 0.7.1 */
>         spice_server_migrate_switch(spice_server);
>  #endif

I agree to add migration_has_finished, but I don't see why we
want to remove get_migration_state.  Are we going to make
migration_has_* for each state even migration gets complicated?

Yoshi

> --
> 1.7.4
>
>
>

Re: [Qemu-devel] [PATCH 19/22] migration: convert current_migration from pointer to struct

[Yoshiaki Tamura]

2011/2/23 Juan Quintela <quintela@redhat.com>:
> This cleans up a lot the code as we don't have to check anymore if
> the variable is NULL or not.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration.c |  119 ++++++++++++++++++++++++----------------------------------
>  1 files changed, 49 insertions(+), 70 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index 4014330..7b1e679 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -34,7 +34,9 @@
>  /* Migration speed throttling */
>  static int64_t max_throttle = (32 << 20);
>
> -static MigrationState *current_migration;
> +static MigrationState current_migration = {
> +    .state = MIG_STATE_NONE,
> +};
>
>  static NotifierList migration_state_notifiers =
>     NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
> @@ -135,37 +137,34 @@ void do_info_migrate(Monitor *mon, QObject **ret_data)
>  {
>     QDict *qdict;
>
> -    if (current_migration) {
> -
> -        switch (current_migration->state) {
> -        case MIG_STATE_NONE:
> -            /* no migration has happened ever */
> -            break;
> -        case MIG_STATE_ACTIVE:
> -            qdict = qdict_new();
> -            qdict_put(qdict, "status", qstring_from_str("active"));
> -
> -            migrate_put_status(qdict, "ram", ram_bytes_transferred(),
> -                               ram_bytes_remaining(), ram_bytes_total());
> -
> -            if (blk_mig_active()) {
> -                migrate_put_status(qdict, "disk", blk_mig_bytes_transferred(),
> -                                   blk_mig_bytes_remaining(),
> -                                   blk_mig_bytes_total());
> -            }
> -
> -            *ret_data = QOBJECT(qdict);
> -            break;
> -        case MIG_STATE_COMPLETED:
> -            *ret_data = qobject_from_jsonf("{ 'status': 'completed' }");
> -            break;
> -        case MIG_STATE_ERROR:
> -            *ret_data = qobject_from_jsonf("{ 'status': 'failed' }");
> -            break;
> -        case MIG_STATE_CANCELLED:
> -            *ret_data = qobject_from_jsonf("{ 'status': 'cancelled' }");
> -            break;
> +    switch (current_migration.state) {
> +    case MIG_STATE_NONE:
> +        /* no migration has happened ever */
> +        break;
> +    case MIG_STATE_ACTIVE:
> +        qdict = qdict_new();
> +        qdict_put(qdict, "status", qstring_from_str("active"));
> +
> +        migrate_put_status(qdict, "ram", ram_bytes_transferred(),
> +                           ram_bytes_remaining(), ram_bytes_total());
> +
> +        if (blk_mig_active()) {
> +            migrate_put_status(qdict, "disk", blk_mig_bytes_transferred(),
> +                               blk_mig_bytes_remaining(),
> +                               blk_mig_bytes_total());
>         }
> +
> +        *ret_data = QOBJECT(qdict);
> +        break;
> +    case MIG_STATE_COMPLETED:
> +        *ret_data = qobject_from_jsonf("{ 'status': 'completed' }");
> +        break;
> +    case MIG_STATE_ERROR:
> +        *ret_data = qobject_from_jsonf("{ 'status': 'failed' }");
> +        break;
> +    case MIG_STATE_CANCELLED:
> +        *ret_data = qobject_from_jsonf("{ 'status': 'cancelled' }");
> +        break;
>     }
>  }
>
> @@ -339,11 +338,7 @@ void remove_migration_state_change_notifier(Notifier *notify)
>
>  int get_migration_state(void)
>  {
> -    if (current_migration) {
> -        return current_migration->state;
> -    } else {
> -        return MIG_STATE_ERROR;
> -    }
> +    return current_migration.state;
>  }
>
>  void migrate_fd_connect(MigrationState *s)
> @@ -369,27 +364,22 @@ void migrate_fd_connect(MigrationState *s)
>     migrate_fd_put_ready(s);
>  }
>
> -static MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limit,
> -                                            int detach, int blk, int inc)
> +static void migrate_init_state(Monitor *mon, int64_t bandwidth_limit,
> +                               int detach, int blk, int inc)
>  {
> -    MigrationState *s = qemu_mallocz(sizeof(*s));
> -
> -    s->blk = blk;
> -    s->shared = inc;
> -    s->mon = NULL;
> -    s->bandwidth_limit = bandwidth_limit;
> -    s->state = MIG_STATE_NONE;
> +    current_migration.blk = blk;
> +    current_migration.shared = inc;
> +    current_migration.mon = NULL;
> +    current_migration.bandwidth_limit = bandwidth_limit;
> +    current_migration.state = MIG_STATE_NONE;
>
>     if (!detach) {
> -        migrate_fd_monitor_suspend(s, mon);
> +        migrate_fd_monitor_suspend(&current_migration, mon);
>     }
> -
> -    return s;
>  }
>
>  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);
> @@ -397,8 +387,7 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
>     const char *uri = qdict_get_str(qdict, "uri");
>     int ret;
>
> -    if (current_migration &&
> -        current_migration->state == MIG_STATE_ACTIVE) {
> +    if (current_migration.state == MIG_STATE_ACTIVE) {
>         monitor_printf(mon, "migration already in progress\n");
>         return -1;
>     }
> @@ -407,42 +396,35 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
>         return -1;
>     }
>
> -    s = migrate_create_state(mon, max_throttle, detach, blk, inc);
> +    migrate_init_state(mon, max_throttle, detach, blk, inc);
>
>     if (strstart(uri, "tcp:", &p)) {
> -        ret = tcp_start_outgoing_migration(s, p);
> +        ret = tcp_start_outgoing_migration(&current_migration, p);
>  #if !defined(WIN32)
>     } else if (strstart(uri, "exec:", &p)) {
> -        ret = exec_start_outgoing_migration(s, p);
> +        ret = exec_start_outgoing_migration(&current_migration, p);
>     } else if (strstart(uri, "unix:", &p)) {
> -        ret = unix_start_outgoing_migration(s, p);
> +        ret = unix_start_outgoing_migration(&current_migration, p);
>     } else if (strstart(uri, "fd:", &p)) {
> -        ret = fd_start_outgoing_migration(s, p);
> +        ret = fd_start_outgoing_migration(&current_migration, p);
>  #endif
>     } else {
>         monitor_printf(mon, "unknown migration protocol: %s\n", uri);
> -        ret  = -EINVAL;
> -        goto free_migrate_state;
> +        return -EINVAL;
>     }
>
>     if (ret < 0) {
>         monitor_printf(mon, "migration failed\n");
> -        goto free_migrate_state;
> +        return ret;
>     }
>
> -    qemu_free(current_migration);
> -    current_migration = s;
>     notifier_list_notify(&migration_state_notifiers);
>     return 0;
> -free_migrate_state:
> -    qemu_free(s);
> -    return -1;
>  }
>
>  int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  {
> -    if (current_migration)
> -        migrate_fd_cancel(current_migration);
> +    migrate_fd_cancel(&current_migration);
>
>     return 0;
>  }
> @@ -457,10 +439,7 @@ int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
>     }
>     max_throttle = d;
>
> -    if (current_migration) {
> -        qemu_file_set_rate_limit(current_migration->file, max_throttle);
> -    }
> -
> +    qemu_file_set_rate_limit(current_migration.file, max_throttle);
>     return 0;
>  }
>

Looks good to me.

Yoshi

> --
> 1.7.4
>
>
>

Re: [Qemu-devel] [PATCH 18/22] migration: another case of global variable assigned to local one

[Yoshiaki Tamura]

2011/2/23 Juan Quintela <quintela@redhat.com>:
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index accc6e4..4014330 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -136,9 +136,8 @@ void do_info_migrate(Monitor *mon, QObject **ret_data)
>     QDict *qdict;
>
>     if (current_migration) {
> -        MigrationState *s = current_migration;
>
> -        switch (s->state) {
> +        switch (current_migration->state) {
>         case MIG_STATE_NONE:
>             /* no migration has happened ever */
>             break;

Looks good to me.

Yoshi

> --
> 1.7.4
>
>
>

[Qemu-devel] Re: [PATCH 00/22] Refactor and cleaup migration code

[Paolo Bonzini]

On 02/23/2011 01:44 AM, Juan Quintela wrote:
> This series:
> - Fold MigrationState into FdMigrationState (and then rename)
> - Factorize migration statec creation in a single place
> - Make use of MIG_STATE_*, setup through helpers and make them local
> - remove relase&  cancel callbacks (where used only one in same
>    file than defined)
> - get_status() is no more, just access directly to .state
> - current_migration use cleanup, and make variable static
> - max_throotle is gone, now inside current_migration
> - change get_migration_status() to migration_has_finished()
>    and actualize single user.
>
> Please review.

Some changes are a matter of taste, but overall looks very nice!

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [Qemu-devel] [PATCH 16/22] migration: Move exported functions to the end of the file

[Yoshiaki Tamura]

2011/2/23 Juan Quintela <quintela@redhat.com>:
> This means we can remove the two forward declarations.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration.c |  188 +++++++++++++++++++++++++++++------------------------------
>  1 files changed, 92 insertions(+), 96 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index 92bff01..d7dfe1e 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -76,90 +76,6 @@ void process_incoming_migration(QEMUFile *f)
>         vm_start();
>  }
>
> -static MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limit,
> -                                            int detach, int blk, int inc);
> -
> -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");
> -    int ret;
> -
> -    if (current_migration &&
> -        current_migration->state == MIG_STATE_ACTIVE) {
> -        monitor_printf(mon, "migration already in progress\n");
> -        return -1;
> -    }
> -
> -    if (qemu_savevm_state_blocked(mon)) {
> -        return -1;
> -    }
> -
> -    s = migrate_create_state(mon, max_throttle, detach, blk, inc);
> -
> -    if (strstart(uri, "tcp:", &p)) {
> -        ret = tcp_start_outgoing_migration(s, p);
> -#if !defined(WIN32)
> -    } else if (strstart(uri, "exec:", &p)) {
> -        ret = exec_start_outgoing_migration(s, p);
> -    } else if (strstart(uri, "unix:", &p)) {
> -        ret = unix_start_outgoing_migration(s, p);
> -    } else if (strstart(uri, "fd:", &p)) {
> -        ret = fd_start_outgoing_migration(s, p);
> -#endif
> -    } else {
> -        monitor_printf(mon, "unknown migration protocol: %s\n", uri);
> -        ret  = -EINVAL;
> -        goto free_migrate_state;
> -    }
> -
> -    if (ret < 0) {
> -        monitor_printf(mon, "migration failed\n");
> -        goto free_migrate_state;
> -    }
> -
> -    qemu_free(current_migration);
> -    current_migration = s;
> -    notifier_list_notify(&migration_state_notifiers);
> -    return 0;
> -free_migrate_state:
> -    qemu_free(s);
> -    return -1;
> -}
> -
> -static void migrate_fd_cancel(MigrationState *s);
> -
> -int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
> -{
> -    if (current_migration)
> -        migrate_fd_cancel(current_migration);
> -
> -    return 0;
> -}
> -
> -int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
> -{
> -    int64_t d;
> -    MigrationState *s;
> -
> -    d = qdict_get_int(qdict, "value");
> -    if (d < 0) {
> -        d = 0;
> -    }
> -    max_throttle = d;
> -
> -    s = current_migration;
> -    if (s && s->file) {
> -        qemu_file_set_rate_limit(s->file, max_throttle);
> -    }
> -
> -    return 0;
> -}
> -
>  /* amount of nanoseconds we are willing to wait for migration to be down.
>  * the choice of nanoseconds is because it is the maximum resolution that
>  * get_clock() can achieve. It is an internal measure. All user-visible
> @@ -171,18 +87,6 @@ uint64_t migrate_max_downtime(void)
>     return max_downtime;
>  }
>
> -int do_migrate_set_downtime(Monitor *mon, const QDict *qdict,
> -                            QObject **ret_data)
> -{
> -    double d;
> -
> -    d = qdict_get_double(qdict, "value") * 1e9;
> -    d = MAX(0, MIN(UINT64_MAX, d));
> -    max_downtime = (uint64_t)d;
> -
> -    return 0;
> -}
> -
>  static void migrate_print_status(Monitor *mon, const char *name,
>                                  const QDict *status_dict)
>  {
> @@ -483,3 +387,95 @@ static MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limi
>
>     return s;
>  }
> +
> +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");
> +    int ret;
> +
> +    if (current_migration &&
> +        current_migration->state == MIG_STATE_ACTIVE) {
> +        monitor_printf(mon, "migration already in progress\n");
> +        return -1;
> +    }
> +
> +    if (qemu_savevm_state_blocked(mon)) {
> +        return -1;
> +    }
> +
> +    s = migrate_create_state(mon, max_throttle, detach, blk, inc);
> +
> +    if (strstart(uri, "tcp:", &p)) {
> +        ret = tcp_start_outgoing_migration(s, p);
> +#if !defined(WIN32)
> +    } else if (strstart(uri, "exec:", &p)) {
> +        ret = exec_start_outgoing_migration(s, p);
> +    } else if (strstart(uri, "unix:", &p)) {
> +        ret = unix_start_outgoing_migration(s, p);
> +    } else if (strstart(uri, "fd:", &p)) {
> +        ret = fd_start_outgoing_migration(s, p);
> +#endif
> +    } else {
> +        monitor_printf(mon, "unknown migration protocol: %s\n", uri);
> +        ret  = -EINVAL;
> +        goto free_migrate_state;
> +    }
> +
> +    if (ret < 0) {
> +        monitor_printf(mon, "migration failed\n");
> +        goto free_migrate_state;
> +    }
> +
> +    qemu_free(current_migration);
> +    current_migration = s;
> +    notifier_list_notify(&migration_state_notifiers);
> +    return 0;
> +free_migrate_state:
> +    qemu_free(s);
> +    return -1;
> +}
> +
> +int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +{
> +    if (current_migration)
> +        migrate_fd_cancel(current_migration);
> +
> +    return 0;
> +}
> +
> +int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +{
> +    int64_t d;
> +    MigrationState *s;
> +
> +    d = qdict_get_int(qdict, "value");
> +    if (d < 0) {
> +        d = 0;
> +    }
> +    max_throttle = d;
> +
> +    s = current_migration;
> +    if (s && s->file) {
> +        qemu_file_set_rate_limit(s->file, max_throttle);
> +    }
> +
> +    return 0;
> +}
> +
> +int do_migrate_set_downtime(Monitor *mon, const QDict *qdict,
> +                            QObject **ret_data)
> +{
> +    double d;
> +
> +    d = qdict_get_double(qdict, "value") * 1e9;
> +    d = MAX(0, MIN(UINT64_MAX, d));
> +    max_downtime = (uint64_t)d;
> +
> +    return 0;
> +}
> +

Looks good to me.

Yoshi

> --
> 1.7.4
>
>
>

[Qemu-devel] Re: [PATCH 03/22] migration: Fold MigrationState into FdMigrationState

[Juan Quintela]

Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> wrote:
> 2011/2/23 Juan Quintela <quintela@redhat.com>:

>>  struct FdMigrationState
>>  {
>> -    MigrationState mig_state;
>>     int64_t bandwidth_limit;
>>     QEMUFile *file;
>>     int fd;
>> @@ -48,7 +35,12 @@ struct FdMigrationState
>>     int (*get_error)(struct FdMigrationState*);
>>     int (*close)(struct FdMigrationState*);
>>     int (*write)(struct FdMigrationState*, const void *, size_t);
>> +    void (*cancel)(FdMigrationState *s);
>> +    int (*get_status)(FdMigrationState *s);
>> +    void (*release)(FdMigrationState *s);
>>     void *opaque;
>> +    int blk;
>> +    int shared;
>>  };
>
> Although I don't have objections for folding MigrationState into
> FdMigrationState, it would be good to ask why the original author
> split it intentionally.

I asked, Anthony answer was that it is a "historical" artifact.

Later, Juan.

[Qemu-devel] Re: [PATCH 08/22] migration: Check that migration is active before cancel it

[Juan Quintela]

Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> wrote:
> 2011/2/23 Juan Quintela <quintela@redhat.com>:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/migration.c b/migration.c
>> index 397a0b9..55f58c8 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -138,7 +138,7 @@ int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
>>  {
>>     MigrationState *s = current_migration;
>>
>> -    if (s)
>> +    if (s && s->get_status(s) == MIG_STATE_ACTIVE)
>>         s->cancel(s);
>>
>>     return 0;
>
> Why don't you remove *s again?

Removed in a next patch.

Later, Juan.

[Qemu-devel] Re: [PATCH 14/22] migration: Remove get_status() accessor

[Juan Quintela]

Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> wrote:
> 2011/2/23 Juan Quintela <quintela@redhat.com>:
>> It is only used inside migration.c, and fields on that struct are
>> accessed all around the place on that file.

>
> I agree to access s->state directly inside of migration.c, but I
> disagree to remove get_status() accessor right away.  We don't
> have strong motivations for doing that AFAIK.

Only user outside of migration.c was ui/spice-core.c, and it just wanted
to know if migration has finished at all.

At this point I was trying to isolate what other parts of MigrationState
are used externally.  That way, it gets easier to change that later.

At this point, only things used outside of migration.c are:
- write, clase, get_error: trivial to fix, just add setters for them.
- fd: that is not enterely trivial to fix.

Later, Juan.

Re: [Qemu-devel] [PATCH 02/22] migration: Use FdMigrationState instead of MigrationState when possible

[Yoshiaki Tamura]

2011/2/23 Juan Quintela <quintela@redhat.com>:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration.c |   31 ++++++++++++++-----------------
>  migration.h |   16 ++++++++--------
>  2 files changed, 22 insertions(+), 25 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index f9aaadc..3a371a3 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -34,7 +34,7 @@
>  /* Migration speed throttling */
>  static int64_t max_throttle = (32 << 20);
>
> -static MigrationState *current_migration;
> +static FdMigrationState *current_migration;
>
>  static NotifierList migration_state_notifiers =
>     NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
> @@ -86,7 +86,7 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
>     const char *uri = qdict_get_str(qdict, "uri");
>
>     if (current_migration &&
> -        current_migration->get_status(current_migration) == MIG_STATE_ACTIVE) {
> +        current_migration->mig_state.get_status(current_migration) == MIG_STATE_ACTIVE) {
>         monitor_printf(mon, "migration already in progress\n");
>         return -1;
>     }
> @@ -120,20 +120,20 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
>     }
>
>     if (current_migration) {
> -        current_migration->release(current_migration);
> +        current_migration->mig_state.release(current_migration);
>     }
>
> -    current_migration = &s->mig_state;
> +    current_migration = s;
>     notifier_list_notify(&migration_state_notifiers);
>     return 0;
>  }
>
>  int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
>  {
> -    MigrationState *s = current_migration;
> +    FdMigrationState *s = current_migration;
>
>     if (s)
> -        s->cancel(s);
> +        s->mig_state.cancel(s);
>
>     return 0;
>  }
> @@ -149,7 +149,7 @@ int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
>     }
>     max_throttle = d;
>
> -    s = migrate_to_fms(current_migration);
> +    s = current_migration;
>     if (s && s->file) {
>         qemu_file_set_rate_limit(s->file, max_throttle);
>     }
> @@ -227,10 +227,11 @@ static void migrate_put_status(QDict *qdict, const char *name,
>  void do_info_migrate(Monitor *mon, QObject **ret_data)
>  {
>     QDict *qdict;
> -    MigrationState *s = current_migration;
>
> -    if (s) {
> -        switch (s->get_status(s)) {
> +    if (current_migration) {
> +        MigrationState *s = &current_migration->mig_state;
> +
> +        switch (s->get_status(current_migration)) {
>         case MIG_STATE_ACTIVE:
>             qdict = qdict_new();
>             qdict_put(qdict, "status", qstring_from_str("active"));
> @@ -399,16 +400,13 @@ void migrate_fd_put_ready(void *opaque)
>     }
>  }
>
> -int migrate_fd_get_status(MigrationState *mig_state)
> +int migrate_fd_get_status(FdMigrationState *s)
>  {
> -    FdMigrationState *s = migrate_to_fms(mig_state);
>     return s->state;
>  }
>
> -void migrate_fd_cancel(MigrationState *mig_state)
> +void migrate_fd_cancel(FdMigrationState *s)
>  {
> -    FdMigrationState *s = migrate_to_fms(mig_state);
> -
>     if (s->state != MIG_STATE_ACTIVE)
>         return;
>
> @@ -421,9 +419,8 @@ void migrate_fd_cancel(MigrationState *mig_state)
>     migrate_fd_cleanup(s);
>  }
>
> -void migrate_fd_release(MigrationState *mig_state)
> +void migrate_fd_release(FdMigrationState *s)
>  {
> -    FdMigrationState *s = migrate_to_fms(mig_state);
>
>     DPRINTF("releasing state\n");
>
> diff --git a/migration.h b/migration.h
> index db0e45a..f49a9e2 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -25,18 +25,18 @@
>
>  typedef struct MigrationState MigrationState;
>
> +typedef struct FdMigrationState FdMigrationState;
> +
>  struct MigrationState
>  {
>     /* FIXME: add more accessors to print migration info */
> -    void (*cancel)(MigrationState *s);
> -    int (*get_status)(MigrationState *s);
> -    void (*release)(MigrationState *s);
> +    void (*cancel)(FdMigrationState *s);
> +    int (*get_status)(FdMigrationState *s);
> +    void (*release)(FdMigrationState *s);
>     int blk;
>     int shared;
>  };
>
> -typedef struct FdMigrationState FdMigrationState;
> -
>  struct FdMigrationState
>  {
>     MigrationState mig_state;
> @@ -120,11 +120,11 @@ void migrate_fd_connect(FdMigrationState *s);
>
>  void migrate_fd_put_ready(void *opaque);
>
> -int migrate_fd_get_status(MigrationState *mig_state);
> +int migrate_fd_get_status(FdMigrationState *mig_state);
>
> -void migrate_fd_cancel(MigrationState *mig_state);
> +void migrate_fd_cancel(FdMigrationState *mig_state);
>
> -void migrate_fd_release(MigrationState *mig_state);
> +void migrate_fd_release(FdMigrationState *mig_state);
>
>  void migrate_fd_wait_for_unfreeze(void *opaque);
>

Looks good to me.

Yoshi

> --
> 1.7.4
>
>
>

[Qemu-devel] Re: [PATCH 22/22] migration: Make state definitions local

[Juan Quintela]

Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> wrote:
> 2011/2/23 Juan Quintela <quintela@redhat.com>:
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration.c |    6 ++++++
>>  migration.h |    6 ------
>>  2 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/migration.c b/migration.c
>> index 383ebaf..90fc2a0 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -31,6 +31,12 @@
>>     do { } while (0)
>>  #endif
>>
>> +#define MIG_STATE_ERROR                -1
>> +#define MIG_STATE_NONE         0
>> +#define MIG_STATE_CANCELLED    1
>> +#define MIG_STATE_ACTIVE       2
>> +#define MIG_STATE_COMPLETED    3
>> +
>>  static MigrationState current_migration = {
>>     .state = MIG_STATE_NONE,
>>      /* Migration speed throttling */
>> diff --git a/migration.h b/migration.h
>> index 9457807..493fbe5 100644
>> --- a/migration.h
>> +++ b/migration.h
>> @@ -18,12 +18,6 @@
>>  #include "qemu-common.h"
>>  #include "notify.h"
>>
>> -#define MIG_STATE_ERROR                -1
>> -#define MIG_STATE_NONE         0
>> -#define MIG_STATE_CANCELLED    1
>> -#define MIG_STATE_ACTIVE       2
>> -#define MIG_STATE_COMPLETED    3
>> -
>
> Although you're right, I would prefer to keep it so that somebody
> outside of migration may understand the status in the future if
> there are no harms.

my plan is to move MigrationState inside migration.c, and then decide
what to export/not export.  Next thing to do is move migration to its
own thread.  Before doing that, I need to know what parts are used/not
used outside migration.c.  Removing it now means that nothing gets to
use it without needing a patch.

Later, Juan..

Re: [Qemu-devel] [PATCH 06/22] migration: Make all posible migration functions static

[Yoshiaki Tamura]

2011/2/23 Juan Quintela <quintela@redhat.com>:
> I have to move two functions postions to avoid forward declarations
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration.c |   72 +++++++++++++++++++++++++++++-----------------------------
>  migration.h |   12 ---------
>  2 files changed, 36 insertions(+), 48 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index e773806..1853380 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -273,15 +273,7 @@ static void migrate_fd_monitor_suspend(MigrationState *s, Monitor *mon)
>     }
>  }
>
> -void migrate_fd_error(MigrationState *s)
> -{
> -    DPRINTF("setting error state\n");
> -    s->state = MIG_STATE_ERROR;
> -    notifier_list_notify(&migration_state_notifiers);
> -    migrate_fd_cleanup(s);
> -}
> -
> -int migrate_fd_cleanup(MigrationState *s)
> +static int migrate_fd_cleanup(MigrationState *s)
>  {
>     int ret = 0;
>
> @@ -308,7 +300,15 @@ int migrate_fd_cleanup(MigrationState *s)
>     return ret;
>  }
>
> -void migrate_fd_put_notify(void *opaque)
> +void migrate_fd_error(MigrationState *s)
> +{
> +    DPRINTF("setting error state\n");
> +    s->state = MIG_STATE_ERROR;
> +    notifier_list_notify(&migration_state_notifiers);
> +    migrate_fd_cleanup(s);
> +}
> +
> +static void migrate_fd_put_notify(void *opaque)
>  {
>     MigrationState *s = opaque;
>
> @@ -316,7 +316,7 @@ void migrate_fd_put_notify(void *opaque)
>     qemu_file_put_notify(s->file);
>  }
>
> -ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size)
> +static ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size)
>  {
>     MigrationState *s = opaque;
>     ssize_t ret;
> @@ -341,29 +341,7 @@ ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size)
>     return ret;
>  }
>
> -void migrate_fd_connect(MigrationState *s)
> -{
> -    int ret;
> -
> -    s->file = qemu_fopen_ops_buffered(s,
> -                                      s->bandwidth_limit,
> -                                      migrate_fd_put_buffer,
> -                                      migrate_fd_put_ready,
> -                                      migrate_fd_wait_for_unfreeze,
> -                                      migrate_fd_close);
> -
> -    DPRINTF("beginning savevm\n");
> -    ret = qemu_savevm_state_begin(s->mon, s->file, s->blk, s->shared);
> -    if (ret < 0) {
> -        DPRINTF("failed, %d\n", ret);
> -        migrate_fd_error(s);
> -        return;
> -    }
> -
> -    migrate_fd_put_ready(s);
> -}
> -
> -void migrate_fd_put_ready(void *opaque)
> +static void migrate_fd_put_ready(void *opaque)
>  {
>     MigrationState *s = opaque;
>
> @@ -431,7 +409,7 @@ static void migrate_fd_release(MigrationState *s)
>     qemu_free(s);
>  }
>
> -void migrate_fd_wait_for_unfreeze(void *opaque)
> +static void migrate_fd_wait_for_unfreeze(void *opaque)
>  {
>     MigrationState *s = opaque;
>     int ret;
> @@ -450,7 +428,7 @@ void migrate_fd_wait_for_unfreeze(void *opaque)
>     } while (ret == -1 && (s->get_error(s)) == EINTR);
>  }
>
> -int migrate_fd_close(void *opaque)
> +static int migrate_fd_close(void *opaque)
>  {
>     MigrationState *s = opaque;
>
> @@ -477,6 +455,28 @@ int get_migration_state(void)
>     }
>  }
>
> +void migrate_fd_connect(MigrationState *s)
> +{
> +    int ret;
> +
> +    s->file = qemu_fopen_ops_buffered(s,
> +                                      s->bandwidth_limit,
> +                                      migrate_fd_put_buffer,
> +                                      migrate_fd_put_ready,
> +                                      migrate_fd_wait_for_unfreeze,
> +                                      migrate_fd_close);
> +
> +    DPRINTF("beginning savevm\n");
> +    ret = qemu_savevm_state_begin(s->mon, s->file, s->blk, s->shared);
> +    if (ret < 0) {
> +        DPRINTF("failed, %d\n", ret);
> +        migrate_fd_error(s);
> +        return;
> +    }
> +
> +    migrate_fd_put_ready(s);
> +}
> +
>  MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limit,
>                                      int detach, int blk, int inc)
>  {
> diff --git a/migration.h b/migration.h
> index 0178414..048ee46 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -100,20 +100,8 @@ MigrationState *fd_start_outgoing_migration(Monitor *mon,
>
>  void migrate_fd_error(MigrationState *s);
>
> -int migrate_fd_cleanup(MigrationState *s);
> -
> -void migrate_fd_put_notify(void *opaque);
> -
> -ssize_t migrate_fd_put_buffer(void *opaque, const void *data, size_t size);
> -
>  void migrate_fd_connect(MigrationState *s);
>
> -void migrate_fd_put_ready(void *opaque);
> -
> -void migrate_fd_wait_for_unfreeze(void *opaque);
> -
> -int migrate_fd_close(void *opaque);
> -
>  MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limit,
>                                      int detach, int blk, int inc);
>

Looks good to me.

Yoshi

> --
> 1.7.4
>
>
>

Re: [Qemu-devel] [PATCH 09/22] migration: Introduce MIG_STATE_NONE

[Yoshiaki Tamura]

2011/2/23 Juan Quintela <quintela@redhat.com>:
> Use MIG_STATE_ACTIVE only when migration has really started
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration.c |    6 +++++-
>  migration.h |    3 ++-
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index 55f58c8..f015e02 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -238,6 +238,9 @@ void do_info_migrate(Monitor *mon, QObject **ret_data)
>         MigrationState *s = current_migration;
>
>         switch (s->get_status(current_migration)) {
> +        case MIG_STATE_NONE:
> +            /* no migration has happened ever */
> +            break;
>         case MIG_STATE_ACTIVE:
>             qdict = qdict_new();
>             qdict_put(qdict, "status", qstring_from_str("active"));
> @@ -465,6 +468,7 @@ void migrate_fd_connect(MigrationState *s)
>  {
>     int ret;
>
> +    s->state = MIG_STATE_ACTIVE;
>     s->file = qemu_fopen_ops_buffered(s,
>                                       s->bandwidth_limit,
>                                       migrate_fd_put_buffer,
> @@ -495,7 +499,7 @@ static MigrationState *migrate_create_state(Monitor *mon, int64_t bandwidth_limi
>     s->shared = inc;
>     s->mon = NULL;
>     s->bandwidth_limit = bandwidth_limit;
> -    s->state = MIG_STATE_ACTIVE;
> +    s->state = MIG_STATE_NONE;
>
>     if (!detach) {
>         migrate_fd_monitor_suspend(s, mon);
> diff --git a/migration.h b/migration.h
> index 7d28dd3..3df2293 100644
> --- a/migration.h
> +++ b/migration.h
> @@ -19,9 +19,10 @@
>  #include "notify.h"
>
>  #define MIG_STATE_ERROR                -1
> -#define MIG_STATE_COMPLETED    0
> +#define MIG_STATE_NONE         0
>  #define MIG_STATE_CANCELLED    1
>  #define MIG_STATE_ACTIVE       2
> +#define MIG_STATE_COMPLETED    3

It may be a good chance to make them enum?

Yoshi

>
>  typedef struct MigrationState MigrationState;
>
> --
> 1.7.4
>
>
>

Re: [Qemu-devel] [PATCH 10/22] migration: Refactor and simplify error checking in migrate_fd_put_ready

[Yoshiaki Tamura]

2011/2/23 Juan Quintela <quintela@redhat.com>:
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration.c |   20 +++++++++-----------
>  1 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/migration.c b/migration.c
> index f015e02..641df9f 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -361,28 +361,26 @@ static void migrate_fd_put_ready(void *opaque)
>
>     DPRINTF("iterate\n");
>     if (qemu_savevm_state_iterate(s->mon, s->file) == 1) {
> -        int state;
>         int old_vm_running = vm_running;
>
>         DPRINTF("done iterating\n");
>         vm_stop(VMSTOP_MIGRATE);
>
> -        if ((qemu_savevm_state_complete(s->mon, s->file)) < 0) {
> -            if (old_vm_running) {
> -                vm_start();
> -            }
> -            state = MIG_STATE_ERROR;
> +        if (qemu_savevm_state_complete(s->mon, s->file) < 0) {
> +            migrate_fd_error(s);
>         } else {
> -            state = MIG_STATE_COMPLETED;
> +            if (migrate_fd_cleanup(s) < 0) {
> +                migrate_fd_error(s);
> +            } else {
> +                s->state = MIG_STATE_COMPLETED;
> +                notifier_list_notify(&migration_state_notifiers);
> +            }
>         }
> -        if (migrate_fd_cleanup(s) < 0) {
> +        if (s->get_status(s) != MIG_STATE_COMPLETED) {
>             if (old_vm_running) {
>                 vm_start();
>             }

This part, although it's not fair to ask you, but calling
vm_start when != MIG_STATE_COMPLETED terrifies me because just
failing migrate_fd_cleanup (mostly calling qemu_fclose) may cause
split brain between src/dst.  Although I haven't encountered this
situation, just having stopped VMs on both sides is safer.

Thanks,

Yoshi

> -            state = MIG_STATE_ERROR;
>         }
> -        s->state = state;
> -        notifier_list_notify(&migration_state_notifiers);
>     }
>  }
>
> --
> 1.7.4
>
>
>

[Qemu-devel] Re: [PATCH 10/22] migration: Refactor and simplify error checking in migrate_fd_put_ready

[Juan Quintela]

Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> wrote:
> 2011/2/23 Juan Quintela <quintela@redhat.com>:
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration.c |   20 +++++++++-----------
>>  1 files changed, 9 insertions(+), 11 deletions(-)
>>
>> diff --git a/migration.c b/migration.c
>> index f015e02..641df9f 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -361,28 +361,26 @@ static void migrate_fd_put_ready(void *opaque)
>>
>>     DPRINTF("iterate\n");
>>     if (qemu_savevm_state_iterate(s->mon, s->file) == 1) {
>> -        int state;
>>         int old_vm_running = vm_running;
>>
>>         DPRINTF("done iterating\n");
>>         vm_stop(VMSTOP_MIGRATE);
>>
>> -        if ((qemu_savevm_state_complete(s->mon, s->file)) < 0) {
>> -            if (old_vm_running) {
>> -                vm_start();
>> -            }
>> -            state = MIG_STATE_ERROR;
>> +        if (qemu_savevm_state_complete(s->mon, s->file) < 0) {
>> +            migrate_fd_error(s);
>>         } else {
>> -            state = MIG_STATE_COMPLETED;
>> +            if (migrate_fd_cleanup(s) < 0) {
>> +                migrate_fd_error(s);
>> +            } else {
>> +                s->state = MIG_STATE_COMPLETED;
>> +                notifier_list_notify(&migration_state_notifiers);
>> +            }
>>         }
>> -        if (migrate_fd_cleanup(s) < 0) {
>> +        if (s->get_status(s) != MIG_STATE_COMPLETED) {
>>             if (old_vm_running) {
>>                 vm_start();
>>             }
>
> This part, although it's not fair to ask you, but calling
> vm_start when != MIG_STATE_COMPLETED terrifies me because just
> failing migrate_fd_cleanup (mostly calling qemu_fclose) may cause
> split brain between src/dst.  Although I haven't encountered this
> situation, just having stopped VMs on both sides is safer.

I see your pain. I am not happy at all, but this was integrated by
Anthony to fix this bug:

commit 41ef56e61153d7bd27d34a634633bb51b1c5988d
Author: Anthony Liguori <aliguori@us.ibm.com>
Date:   Wed Jun 2 14:55:25 2010 -0500

    migration: respect exit status with exec:

 This fixes https://bugs.launchpad.net/qemu/+bug/391879


I think that it fixes that bug, but it makes me un-easy to restart vm if
there is a failure in migrate_fd_cleanup().  As I didn't wanted to
change behaviour with this series, I left it as it was.

Next on ToDo list is to do something sensible with errors, just now we
are not very good at handling them.

Later, Juan.

[Qemu-devel] Re: [PATCH 00/22] Refactor and cleaup migration code

[Jan Kiszka]

On 2011-02-23 01:44, Juan Quintela wrote:
> This series:
> - Fold MigrationState into FdMigrationState (and then rename)
> - Factorize migration statec creation in a single place
> - Make use of MIG_STATE_*, setup through helpers and make them local
> - remove relase & cancel callbacks (where used only one in same
>   file than defined)
> - get_status() is no more, just access directly to .state
> - current_migration use cleanup, and make variable static
> - max_throotle is gone, now inside current_migration
> - change get_migration_status() to migration_has_finished()
>   and actualize single user.
> 
> Please review.

Tried checkpatch.pl? :) I can only recommend to include it in personal
preparation scripts for patch series.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [PATCH 10/22] migration: Refactor and simplify error checking in migrate_fd_put_ready

[Yoshiaki Tamura]

2011/2/23 Juan Quintela <quintela@redhat.com>:
> Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> wrote:
>> 2011/2/23 Juan Quintela <quintela@redhat.com>:
>>>
>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>> ---
>>>  migration.c |   20 +++++++++-----------
>>>  1 files changed, 9 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/migration.c b/migration.c
>>> index f015e02..641df9f 100644
>>> --- a/migration.c
>>> +++ b/migration.c
>>> @@ -361,28 +361,26 @@ static void migrate_fd_put_ready(void *opaque)
>>>
>>>     DPRINTF("iterate\n");
>>>     if (qemu_savevm_state_iterate(s->mon, s->file) == 1) {
>>> -        int state;
>>>         int old_vm_running = vm_running;
>>>
>>>         DPRINTF("done iterating\n");
>>>         vm_stop(VMSTOP_MIGRATE);
>>>
>>> -        if ((qemu_savevm_state_complete(s->mon, s->file)) < 0) {
>>> -            if (old_vm_running) {
>>> -                vm_start();
>>> -            }
>>> -            state = MIG_STATE_ERROR;
>>> +        if (qemu_savevm_state_complete(s->mon, s->file) < 0) {
>>> +            migrate_fd_error(s);
>>>         } else {
>>> -            state = MIG_STATE_COMPLETED;
>>> +            if (migrate_fd_cleanup(s) < 0) {
>>> +                migrate_fd_error(s);
>>> +            } else {
>>> +                s->state = MIG_STATE_COMPLETED;
>>> +                notifier_list_notify(&migration_state_notifiers);
>>> +            }
>>>         }
>>> -        if (migrate_fd_cleanup(s) < 0) {
>>> +        if (s->get_status(s) != MIG_STATE_COMPLETED) {
>>>             if (old_vm_running) {
>>>                 vm_start();
>>>             }
>>
>> This part, although it's not fair to ask you, but calling
>> vm_start when != MIG_STATE_COMPLETED terrifies me because just
>> failing migrate_fd_cleanup (mostly calling qemu_fclose) may cause
>> split brain between src/dst.  Although I haven't encountered this
>> situation, just having stopped VMs on both sides is safer.
>
> I see your pain. I am not happy at all, but this was integrated by
> Anthony to fix this bug:
>
> commit 41ef56e61153d7bd27d34a634633bb51b1c5988d
> Author: Anthony Liguori <aliguori@us.ibm.com>
> Date:   Wed Jun 2 14:55:25 2010 -0500
>
>    migration: respect exit status with exec:
>
>  This fixes https://bugs.launchpad.net/qemu/+bug/391879
>

Thanks for the link.  I don't know IIUC, why stopping the VM was
a problem?  The essential thing is that we need to introduce a
flag that whether user wants to continue a VM when something goes
wrong during live migration.  Deciding only with old_vm_running is
wrong.

> I think that it fixes that bug, but it makes me un-easy to restart vm if
> there is a failure in migrate_fd_cleanup().  As I didn't wanted to
> change behaviour with this series, I left it as it was.

I agree with keeping the behavior unchanged.

> Next on ToDo list is to do something sensible with errors, just now we
> are not very good at handling them.

Yeah.  If we introduce Kemari, the migration code becomes more
important because it'll be part of the normal VM execution
path :)

Thanks,

Yoshi

>
> Later, Juan.
>
>

Re: [Qemu-devel] Re: [PATCH 22/22] migration: Make state definitions local

[Yoshiaki Tamura]

2011/2/23 Juan Quintela <quintela@redhat.com>:
> Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> wrote:
>> 2011/2/23 Juan Quintela <quintela@redhat.com>:
>>>
>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>> ---
>>>  migration.c |    6 ++++++
>>>  migration.h |    6 ------
>>>  2 files changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/migration.c b/migration.c
>>> index 383ebaf..90fc2a0 100644
>>> --- a/migration.c
>>> +++ b/migration.c
>>> @@ -31,6 +31,12 @@
>>>     do { } while (0)
>>>  #endif
>>>
>>> +#define MIG_STATE_ERROR                -1
>>> +#define MIG_STATE_NONE         0
>>> +#define MIG_STATE_CANCELLED    1
>>> +#define MIG_STATE_ACTIVE       2
>>> +#define MIG_STATE_COMPLETED    3
>>> +
>>>  static MigrationState current_migration = {
>>>     .state = MIG_STATE_NONE,
>>>      /* Migration speed throttling */
>>> diff --git a/migration.h b/migration.h
>>> index 9457807..493fbe5 100644
>>> --- a/migration.h
>>> +++ b/migration.h
>>> @@ -18,12 +18,6 @@
>>>  #include "qemu-common.h"
>>>  #include "notify.h"
>>>
>>> -#define MIG_STATE_ERROR                -1
>>> -#define MIG_STATE_NONE         0
>>> -#define MIG_STATE_CANCELLED    1
>>> -#define MIG_STATE_ACTIVE       2
>>> -#define MIG_STATE_COMPLETED    3
>>> -
>>
>> Although you're right, I would prefer to keep it so that somebody
>> outside of migration may understand the status in the future if
>> there are no harms.
>
> my plan is to move MigrationState inside migration.c, and then decide
> what to export/not export.

Well, it may be just a policy, but it's already exported, and I
would like to keep it unless it bothers your plan.  IIUC, I don't
think it does.

> Next thing to do is move migration to its
> own thread.  Before doing that, I need to know what parts are used/not
> used outside migration.c.  Removing it now means that nothing gets to
> use it without needing a patch.

I've once asked Anthony whether it's possible to make migration
to different threads, but his answer was no due to hard
dependency of qemu's internal code, and making migration to
different threads are bad design.

Thanks,

Yoshi

>
> Later, Juan..
>
>

[Qemu-devel] Re: [PATCH 22/22] migration: Make state definitions local

[Juan Quintela]

Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> wrote:
> 2011/2/23 Juan Quintela <quintela@redhat.com>:
>> Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> wrote:
>>> 2011/2/23 Juan Quintela <quintela@redhat.com>:

>>> Although you're right, I would prefer to keep it so that somebody
>>> outside of migration may understand the status in the future if
>>> there are no harms.
>>
>> my plan is to move MigrationState inside migration.c, and then decide
>> what to export/not export.
>
> Well, it may be just a policy, but it's already exported, and I
> would like to keep it unless it bothers your plan.  IIUC, I don't
> think it does.
>
>> Next thing to do is move migration to its
>> own thread.  Before doing that, I need to know what parts are used/not
>> used outside migration.c.  Removing it now means that nothing gets to
>> use it without needing a patch.
>
> I've once asked Anthony whether it's possible to make migration
> to different threads, but his answer was no due to hard
> dependency of qemu's internal code, and making migration to
> different threads are bad design.

I know.  But Anthony is seeing the light O:-)

Basically, without an own thread we are not able to:
- do anything else while on incoming migration
  (namely using the monitor)
- do anything else than migration.  We can try hard and let vcpus to
  run, but we would still clog the io_thread.
- We are not able to saturate 10Gbit networking (basically we are doing
  2/3 level of bufferering (depending on how you count).

So, once code is there, I guess we will convince Anthony to commit it.

Later, Juan.

Re: [Qemu-devel] Re: [PATCH 22/22] migration: Make state definitions local

[Anthony Liguori]

On 02/24/2011 06:23 AM, Juan Quintela wrote:
> Yoshiaki Tamura<tamura.yoshiaki@lab.ntt.co.jp>  wrote:
>    
>> 2011/2/23 Juan Quintela<quintela@redhat.com>:
>>      
>>> Yoshiaki Tamura<tamura.yoshiaki@lab.ntt.co.jp>  wrote:
>>>        
>>>> 2011/2/23 Juan Quintela<quintela@redhat.com>:
>>>>          
>    
>>>> Although you're right, I would prefer to keep it so that somebody
>>>> outside of migration may understand the status in the future if
>>>> there are no harms.
>>>>          
>>> my plan is to move MigrationState inside migration.c, and then decide
>>> what to export/not export.
>>>        
>> Well, it may be just a policy, but it's already exported, and I
>> would like to keep it unless it bothers your plan.  IIUC, I don't
>> think it does.
>>
>>      
>>> Next thing to do is move migration to its
>>> own thread.  Before doing that, I need to know what parts are used/not
>>> used outside migration.c.  Removing it now means that nothing gets to
>>> use it without needing a patch.
>>>        
>> I've once asked Anthony whether it's possible to make migration
>> to different threads, but his answer was no due to hard
>> dependency of qemu's internal code, and making migration to
>> different threads are bad design.
>>      
> I know.  But Anthony is seeing the light O:-)
>    

Let's be very careful about quoting Anthony as he's known to be 
incoherent 90% of the time :-)

I don't quite recall the context of the discussion with Yoshi, but I'm 
not quite there in terms of advocating that we throw a bucket full of 
threads at migration.  I think we should move the ram migration to 
another I/O thread that doesn't hold a lock against the main I/O 
thread.  That's all.

Regards,

Anthony Liguori

> Basically, without an own thread we are not able to:
> - do anything else while on incoming migration
>    (namely using the monitor)
> - do anything else than migration.  We can try hard and let vcpus to
>    run, but we would still clog the io_thread.
> - We are not able to saturate 10Gbit networking (basically we are doing
>    2/3 level of bufferering (depending on how you count).
>
> So, once code is there, I guess we will convince Anthony to commit it.
>
> Later, Juan.
>
>    

Re: [Qemu-devel] Re: [PATCH 22/22] migration: Make state definitions local

[Yoshiaki Tamura]

2011/2/24 Juan Quintela <quintela@redhat.com>:
> Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> wrote:
>> 2011/2/23 Juan Quintela <quintela@redhat.com>:
>>> Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp> wrote:
>>>> 2011/2/23 Juan Quintela <quintela@redhat.com>:
>
>>>> Although you're right, I would prefer to keep it so that somebody
>>>> outside of migration may understand the status in the future if
>>>> there are no harms.
>>>
>>> my plan is to move MigrationState inside migration.c, and then decide
>>> what to export/not export.
>>
>> Well, it may be just a policy, but it's already exported, and I
>> would like to keep it unless it bothers your plan.  IIUC, I don't
>> think it does.
>>
>>> Next thing to do is move migration to its
>>> own thread.  Before doing that, I need to know what parts are used/not
>>> used outside migration.c.  Removing it now means that nothing gets to
>>> use it without needing a patch.
>>
>> I've once asked Anthony whether it's possible to make migration
>> to different threads, but his answer was no due to hard
>> dependency of qemu's internal code, and making migration to
>> different threads are bad design.
>
> I know.  But Anthony is seeing the light O:-)

I'm with you at making live migration fast, but let me comment a
few.

> Basically, without an own thread we are not able to:
> - do anything else while on incoming migration
>  (namely using the monitor)

It's not true.  Just adding fixed headers to buffered file should
be enough for that.  I've done it for Kemari as you can see
this (http://www.osrg.net/kemari/download/kemari-v0.2-fedora11.mov).

> - do anything else than migration.  We can try hard and let vcpus to
>  run, but we would still clog the io_thread.
> - We are not able to saturate 10Gbit networking (basically we are doing
>  2/3 level of bufferering (depending on how you count).

I think current byte-based dirty bitmap for sending rams are
responsible for this too.  I've converted it to bit-based dirty
and made traversing 100x faster.  Also bypassing QEMUFile buffer
in case of rams would boost to some degrees.

Thanks,

Yoshi

> So, once code is there, I guess we will convince Anthony to commit it.
>
> Later, Juan.
>
>

Re: [Qemu-devel] Re: [PATCH 22/22] migration: Make state definitions local

[Yoshiaki Tamura]

2011/2/24 Anthony Liguori <anthony@codemonkey.ws>:
> On 02/24/2011 06:23 AM, Juan Quintela wrote:
>>
>> Yoshiaki Tamura<tamura.yoshiaki@lab.ntt.co.jp>  wrote:
>>
>>>
>>> 2011/2/23 Juan Quintela<quintela@redhat.com>:
>>>
>>>>
>>>> Yoshiaki Tamura<tamura.yoshiaki@lab.ntt.co.jp>  wrote:
>>>>
>>>>>
>>>>> 2011/2/23 Juan Quintela<quintela@redhat.com>:
>>>>>
>>
>>
>>>>>
>>>>> Although you're right, I would prefer to keep it so that somebody
>>>>> outside of migration may understand the status in the future if
>>>>> there are no harms.
>>>>>
>>>>
>>>> my plan is to move MigrationState inside migration.c, and then decide
>>>> what to export/not export.
>>>>
>>>
>>> Well, it may be just a policy, but it's already exported, and I
>>> would like to keep it unless it bothers your plan.  IIUC, I don't
>>> think it does.
>>>
>>>
>>>>
>>>> Next thing to do is move migration to its
>>>> own thread.  Before doing that, I need to know what parts are used/not
>>>> used outside migration.c.  Removing it now means that nothing gets to
>>>> use it without needing a patch.
>>>>
>>>
>>> I've once asked Anthony whether it's possible to make migration
>>> to different threads, but his answer was no due to hard
>>> dependency of qemu's internal code, and making migration to
>>> different threads are bad design.
>>>
>>
>> I know.  But Anthony is seeing the light O:-)
>>
>
> Let's be very careful about quoting Anthony as he's known to be incoherent
> 90% of the time :-)

There you are :)

> I don't quite recall the context of the discussion with Yoshi, but I'm not
> quite there in terms of advocating that we throw a bucket full of threads at
> migration.  I think we should move the ram migration to another I/O thread
> that doesn't hold a lock against the main I/O thread.  That's all.

Let's just forget about the old discussion and have a new one.
Why do you want to have a new thread only for ram migration?  I
know that it's the majority of the migration, but how do you
serialize it with other device migration for single QEMUFile?
IIUC, it seems to get mixed up easily or lose paralism.

Thanks,

Yoshi

>
> Regards,
>
> Anthony Liguori
>
>> Basically, without an own thread we are not able to:
>> - do anything else while on incoming migration
>>   (namely using the monitor)
>> - do anything else than migration.  We can try hard and let vcpus to
>>   run, but we would still clog the io_thread.
>> - We are not able to saturate 10Gbit networking (basically we are doing
>>   2/3 level of bufferering (depending on how you count).
>>
>> So, once code is there, I guess we will convince Anthony to commit it.
>>
>> Later, Juan.
>>
>>
>
>
>