Skip to contentSkip to conversation

Refactor and cleanup migration code

PATCH Superseded Juan Quintela 6 participants 41 messages Feb 23, 2011 – Apr 2, 2011

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

v2 cover Refactor and cleanup migration code

v2:
- make Jan^Wcheckpatch.pl happy
- Yoshiaki Tamura suggestions:
- include its two patches to clean things
- MAX_THROTTLE define
- migration_state enum
- I removed spurious differences between migration-{tcp,unix}
- better error propagation, after this patch:
migrate -d "tcp:name_don_exist:port"
migrate -d "tcp:name:port_dont_exist"
migrate -d "exec: prog_dont_exist"
migrate -d "exec: gzip > /path/dont/exist"
fail as expected. Last two used to enter an infinite loop.

Later, Juan.

v1:
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 (26):
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: 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: Don't use callback on file defining it
migration: propagate error correctly
migration: qemu_savevm_iterate has three return values
migration: If there is one error, it makes no sense to continue
migration: make migration-{tcp,unix} consistent

Yoshiaki Tamura (2):
savevm: avoid qemu_savevm_state_iteate() to return 1 when qemu file
has error.
migration: add error handling to migrate_fd_put_notify().

 buffered_file.c  |    2 +-
 migration-exec.c |   39 +----
 migration-fd.c   |   42 ++-----
 migration-tcp.c  |   76 ++++------
 migration-unix.c |  112 ++++++---------
 migration.c      |  407 ++++++++++++++++++++++++++----------------------------
 migration.h      |   85 ++----------
 savevm.c         |    7 +-
 ui/spice-core.c  |    4 +-
 9 files changed, 307 insertions(+), 467 deletions(-)
Signature · 1 line
1.7.4
#1 lore raw

PATCH v2 1/28 migration: Make *start_outgoing_migration return FdMigrationState

---
 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(-)
migration-exec.c +2 -2
index 14718dd..b49a475 100644
@@ -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);
migration-fd.c +2 -2
index 6d14505..bd5e8a9 100644
@@ -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);
migration-tcp.c +2 -2
index b55f419..355bc37 100644
@@ -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)
migration-unix.c +2 -2
index 57232c0..b9b0dbf 100644
@@ -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);
migration.c +2 -2
index af3a1f2..f9aaadc 100644
@@ -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;
 }
migration.h +4 -4
index 2170792..db0e45a 100644
@@ -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,
Signature · 1 line
1.7.4
#2 lore raw

PATCH v2 2/28 migration: Use FdMigrationState instead of MigrationState when possible

In reply to Juan Quintela
---
 migration.c |   32 +++++++++++++++-----------------
 migration.h |   16 ++++++++--------
 2 files changed, 23 insertions(+), 25 deletions(-)
migration.c +15 -17
index f9aaadc..7d7a2f9 100644
@@ -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,8 @@ 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 +121,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 +150,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 +228,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 +401,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 +420,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");
    
migration.h +8 -8
index db0e45a..f49a9e2 100644
@@ -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);
 
Signature · 1 line
1.7.4
#3 lore raw

PATCH v2 3/28 migration: Fold MigrationState into FdMigrationState

In reply to Juan Quintela
---
 migration-exec.c |   10 +++++-----
 migration-fd.c   |   10 +++++-----
 migration-tcp.c  |   10 +++++-----
 migration-unix.c |   10 +++++-----
 migration.c      |   12 +++++-------
 migration.h      |   23 +++++------------------
 6 files changed, 30 insertions(+), 45 deletions(-)
migration-exec.c +5 -5
index b49a475..02b0667 100644
@@ -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;
migration-fd.c +5 -5
index bd5e8a9..ccba86b 100644
@@ -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;
migration-tcp.c +5 -5
index 355bc37..02b01ed 100644
@@ -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;
migration-unix.c +5 -5
index b9b0dbf..fb73f46 100644
@@ -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;
migration.c +5 -7
index 7d7a2f9..dd4cdab 100644
@@ -86,8 +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;
     }
@@ -121,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;
@@ -134,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;
 }
@@ -230,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:
@@ -354,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);
migration.h +5 -18
index f49a9e2..93441e4 100644
@@ -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);
Signature · 1 line
1.7.4
#4 lore raw

PATCH v2 4/28 migration: Rename FdMigrationState MigrationState

In reply to Juan Quintela
---
 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(-)
migration-exec.c +5 -5
index 02b0667..e91e1e2 100644
@@ -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));
migration-fd.c +5 -5
index ccba86b..4073000 100644
@@ -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));
 
migration-tcp.c +6 -6
index 02b01ed..e8fa46c 100644
@@ -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)
migration-unix.c +6 -6
index fb73f46..9b74401 100644
@@ -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;
 
migration.c +17 -17
index dd4cdab..9ad45d6 100644
@@ -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);
migration.h +19 -19
index 93441e4..6e474fc 100644
@@ -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 *s);
+    int (*close)(MigrationState *s);
+    int (*write)(MigrationState *s, const void *buffer, size_t size);
+    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);
 
Signature · 1 line
1.7.4
#5 lore raw

PATCH v2 5/28 migration: Refactor MigrationState creation

In reply to Juan Quintela
---
 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(-)
migration-exec.c +1 -15
index e91e1e2..67dec27 100644
@@ -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;
migration-fd.c +1 -15
index 4073000..1deedd7 100644
@@ -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;
migration-tcp.c +1 -14
index e8fa46c..c1c7bc3 100644
@@ -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)
migration-unix.c +1 -14
index 9b74401..94995af 100644
@@ -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);
 
migration.c +25 -4
index 9ad45d6..e773806 100644
@@ -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;
+}
migration.h +3 -8
index 6e474fc..fe57eac 100644
@@ -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);
Signature · 1 line
1.7.4
#6 lore raw

PATCH v2 6/28 migration: Make all posible migration functions static

In reply to Juan Quintela

I have to move two functions postions to avoid forward declarations

---
 migration.c |   73 ++++++++++++++++++++++++++++++-----------------------------
 migration.h |   12 ---------
 2 files changed, 37 insertions(+), 48 deletions(-)
migration.c +37 -36
index e773806..84a33ae 100644
@@ -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,8 @@ 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 +342,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 +410,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 +429,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 +456,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)
 {
migration.h +0 -12
index fe57eac..52cf2fa 100644
@@ -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);
 
Signature · 1 line
1.7.4
#7 lore raw

PATCH v2 7/28 migration: move migrate_create_state to do_migrate

In reply to Juan Quintela

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

---
 migration-exec.c |   19 +++++--------------
 migration-fd.c   |   22 ++++++----------------
 migration-tcp.c  |   22 +++++++---------------
 migration-unix.c |   19 +++++--------------
 migration.c      |   34 +++++++++++++++++++++-------------
 migration.h      |   31 ++++---------------------------
 6 files changed, 48 insertions(+), 99 deletions(-)
migration-exec.c +5 -14
index 67dec27..ed0798b 100644
@@ -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)
migration-fd.c +6 -16
index 1deedd7..e2dd61f 100644
@@ -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)
migration-tcp.c +7 -15
index c1c7bc3..9418abd 100644
@@ -76,30 +76,22 @@ 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;
     s->close = tcp_close;
 
     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)
migration-unix.c +5 -14
index 94995af..d562456 100644
@@ -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)
migration.c +21 -13
index 84a33ae..1105757 100644
@@ -76,6 +76,10 @@ 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 +88,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 +100,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 +130,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)
@@ -478,8 +485,9 @@ 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));
 
migration.h +4 -27
index 52cf2fa..b633f55 100644
@@ -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);
Signature · 1 line
1.7.4
#8 lore raw

PATCH v2 8/28 migration: Check that migration is active before cancel it

In reply to Juan Quintela
---
 migration.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
migration.c +2 -2
index 1105757..e8e593d 100644
@@ -139,9 +139,9 @@ 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;
 }
 
Signature · 1 line
1.7.4
#9 lore raw

PATCH v2 9/28 migration: Introduce MIG_STATE_NONE

In reply to Juan Quintela

Use MIG_STATE_ACTIVE only when migration has really started

---
 migration.c |    6 +++++-
 migration.h |   11 +++++++----
 2 files changed, 12 insertions(+), 5 deletions(-)
migration.c +5 -1
index e8e593d..45cc263 100644
@@ -239,6 +239,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"));
@@ -467,6 +470,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,
@@ -498,7 +502,7 @@ static MigrationState *migrate_create_state(Monitor *mon,
     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);
migration.h +7 -4
index b633f55..266c9f0 100644
@@ -18,10 +18,13 @@
 #include "qemu-common.h"
 #include "notify.h"
 
-#define MIG_STATE_ERROR		-1
-#define MIG_STATE_COMPLETED	0
-#define MIG_STATE_CANCELLED	1
-#define MIG_STATE_ACTIVE	2
+enum migration_state {
+    MIG_STATE_ERROR,
+    MIG_STATE_NONE,
+    MIG_STATE_CANCELLED,
+    MIG_STATE_ACTIVE,
+    MIG_STATE_COMPLETED,
+};
 
 typedef struct MigrationState MigrationState;
 
Signature · 1 line
1.7.4
#10 lore raw

PATCH v2 10/28 migration: Refactor and simplify error checking in migrate_fd_put_ready

In reply to Juan Quintela
---
 migration.c |   20 +++++++++-----------
 1 files changed, 9 insertions(+), 11 deletions(-)
migration.c +9 -11
index 45cc263..ed6e626 100644
@@ -363,28 +363,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);
     }
 }
 
Signature · 1 line
1.7.4
#11 lore raw

PATCH v2 11/28 migration: Introduce migrate_fd_completed() for consistenncy

In reply to 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.

---
 migration.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)
migration.c +12 -6
index ed6e626..9be6bb0 100644
@@ -318,6 +318,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;
@@ -371,12 +382,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) {
Signature · 1 line
1.7.4
#12 lore raw

PATCH v2 12/28 migration: Our release callback was just free

In reply to 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).

---
 migration.c |   19 +------------------
 migration.h |    1 -
 2 files changed, 1 insertions(+), 19 deletions(-)
migration.c +1 -18
index 9be6bb0..936b8f1 100644
@@ -123,10 +123,7 @@ int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
         goto free_migrate_state;
     }
 
-    if (current_migration) {
-        current_migration->release(current_migration);
-    }
-
+    qemu_free(current_migration);
     current_migration = s;
     notifier_list_notify(&migration_state_notifiers);
     return 0;
@@ -411,19 +408,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;
@@ -501,7 +485,6 @@ static MigrationState *migrate_create_state(Monitor *mon,
 
     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;
migration.h +0 -1
index 266c9f0..b90e344 100644
@@ -40,7 +40,6 @@ struct MigrationState
     int (*write)(MigrationState *s, const void *buffer, size_t size);
     void (*cancel)(MigrationState *s);
     int (*get_status)(MigrationState *s);
-    void (*release)(MigrationState *s);
     void *opaque;
     int blk;
     int shared;
Signature · 1 line
1.7.4
#13 lore raw

PATCH v2 13/28 migration: Remove get_status() accessor

In reply to Juan Quintela

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

---
 migration.c |   16 +++++-----------
 migration.h |    1 -
 2 files changed, 5 insertions(+), 12 deletions(-)
migration.c +5 -11
index 936b8f1..c1732e5 100644
@@ -91,7 +91,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;
     }
@@ -136,7 +136,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;
@@ -235,7 +235,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;
@@ -381,7 +381,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();
             }
@@ -389,11 +389,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)
@@ -448,7 +443,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;
     }
@@ -484,7 +479,6 @@ static MigrationState *migrate_create_state(Monitor *mon,
     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;
migration.h +0 -1
index b90e344..7016710 100644
@@ -39,7 +39,6 @@ struct MigrationState
     int (*close)(MigrationState *s);
     int (*write)(MigrationState *s, const void *buffer, size_t size);
     void (*cancel)(MigrationState *s);
-    int (*get_status)(MigrationState *s);
     void *opaque;
     int blk;
     int shared;
Signature · 1 line
1.7.4
#14 lore raw

PATCH v2 14/28 migration: Remove migration cancel() callback

In reply to Juan Quintela

It is used only in one place

---
 migration.c |    9 ++++-----
 migration.h |    1 -
 2 files changed, 4 insertions(+), 6 deletions(-)
migration.c +4 -5
index c1732e5..dba9034 100644
@@ -132,12 +132,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;
 }
@@ -478,7 +478,6 @@ static MigrationState *migrate_create_state(Monitor *mon,
 {
     MigrationState *s = qemu_mallocz(sizeof(*s));
 
-    s->cancel = migrate_fd_cancel;
     s->blk = blk;
     s->shared = inc;
     s->mon = NULL;
migration.h +0 -1
index 7016710..347e7e8 100644
@@ -38,7 +38,6 @@ struct MigrationState
     int (*get_error)(MigrationState *s);
     int (*close)(MigrationState *s);
     int (*write)(MigrationState *s, const void *buffer, size_t size);
-    void (*cancel)(MigrationState *s);
     void *opaque;
     int blk;
     int shared;
Signature · 1 line
1.7.4
#15 lore raw

PATCH v2 15/28 migration: Move exported functions to the end of the file

In reply to Juan Quintela

This means we can remove the two forward declarations.

---
 migration.c |  189 +++++++++++++++++++++++++++++------------------------------
 1 files changed, 92 insertions(+), 97 deletions(-)
migration.c +92 -97
index dba9034..caf5044 100644
@@ -76,91 +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
@@ -172,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)
 {
@@ -490,3 +393,95 @@ static MigrationState *migrate_create_state(Monitor *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);
+    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;
+}
+
Signature · 1 line
1.7.4
#16 lore raw

PATCH v2 16/28 migration: use global variable directly

In reply to 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.

---
 migration.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)
migration.c +2 -4
index caf5044..21f5a9a 100644
@@ -457,7 +457,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) {
@@ -465,9 +464,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;
Signature · 1 line
1.7.4
#17 lore raw

Re: [Qemu-devel] [PATCH 16/28] migration: use global variable directly

Quoted 4 lines · 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.

I think this is bad form generally speaking. Globals are not something to be embraced but rather to be isolated as much as humanly possible.

Regards,

Anthony Liguori

Quoted 30 lines
---
  migration.c |    6 ++----
  1 files changed, 2 insertions(+), 4 deletions(-)
migration.c +2 -4
index caf5044..21f5a9a 100644
@@ -457,7 +457,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) {
@@ -465,9 +464,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;
#18 lore raw

[Qemu-devel] Re: [PATCH 16/28] migration: use global variable directly

Quoted 9 lines · Anthony Liguori
Quoted 4 lines · 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.

I think this is bad form generally speaking. Globals are not something to be embraced but rather to be isolated as much as humanly possible.

current_migration is a global variable.

And just doing:

s = current_migration;

foo(s);

helps nothing. We are not going to happen more than one migration at the same time anytime soon. Notice that everything that is outside of migration.c already receives a pointer to it, i.e. not use the global one. So, I claim this is a very special case, not the normal case about global variables.

Later, Juan.

Quoted 34 lines

Regards,

Anthony Liguori

Quoted 30 lines
---
  migration.c |    6 ++----
  1 files changed, 2 insertions(+), 4 deletions(-)
migration.c +2 -4
index caf5044..21f5a9a 100644
@@ -457,7 +457,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) {
@@ -465,9 +464,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;
#19 lore raw

[Qemu-devel] Re: [PATCH 16/28] migration: use global variable directly

Quoted 15 lines · Juan Quintela
Quoted 11 lines · Anthony Liguori
Quoted 5 lines · 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.

I think this is bad form generally speaking. Globals are not something to be embraced but rather to be isolated as much as humanly possible.

current_migration is a global variable.

Quoted 7 lines

And just doing:

s = current_migration;

foo(s);

helps nothing.

It's still bad form IMHO. You should always use local variables to reference global variables unless you're explicitly setting a global variable.

Regards,

Anthony Liguori

Quoted 45 lines

We are not going to happen more than one migration at
the same time anytime soon. Notice that everything that is outside of migration.c already receives a pointer to it, i.e. not use the global one. So, I claim this is a very special case, not the normal case about global variables.

Later, Juan.

Quoted 36 lines

Regards,

Anthony Liguori

Quoted 31 lines
---
   migration.c |    6 ++----
   1 files changed, 2 insertions(+), 4 deletions(-)
migration.c +2 -4
index caf5044..21f5a9a 100644
@@ -457,7 +457,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) {
@@ -465,9 +464,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;
#20 lore raw

[Qemu-devel] Re: [PATCH 16/28] migration: use global variable directly

Quoted 20 lines · Anthony Liguori
Quoted 19 lines · Juan Quintela
Quoted 11 lines · Anthony Liguori
Quoted 5 lines · 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.

I think this is bad form generally speaking. Globals are not something to be embraced but rather to be isolated as much as humanly possible.

current_migration is a global variable.

And just doing:

s = current_migration;

Quoted 7 lines
Quoted 3 lines

foo(s);

helps nothing.

It's still bad form IMHO. You should always use local variables to reference global variables unless you're explicitly setting a global variable.

Obviounly tastes change between us :-(

If I want to do that, instead of creating a local variable, I will just add a parameter to the function and make the callers to pass it.

For me, a variable in a function is global or local, hidden it as a local has no merit.

Later, Juan.

#21 lore raw

Re: [Qemu-devel] Re: [PATCH 16/28] migration: use global variable directly

In reply to Anthony Liguori
Quoted 20 lines · Anthony Liguori
Quoted 19 lines · Juan Quintela
Quoted 11 lines · Anthony Liguori
Quoted 5 lines · 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.

I think this is bad form generally speaking. Globals are not something to be embraced but rather to be isolated as much as humanly possible.

current_migration is a global variable.

And just doing:

s = current_migration;

Quoted 7 lines
Quoted 3 lines

foo(s);

helps nothing.

It's still bad form IMHO. You should always use local variables to reference global variables unless you're explicitly setting a global variable.

I disagree. The use of global variables should be made as painfully explicit as possible. Hiding them behind local pointers is sweeping the globals under the rug.

For completeness: a local variable may be necessary to convince the optimizer that the value doesn't change. Cases where this matters exist, but they're rare.

#22 lore raw

[Qemu-devel] Re: [PATCH 16/28] migration: use global variable directly

Quoted 3 lines · Markus Armbruster

For completeness: a local variable may be necessary to convince the optimizer that the value doesn't change. Cases where this matters exist, but they're rare.

In particular, for non-pointers they're nonexistent if the variable is static and you never use &current_migration, i.e. if the variable doesn't escape.

If the above conditions are satisfied and you have a loop that never "leaves" the C file, you could even see the compiler keep the variable in a register for the whole duration of the loop and store the modified change after the loop.

Paolo

#23 lore raw

PATCH v2 17/28 migration: another case of global variable assigned to local one

In reply to Juan Quintela
---
 migration.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
migration.c +1 -2
index 21f5a9a..593adee 100644
@@ -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;
Signature · 1 line
1.7.4
#24 lore raw

PATCH v2 18/28 migration: convert current_migration from pointer to struct

In reply to Juan Quintela

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

---
 migration.c |  121 ++++++++++++++++++++++++-----------------------------------
 1 files changed, 49 insertions(+), 72 deletions(-)
migration.c +49 -72
index 593adee..f8c6d09 100644
@@ -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;
     }
 }
 
@@ -344,11 +343,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)
@@ -374,28 +369,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);
@@ -403,8 +392,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;
     }
@@ -413,43 +401,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;
 }
 
@@ -463,10 +443,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;
 }
 
Signature · 1 line
1.7.4
#25 lore raw

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

Quoted 5 lines · Juan Quintela

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

Yeah, but now you have to check for MIG_STATE_NONE. I don't think this is an improvement.

Regards,

Anthony Liguori

Quoted 209 lines
---
  migration.c |  121 ++++++++++++++++++++++++-----------------------------------
  1 files changed, 49 insertions(+), 72 deletions(-)
migration.c +49 -72
index 593adee..f8c6d09 100644
@@ -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;
      }
  }
 
@@ -344,11 +343,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)
@@ -374,28 +369,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);
@@ -403,8 +392,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;
      }
@@ -413,43 +401,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;
  }
 
@@ -463,10 +443,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;
  }
 
#26 lore raw

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

Quoted 9 lines · Anthony Liguori
Quoted 5 lines · Juan Quintela

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

Yeah, but now you have to check for MIG_STATE_NONE. I don't think this is an improvement.

We can:
a- remove max_throttle global (now we always have this variable to have
fields in).
b- transitioning to ACTIVE state is a normal transition (if you look,
you will see that it is the only case where we call the migration notifier without an explicit change in state.
c- We remove all the cases of:
if (current_migration)
foo(current_migration)

Notice that we don't check against MIG_STATE_NONE in the whole patch.

Later, Juan.

Quoted 213 lines

Regards,

Anthony Liguori

Quoted 209 lines
---
  migration.c |  121 ++++++++++++++++++++++++-----------------------------------
  1 files changed, 49 insertions(+), 72 deletions(-)
migration.c +49 -72
index 593adee..f8c6d09 100644
@@ -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;
      }
  }
 
@@ -344,11 +343,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)
@@ -374,28 +369,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);
@@ -403,8 +392,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;
      }
@@ -413,43 +401,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;
  }
 
@@ -463,10 +443,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;
  }
 
#27 lore raw

PATCH v2 19/28 migration: Use bandwidth_limit directly

In reply to Juan Quintela

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

---
 migration.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)
migration.c +7 -8
index f8c6d09..49cdf72 100644
@@ -31,11 +31,11 @@
     do { } while (0)
 #endif
 
-/* Migration speed throttling */
-static int64_t max_throttle = (32 << 20);
+#define MAX_THROTTLE  (32 << 20)      /* Migration speed throttling */
 
 static MigrationState current_migration = {
     .state = MIG_STATE_NONE,
+    .bandwidth_limit = MAX_THROTTLE,
 };
 
 static NotifierList migration_state_notifiers =
@@ -369,13 +369,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) {
@@ -401,7 +399,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);
@@ -441,9 +439,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;
 }
 
Signature · 1 line
1.7.4
#28 lore raw

PATCH v2 20/28 migration: Export a function that tells if the migration has finished correctly

In reply to Juan Quintela

This will allows us to hide the status values.

---
 migration.c     |    4 ++--
 migration.h     |    2 +-
 ui/spice-core.c |    4 +---
 3 files changed, 4 insertions(+), 6 deletions(-)
migration.c +2 -2
index 49cdf72..493c2d7 100644
@@ -341,9 +341,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)
migration.h +1 -1
index 347e7e8..3c5bb6a 100644
@@ -84,6 +84,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
ui/spice-core.c +1 -3
index 1aa1a5e..997603d 100644
@@ -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
Signature · 1 line
1.7.4
#29 lore raw

PATCH v2 21/28 migration: Make state definitions local

In reply to Juan Quintela
---
 migration.c |    8 ++++++++
 migration.h |    8 --------
 2 files changed, 8 insertions(+), 8 deletions(-)
migration.c +8 -0
index 493c2d7..697c74f 100644
@@ -31,6 +31,14 @@
     do { } while (0)
 #endif
 
+enum migration_state {
+    MIG_STATE_ERROR,
+    MIG_STATE_NONE,
+    MIG_STATE_CANCELLED,
+    MIG_STATE_ACTIVE,
+    MIG_STATE_COMPLETED,
+};
+
 #define MAX_THROTTLE  (32 << 20)      /* Migration speed throttling */
 
 static MigrationState current_migration = {
migration.h +0 -8
index 3c5bb6a..e1fc921 100644
@@ -18,14 +18,6 @@
 #include "qemu-common.h"
 #include "notify.h"
 
-enum migration_state {
-    MIG_STATE_ERROR,
-    MIG_STATE_NONE,
-    MIG_STATE_CANCELLED,
-    MIG_STATE_ACTIVE,
-    MIG_STATE_COMPLETED,
-};
-
 typedef struct MigrationState MigrationState;
 
 struct MigrationState
Signature · 1 line
1.7.4
#30 lore raw

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

2011/2/24 Juan Quintela <[email protected]>:

Quoted 12 lines

---
 migration.c |    8 ++++++++
 migration.h |    8 --------

 2 files changed, 8 insertions(+), 8 deletions(-)
migration.c +0 -0
index 493c2d7..697c74f 100644

migration.c line 35

Quoted 5 lines
migration.c +2 -0
@@ -31,6 +31,14 @@
     do { } while (0)
  #endif
 
+enum migration_state {
+    MIG_STATE_ERROR,

Would be better to say:

MIG_STATE_ERROR = -1,

Yoshi

Quoted 33 lines
migration.c +6 -0
+    MIG_STATE_NONE,
+    MIG_STATE_CANCELLED,
+    MIG_STATE_ACTIVE,
+    MIG_STATE_COMPLETED,
+};
+
  #define MAX_THROTTLE  (32 << 20)      /* Migration speed throttling */
 
  static MigrationState current_migration = {
migration.h +0 -8
index 3c5bb6a..e1fc921 100644
@@ -18,14 +18,6 @@
  #include "qemu-common.h"
  #include "notify.h"
 
-enum migration_state {
-    MIG_STATE_ERROR,
-    MIG_STATE_NONE,
-    MIG_STATE_CANCELLED,
-    MIG_STATE_ACTIVE,
-    MIG_STATE_COMPLETED,
-};
-
  typedef struct MigrationState MigrationState;
 

 struct MigrationState
--
1.7.4

#31 lore raw

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

Quoted 16 lines · Yoshiaki Tamura

2011/2/24 Juan Quintela <[email protected]>:

Quoted 15 lines

---
 migration.c |    8 ++++++++
 migration.h |    8 --------

 2 files changed, 8 insertions(+), 8 deletions(-)
migration.c +0 -0
index 493c2d7..697c74f 100644
@@ -31,6 +31,14 @@
     do { } while (0)
  #endif
 
Quoted 6 lines

migration.c line 35

Quoted 2 lines
migration.c +2 -0
+enum migration_state {
+    MIG_STATE_ERROR,

Would be better to say:

MIG_STATE_ERROR = -1,

I thought about it, but basically it shouldn't matter, no?

Later, Juan.

#32 lore raw

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

Quoted 17 lines · Juan Quintela
Quoted 16 lines · Yoshiaki Tamura

2011/2/24 Juan Quintela<[email protected]>:

Quoted 15 lines
---
  migration.c |    8 ++++++++
  migration.h |    8 --------
  2 files changed, 8 insertions(+), 8 deletions(-)
migration.c +0 -0
index 493c2d7..697c74f 100644
@@ -31,6 +31,14 @@
     do { } while (0)
  #endif
 
Quoted 8 lines
Quoted 6 lines

migration.c line 35

Quoted 2 lines
migration.c +2 -0
+enum migration_state {
+    MIG_STATE_ERROR,

Would be better to say:

MIG_STATE_ERROR = -1,

I thought about it, but basically it shouldn't matter, no?

It shouldn't. Just gives clear impression :)

Yoshi

Quoted 4 lines

Later, Juan.

#33 lore raw

PATCH v2 22/28 savevm: avoid qemu_savevm_state_iteate() to return 1 when qemu file has error.

In reply to Juan Quintela

From: Yoshiaki Tamura <[email protected]>

When qemu on the receiver gets killed during live migration, if debug is turned on, migrate_fd_put_ready() says,

migration: done iterating

and proceeds. The reason was qemu_savevm_state_iterate() returning 1 even when qemu file has error. This patch checks qemu_file_has_error() before returning 1/0, and avoids migrate_fd_put_ready() to proceed in case of error.

---
 savevm.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
savevm.c +4 -3
index a50fd31..1a0be58 100644
@@ -1501,14 +1501,15 @@ int qemu_savevm_state_iterate(Monitor *mon, QEMUFile *f)
         }
     }
 
-    if (ret)
-        return 1;
-
     if (qemu_file_has_error(f)) {
         qemu_savevm_state_cancel(mon, f);
         return -EIO;
     }
 
+    if (ret) {
+        return 1;
+    }
+
     return 0;
 }
 
Signature · 1 line
1.7.4
#34 lore raw

PATCH v2 23/28 migration: add error handling to migrate_fd_put_notify().

In reply to Juan Quintela

From: Yoshiaki Tamura <[email protected]>

Although migrate_fd_put_buffer() sets MIG_STATE_ERROR if it failed, since migrate_fd_put_notify() isn't checking error of underlying QEMUFile, those resources are kept open. This patch checks it and calls migrate_fd_error() in case of error.

---
 migration.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)
migration.c +3 -6
index 697c74f..a02100b 100644
@@ -241,6 +241,9 @@ static void migrate_fd_put_notify(void *opaque)
 
     qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
     qemu_file_put_notify(s->file);
+    if (qemu_file_has_error(s->file)) {
+        migrate_fd_error(s);
+    }
 }
 
 static ssize_t migrate_fd_put_buffer(void *opaque, const void *data,
@@ -258,12 +261,6 @@ static ssize_t migrate_fd_put_buffer(void *opaque, const void *data,
 
     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);
     }
 
     return ret;
Signature · 1 line
1.7.4
#35 lore raw

PATCH v2 24/28 migration: Don't use callback on file defining it

In reply to Juan Quintela
---
 migration-tcp.c  |    4 ++--
 migration-unix.c |    6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)
migration-tcp.c +2 -2
index 9418abd..6acf6c6 100644
@@ -59,7 +59,7 @@ static void tcp_wait_for_connect(void *opaque)
     DPRINTF("connect completed\n");
     do {
         ret = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, &valsize);
-    } while (ret == -1 && (s->get_error(s)) == EINTR);
+    } while (ret == -1 && (socket_error()) == EINTR);
 
     if (ret < 0) {
         migrate_fd_error(s);
@@ -99,7 +99,7 @@ int tcp_start_outgoing_migration(MigrationState *s, const char *host_port)
     do {
         ret = connect(s->fd, (struct sockaddr *)&addr, sizeof(addr));
         if (ret == -1)
-            ret = -(s->get_error(s));
+            ret = -(socket_error());
 
         if (ret == -EINPROGRESS || ret == -EWOULDBLOCK)
             qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
migration-unix.c +3 -3
index d562456..f9e7266 100644
@@ -58,7 +58,7 @@ static void unix_wait_for_connect(void *opaque)
     DPRINTF("connect completed\n");
     do {
         ret = getsockopt(s->fd, SOL_SOCKET, SO_ERROR, (void *) &val, &valsize);
-    } while (ret == -1 && (s->get_error(s)) == EINTR);
+    } while (ret == -1 && errno == EINTR);
 
     if (ret < 0) {
         migrate_fd_error(s);
@@ -98,7 +98,7 @@ int unix_start_outgoing_migration(MigrationState *s, const char *path)
     do {
         ret = connect(s->fd, (struct sockaddr *)&addr, sizeof(addr));
         if (ret == -1)
-	    ret = -(s->get_error(s));
+            ret = -errno;
 
         if (ret == -EINPROGRESS || ret == -EWOULDBLOCK)
 	    qemu_set_fd_handler2(s->fd, NULL, NULL, unix_wait_for_connect, s);
@@ -131,7 +131,7 @@ static void unix_accept_incoming_migration(void *opaque)
 
     do {
         c = qemu_accept(s, (struct sockaddr *)&addr, &addrlen);
-    } while (c == -1 && socket_error() == EINTR);
+    } while (c == -1 && errno == EINTR);
 
     DPRINTF("accepted migration\n");
 
Signature · 1 line
1.7.4
#36 lore raw

PATCH v2 25/28 migration: propagate error correctly

In reply to Juan Quintela

unix and tcp outgoing migration have error values, but didn't returned it. Make them return the error. Notice that EINPROGRESS & EWOULDBLOCK are not considered errors as callwill be retry later.

---
 migration-tcp.c  |   20 +++++++++++---------
 migration-unix.c |   26 ++++++++++----------------
 2 files changed, 21 insertions(+), 25 deletions(-)
migration-tcp.c +11 -9
index 6acf6c6..ffe8ed6 100644
@@ -91,26 +91,28 @@ int tcp_start_outgoing_migration(MigrationState *s, const char *host_port)
 
     s->fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (s->fd == -1) {
-        return -1;
+        return -socket_error();
     }
 
     socket_set_nonblock(s->fd);
 
     do {
         ret = connect(s->fd, (struct sockaddr *)&addr, sizeof(addr));
-        if (ret == -1)
-            ret = -(socket_error());
-
-        if (ret == -EINPROGRESS || ret == -EWOULDBLOCK)
+        if (ret == -1) {
+            ret = -socket_error();
+        }
+        if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
             qemu_set_fd_handler2(s->fd, NULL, NULL, tcp_wait_for_connect, s);
+            return 0;
+        }
     } while (ret == -EINTR);
 
-    if (ret < 0 && ret != -EINPROGRESS && ret != -EWOULDBLOCK) {
+    if (ret < 0) {
         DPRINTF("connect failed\n");
         migrate_fd_error(s);
-    } else if (ret >= 0)
-        migrate_fd_connect(s);
-
+        return ret;
+    }
+    migrate_fd_connect(s);
     return 0;
 }
 
migration-unix.c +10 -16
index f9e7266..8563bbb 100644
@@ -90,35 +90,29 @@ int unix_start_outgoing_migration(MigrationState *s, const char *path)
     s->fd = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
     if (s->fd < 0) {
         DPRINTF("Unable to open socket");
-        goto err_after_socket;
+        return -errno;
     }
 
     socket_set_nonblock(s->fd);
 
     do {
         ret = connect(s->fd, (struct sockaddr *)&addr, sizeof(addr));
-        if (ret == -1)
+        if (ret == -1) {
             ret = -errno;
-
-        if (ret == -EINPROGRESS || ret == -EWOULDBLOCK)
+        }
+        if (ret == -EINPROGRESS || ret == -EWOULDBLOCK) {
 	    qemu_set_fd_handler2(s->fd, NULL, NULL, unix_wait_for_connect, s);
+            return 0;
+        }
     } while (ret == -EINTR);
 
-    if (ret < 0 && ret != -EINPROGRESS && ret != -EWOULDBLOCK) {
+    if (ret < 0) {
         DPRINTF("connect failed\n");
-        goto err_after_open;
+        migrate_fd_error(s);
+        return ret;
     }
-
-    if (ret >= 0)
-        migrate_fd_connect(s);
-
+    migrate_fd_connect(s);
     return 0;
-
-err_after_open:
-    close(s->fd);
-
-err_after_socket:
-    return -1;
 }
 
 static void unix_accept_incoming_migration(void *opaque)
Signature · 1 line
1.7.4
#37 lore raw

PATCH v2 26/28 migration: qemu_savevm_iterate has three return values

In reply to Juan Quintela

We were retrying when there was one error, entering a loop.

---
 migration.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
migration.c +5 -1
index a02100b..6082c32 100644
@@ -269,6 +269,7 @@ static ssize_t migrate_fd_put_buffer(void *opaque, const void *data,
 static void migrate_fd_put_ready(void *opaque)
 {
     MigrationState *s = opaque;
+    int ret;
 
     if (s->state != MIG_STATE_ACTIVE) {
         DPRINTF("put_ready returning because of non-active state\n");
@@ -276,7 +277,10 @@ static void migrate_fd_put_ready(void *opaque)
     }
 
     DPRINTF("iterate\n");
-    if (qemu_savevm_state_iterate(s->mon, s->file) == 1) {
+    ret = qemu_savevm_state_iterate(s->mon, s->file);
+    if (ret == -1) {
+        migrate_fd_error(s);
+    } else if (ret == 1) {
         int old_vm_running = vm_running;
 
         DPRINTF("done iterating\n");
Signature · 1 line
1.7.4
#38 lore raw

PATCH v2 27/28 migration: If there is one error, it makes no sense to continue

In reply to Juan Quintela
---
 buffered_file.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
buffered_file.c +1 -1
index 8435a31..3c917ff 100644
@@ -195,7 +195,7 @@ static int buffered_rate_limit(void *opaque)
     QEMUFileBuffered *s = opaque;
 
     if (s->has_error)
-        return 0;
+        return -1;
 
     if (s->freeze_output)
         return 1;
Signature · 1 line
1.7.4
#39 lore raw

PATCH v2 28/28 migration: make migration-{tcp, unix} consistent

In reply to Juan Quintela

Files are almost identical in functionality, just remove the differences that make no sense.

---
 migration-tcp.c  |   15 ++++++++++-----
 migration-unix.c |   46 +++++++++++++++++++++++++---------------------
 2 files changed, 35 insertions(+), 26 deletions(-)
migration-tcp.c +10 -5
index ffe8ed6..85ef300 100644
@@ -49,7 +49,6 @@ static int tcp_close(MigrationState *s)
     return 0;
 }
 
-
 static void tcp_wait_for_connect(void *opaque)
 {
     MigrationState *s = opaque;
@@ -85,12 +84,14 @@ int tcp_start_outgoing_migration(MigrationState *s, const char *host_port)
     if (ret < 0) {
         return ret;
     }
+
     s->get_error = socket_errno;
     s->write = socket_write;
     s->close = tcp_close;
 
     s->fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (s->fd == -1) {
+        DPRINTF("Unable to open socket");
         return -socket_error();
     }
 
@@ -156,23 +157,27 @@ int tcp_start_incoming_migration(const char *host_port)
     int val;
     int s;
 
+    DPRINTF("Attempting to start an incoming migration\n");
+
     if (parse_host_port(&addr, host_port) < 0) {
         fprintf(stderr, "invalid host/port combination: %s\n", host_port);
         return -EINVAL;
     }
 
     s = qemu_socket(PF_INET, SOCK_STREAM, 0);
-    if (s == -1)
+    if (s == -1) {
         return -socket_error();
+    }
 
     val = 1;
     setsockopt(s, SOL_SOCKET, SO_REUSEADDR, (const char *)&val, sizeof(val));
 
-    if (bind(s, (struct sockaddr *)&addr, sizeof(addr)) == -1)
+    if (bind(s, (struct sockaddr *)&addr, sizeof(addr)) == -1) {
         goto err;
-
-    if (listen(s, 1) == -1)
+    }
+    if (listen(s, 1) == -1) {
         goto err;
+    }
 
     qemu_set_fd_handler2(s, NULL, tcp_accept_incoming_migration, NULL,
                          (void *)(unsigned long)s);
migration-unix.c +25 -21

84 lines not on this page · raw message

Signature · 1 line
1.7.4
Acked-by on the v2 series
#40 lore raw
In reply to Juan Quintela

2011/2/24 Juan Quintela <[email protected]>:

Quoted 78 lines

v2:
- make Jan^Wcheckpatch.pl happy
- Yoshiaki Tamura suggestions:
 - include its two patches to clean things
 - MAX_THROTTLE define
 - migration_state enum
- I removed spurious differences between migration-{tcp,unix}
- better error propagation, after this patch:
  migrate -d "tcp:name_don_exist:port"
  migrate -d "tcp:name:port_dont_exist"
  migrate -d "exec: prog_dont_exist"
  migrate -d "exec: gzip > /path/dont/exist"
 fail as expected.  Last two used to enter an infinite loop.

Later, Juan.

v1:
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 (26):
 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: 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: Don't use callback on file defining it
 migration: propagate error correctly
 migration: qemu_savevm_iterate has three return values
 migration: If there is one error, it makes no sense to continue
 migration: make migration-{tcp,unix} consistent

Yoshiaki Tamura (2):
 savevm: avoid qemu_savevm_state_iteate() to return 1 when qemu file has error.
 migration: add error handling to migrate_fd_put_notify().

 buffered_file.c  |    2 +-
 migration-exec.c |   39 +----
 migration-fd.c   |   42 ++-----
 migration-tcp.c  |   76 ++++------
 migration-unix.c |  112 ++++++---------
 migration.c      |  407 ++++++++++++++++++++++++++---------------------------- migration.h      |   85 ++----------
 savevm.c         |    7 +-
 ui/spice-core.c  |    4 +-

Quoted 7 lines
 9 files changed, 307 insertions(+), 467 deletions(-)

--
1.7.4