[Qemu-devel] [PATCH v3 0/4] Convert 'status' of MigrationInfo from open-coded 'str' to enum type

[Qemu-devel] [PATCH v3 0/4] Convert 'status' of MigrationInfo from open-coded 'str' to enum type

[zhanghailiang]

Hi,

This series converts open-coded 'str' type to enum type for 'status'.

This conversion will be more convenient for future extensibility.
Actually, I will add a MIG_STATE_COLO state for COLO, and i also
saw Dave added MIG_STATE_POSTCOPY_ACTIVE for postcopy.

Patch 1 and 2 are preparation for the conversion, patch 3 completes the conversion.
Besides, i add a additional patch (patch 4) to expose 'cancelling' to user,
it will influence libvirt side. And i have CC libvirt development. 

One more thing, i have to replace MIG_STATE_ERROR with MIG_STATE_FAILED,
and it begin from 0, not its original -1. I think it has no side effect.

Please review.

v3:
- Use longer name for Migration status macro. (Eric Blake)
- Rename internal-only typename 'MigrationStatus'. (Eric, Dave, Markus)
- Expose 'cancelling' state. (Eric Blake)
Thanks for their comments. ;)

v2:
- Remove '(since xyz)' strings. (Eric Blake)

zhanghailiang (4):
  migration: Rename abbreviated macro MIG_STATE_* to 
    MIGRATION_STATUS_*
  hmp: Rename 'MigrationStatus' to 'HMPMigrationStatus'
  migration: Convert 'status' of MigrationInfo to use an enum type
  migration: Expose 'cancelling' status to user

 hmp.c                 | 15 ++++----
 migration/migration.c | 95 ++++++++++++++++++++++++---------------------------
 qapi-schema.json      | 34 +++++++++++++++---
 3 files changed, 81 insertions(+), 63 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH v3 1/4] migration: Rename abbreviated macro MIG_STATE_* to MIGRATION_STATUS_*

[zhanghailiang]

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 migration/migration.c | 93 +++++++++++++++++++++++++++------------------------
 1 file changed, 50 insertions(+), 43 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index b3adbc6..0aafbdf 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -27,13 +27,13 @@
 #include "trace.h"
 
 enum {
-    MIG_STATE_ERROR = -1,
-    MIG_STATE_NONE,
-    MIG_STATE_SETUP,
-    MIG_STATE_CANCELLING,
-    MIG_STATE_CANCELLED,
-    MIG_STATE_ACTIVE,
-    MIG_STATE_COMPLETED,
+    MIGRATION_STATUS_ERROR = -1,
+    MIGRATION_STATUS_NONE,
+    MIGRATION_STATUS_SETUP,
+    MIGRATION_STATUS_CANCELLING,
+    MIGRATION_STATUS_CANCELLED,
+    MIGRATION_STATUS_ACTIVE,
+    MIGRATION_STATUS_COMPLETED,
 };
 
 #define MAX_THROTTLE  (32 << 20)      /* Migration speed throttling */
@@ -56,7 +56,7 @@ static NotifierList migration_state_notifiers =
 MigrationState *migrate_get_current(void)
 {
     static MigrationState current_migration = {
-        .state = MIG_STATE_NONE,
+        .state = MIGRATION_STATUS_NONE,
         .bandwidth_limit = MAX_THROTTLE,
         .xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE,
         .mbps = -1,
@@ -184,16 +184,16 @@ MigrationInfo *qmp_query_migrate(Error **errp)
     MigrationState *s = migrate_get_current();
 
     switch (s->state) {
-    case MIG_STATE_NONE:
+    case MIGRATION_STATUS_NONE:
         /* no migration has happened ever */
         break;
-    case MIG_STATE_SETUP:
+    case MIGRATION_STATUS_SETUP:
         info->has_status = true;
         info->status = g_strdup("setup");
         info->has_total_time = false;
         break;
-    case MIG_STATE_ACTIVE:
-    case MIG_STATE_CANCELLING:
+    case MIGRATION_STATUS_ACTIVE:
+    case MIGRATION_STATUS_CANCELLING:
         info->has_status = true;
         info->status = g_strdup("active");
         info->has_total_time = true;
@@ -227,7 +227,7 @@ MigrationInfo *qmp_query_migrate(Error **errp)
 
         get_xbzrle_cache_stats(info);
         break;
-    case MIG_STATE_COMPLETED:
+    case MIGRATION_STATUS_COMPLETED:
         get_xbzrle_cache_stats(info);
 
         info->has_status = true;
@@ -251,11 +251,11 @@ MigrationInfo *qmp_query_migrate(Error **errp)
         info->ram->mbps = s->mbps;
         info->ram->dirty_sync_count = s->dirty_sync_count;
         break;
-    case MIG_STATE_ERROR:
+    case MIGRATION_STATUS_ERROR:
         info->has_status = true;
         info->status = g_strdup("failed");
         break;
-    case MIG_STATE_CANCELLED:
+    case MIGRATION_STATUS_CANCELLED:
         info->has_status = true;
         info->status = g_strdup("cancelled");
         break;
@@ -270,7 +270,8 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
     MigrationState *s = migrate_get_current();
     MigrationCapabilityStatusList *cap;
 
-    if (s->state == MIG_STATE_ACTIVE || s->state == MIG_STATE_SETUP) {
+    if (s->state == MIGRATION_STATUS_ACTIVE ||
+        s->state == MIGRATION_STATUS_SETUP) {
         error_set(errp, QERR_MIGRATION_ACTIVE);
         return;
     }
@@ -306,12 +307,13 @@ static void migrate_fd_cleanup(void *opaque)
         s->file = NULL;
     }
 
-    assert(s->state != MIG_STATE_ACTIVE);
+    assert(s->state != MIGRATION_STATUS_ACTIVE);
 
-    if (s->state != MIG_STATE_COMPLETED) {
+    if (s->state != MIGRATION_STATUS_COMPLETED) {
         qemu_savevm_state_cancel();
-        if (s->state == MIG_STATE_CANCELLING) {
-            migrate_set_state(s, MIG_STATE_CANCELLING, MIG_STATE_CANCELLED);
+        if (s->state == MIGRATION_STATUS_CANCELLING) {
+            migrate_set_state(s, MIGRATION_STATUS_CANCELLING,
+                              MIGRATION_STATUS_CANCELLED);
         }
     }
 
@@ -322,8 +324,8 @@ void migrate_fd_error(MigrationState *s)
 {
     trace_migrate_fd_error();
     assert(s->file == NULL);
-    s->state = MIG_STATE_ERROR;
-    trace_migrate_set_state(MIG_STATE_ERROR);
+    s->state = MIGRATION_STATUS_ERROR;
+    trace_migrate_set_state(MIGRATION_STATUS_ERROR);
     notifier_list_notify(&migration_state_notifiers, s);
 }
 
@@ -335,11 +337,12 @@ static void migrate_fd_cancel(MigrationState *s)
 
     do {
         old_state = s->state;
-        if (old_state != MIG_STATE_SETUP && old_state != MIG_STATE_ACTIVE) {
+        if (old_state != MIGRATION_STATUS_SETUP &&
+            old_state != MIGRATION_STATUS_ACTIVE) {
             break;
         }
-        migrate_set_state(s, old_state, MIG_STATE_CANCELLING);
-    } while (s->state != MIG_STATE_CANCELLING);
+        migrate_set_state(s, old_state, MIGRATION_STATUS_CANCELLING);
+    } while (s->state != MIGRATION_STATUS_CANCELLING);
 
     /*
      * If we're unlucky the migration code might be stuck somewhere in a
@@ -348,7 +351,7 @@ static void migrate_fd_cancel(MigrationState *s)
      * The outgoing qemu file gets closed in migrate_fd_cleanup that is
      * called in a bh, so there is no race against this cancel.
      */
-    if (s->state == MIG_STATE_CANCELLING && f) {
+    if (s->state == MIGRATION_STATUS_CANCELLING && f) {
         qemu_file_shutdown(f);
     }
 }
@@ -365,18 +368,18 @@ void remove_migration_state_change_notifier(Notifier *notify)
 
 bool migration_in_setup(MigrationState *s)
 {
-    return s->state == MIG_STATE_SETUP;
+    return s->state == MIGRATION_STATUS_SETUP;
 }
 
 bool migration_has_finished(MigrationState *s)
 {
-    return s->state == MIG_STATE_COMPLETED;
+    return s->state == MIGRATION_STATUS_COMPLETED;
 }
 
 bool migration_has_failed(MigrationState *s)
 {
-    return (s->state == MIG_STATE_CANCELLED ||
-            s->state == MIG_STATE_ERROR);
+    return (s->state == MIGRATION_STATUS_CANCELLED ||
+            s->state == MIGRATION_STATUS_ERROR);
 }
 
 static MigrationState *migrate_init(const MigrationParams *params)
@@ -396,8 +399,8 @@ static MigrationState *migrate_init(const MigrationParams *params)
     s->xbzrle_cache_size = xbzrle_cache_size;
 
     s->bandwidth_limit = bandwidth_limit;
-    s->state = MIG_STATE_SETUP;
-    trace_migrate_set_state(MIG_STATE_SETUP);
+    s->state = MIGRATION_STATUS_SETUP;
+    trace_migrate_set_state(MIGRATION_STATUS_SETUP);
 
     s->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
     return s;
@@ -427,8 +430,9 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     params.blk = has_blk && blk;
     params.shared = has_inc && inc;
 
-    if (s->state == MIG_STATE_ACTIVE || s->state == MIG_STATE_SETUP ||
-        s->state == MIG_STATE_CANCELLING) {
+    if (s->state == MIGRATION_STATUS_ACTIVE ||
+        s->state == MIGRATION_STATUS_SETUP ||
+        s->state == MIGRATION_STATUS_CANCELLING) {
         error_set(errp, QERR_MIGRATION_ACTIVE);
         return;
     }
@@ -465,7 +469,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 #endif
     } else {
         error_set(errp, QERR_INVALID_PARAMETER_VALUE, "uri", "a valid migration protocol");
-        s->state = MIG_STATE_ERROR;
+        s->state = MIGRATION_STATUS_ERROR;
         return;
     }
 
@@ -600,9 +604,9 @@ static void *migration_thread(void *opaque)
     qemu_savevm_state_begin(s->file, &s->params);
 
     s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
-    migrate_set_state(s, MIG_STATE_SETUP, MIG_STATE_ACTIVE);
+    migrate_set_state(s, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_ACTIVE);
 
-    while (s->state == MIG_STATE_ACTIVE) {
+    while (s->state == MIGRATION_STATUS_ACTIVE) {
         int64_t current_time;
         uint64_t pending_size;
 
@@ -627,19 +631,22 @@ static void *migration_thread(void *opaque)
                 qemu_mutex_unlock_iothread();
 
                 if (ret < 0) {
-                    migrate_set_state(s, MIG_STATE_ACTIVE, MIG_STATE_ERROR);
+                    migrate_set_state(s, MIGRATION_STATUS_ACTIVE,
+                                      MIGRATION_STATUS_ERROR);
                     break;
                 }
 
                 if (!qemu_file_get_error(s->file)) {
-                    migrate_set_state(s, MIG_STATE_ACTIVE, MIG_STATE_COMPLETED);
+                    migrate_set_state(s, MIGRATION_STATUS_ACTIVE,
+                                      MIGRATION_STATUS_COMPLETED);
                     break;
                 }
             }
         }
 
         if (qemu_file_get_error(s->file)) {
-            migrate_set_state(s, MIG_STATE_ACTIVE, MIG_STATE_ERROR);
+            migrate_set_state(s, MIGRATION_STATUS_ACTIVE,
+                              MIGRATION_STATUS_ERROR);
             break;
         }
         current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
@@ -671,7 +678,7 @@ static void *migration_thread(void *opaque)
     }
 
     qemu_mutex_lock_iothread();
-    if (s->state == MIG_STATE_COMPLETED) {
+    if (s->state == MIGRATION_STATUS_COMPLETED) {
         int64_t end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
         uint64_t transferred_bytes = qemu_ftell(s->file);
         s->total_time = end_time - s->total_time;
@@ -694,8 +701,8 @@ static void *migration_thread(void *opaque)
 
 void migrate_fd_connect(MigrationState *s)
 {
-    s->state = MIG_STATE_SETUP;
-    trace_migrate_set_state(MIG_STATE_SETUP);
+    s->state = MIGRATION_STATUS_SETUP;
+    trace_migrate_set_state(MIGRATION_STATUS_SETUP);
 
     /* This is a best 1st approximation. ns to ms */
     s->expected_downtime = max_downtime/1000000;
-- 
1.7.12.4

[Qemu-devel] [PATCH v3 2/4] hmp: Rename 'MigrationStatus' to 'HMPMigrationStatus'

[zhanghailiang]

We will use the typename 'MigrationStatus' for publicly exported typename,
So here we rename the internal-only 'MigrationStatus' to
'HMPMigrationStatus'.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 hmp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hmp.c b/hmp.c
index 735097c..8a7561f 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1299,16 +1299,16 @@ void hmp_block_job_complete(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &error);
 }
 
-typedef struct MigrationStatus
+typedef struct HMPMigrationStatus
 {
     QEMUTimer *timer;
     Monitor *mon;
     bool is_block_migration;
-} MigrationStatus;
+} HMPMigrationStatus;
 
 static void hmp_migrate_status_cb(void *opaque)
 {
-    MigrationStatus *status = opaque;
+    HMPMigrationStatus *status = opaque;
     MigrationInfo *info;
 
     info = qmp_query_migrate(NULL);
@@ -1356,7 +1356,7 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
     }
 
     if (!detach) {
-        MigrationStatus *status;
+        HMPMigrationStatus *status;
 
         if (monitor_suspend(mon) < 0) {
             monitor_printf(mon, "terminal does not allow synchronous "
-- 
1.7.12.4

[Qemu-devel] [PATCH v3 3/4] migration: Convert 'status' of MigrationInfo to use an enum type

[zhanghailiang]

The original 'status' is an open-coded 'str' type, convert it to use an
enum type.
This conversion is backwards compatible, better documented and
more convenient for future extensibility.

We also rename 'MIGRATION_STATUS_ERROR' to 'MIGRATION_STATUS_FAILED'.
In addition, Fix a typo for qapi-schema.json: comppleted -> completed

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 hmp.c                 |  7 ++++---
 migration/migration.c | 34 ++++++++++++----------------------
 qapi-schema.json      | 34 +++++++++++++++++++++++++++++-----
 3 files changed, 45 insertions(+), 30 deletions(-)

diff --git a/hmp.c b/hmp.c
index 8a7561f..635f73b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -158,7 +158,8 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
     }
 
     if (info->has_status) {
-        monitor_printf(mon, "Migration status: %s\n", info->status);
+        monitor_printf(mon, "Migration status: %s\n",
+                       MigrationStatus_lookup[info->status]);
         monitor_printf(mon, "total time: %" PRIu64 " milliseconds\n",
                        info->total_time);
         if (info->has_expected_downtime) {
@@ -1312,8 +1313,8 @@ static void hmp_migrate_status_cb(void *opaque)
     MigrationInfo *info;
 
     info = qmp_query_migrate(NULL);
-    if (!info->has_status || strcmp(info->status, "active") == 0 ||
-        strcmp(info->status, "setup") == 0) {
+    if (!info->has_status || info->status == MIGRATION_STATUS_ACTIVE ||
+        info->status == MIGRATION_STATUS_SETUP) {
         if (info->has_disk) {
             int progress;
 
diff --git a/migration/migration.c b/migration/migration.c
index 0aafbdf..035e005 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -26,16 +26,6 @@
 #include "qmp-commands.h"
 #include "trace.h"
 
-enum {
-    MIGRATION_STATUS_ERROR = -1,
-    MIGRATION_STATUS_NONE,
-    MIGRATION_STATUS_SETUP,
-    MIGRATION_STATUS_CANCELLING,
-    MIGRATION_STATUS_CANCELLED,
-    MIGRATION_STATUS_ACTIVE,
-    MIGRATION_STATUS_COMPLETED,
-};
-
 #define MAX_THROTTLE  (32 << 20)      /* Migration speed throttling */
 
 /* Amount of time to allocate to each "chunk" of bandwidth-throttled
@@ -189,13 +179,13 @@ MigrationInfo *qmp_query_migrate(Error **errp)
         break;
     case MIGRATION_STATUS_SETUP:
         info->has_status = true;
-        info->status = g_strdup("setup");
+        info->status = MIGRATION_STATUS_SETUP;
         info->has_total_time = false;
         break;
     case MIGRATION_STATUS_ACTIVE:
     case MIGRATION_STATUS_CANCELLING:
         info->has_status = true;
-        info->status = g_strdup("active");
+        info->status = MIGRATION_STATUS_ACTIVE;
         info->has_total_time = true;
         info->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME)
             - s->total_time;
@@ -231,7 +221,7 @@ MigrationInfo *qmp_query_migrate(Error **errp)
         get_xbzrle_cache_stats(info);
 
         info->has_status = true;
-        info->status = g_strdup("completed");
+        info->status = MIGRATION_STATUS_COMPLETED;
         info->has_total_time = true;
         info->total_time = s->total_time;
         info->has_downtime = true;
@@ -251,13 +241,13 @@ MigrationInfo *qmp_query_migrate(Error **errp)
         info->ram->mbps = s->mbps;
         info->ram->dirty_sync_count = s->dirty_sync_count;
         break;
-    case MIGRATION_STATUS_ERROR:
+    case MIGRATION_STATUS_FAILED:
         info->has_status = true;
-        info->status = g_strdup("failed");
+        info->status = MIGRATION_STATUS_FAILED;
         break;
     case MIGRATION_STATUS_CANCELLED:
         info->has_status = true;
-        info->status = g_strdup("cancelled");
+        info->status = MIGRATION_STATUS_CANCELLED;
         break;
     }
 
@@ -324,8 +314,8 @@ void migrate_fd_error(MigrationState *s)
 {
     trace_migrate_fd_error();
     assert(s->file == NULL);
-    s->state = MIGRATION_STATUS_ERROR;
-    trace_migrate_set_state(MIGRATION_STATUS_ERROR);
+    s->state = MIGRATION_STATUS_FAILED;
+    trace_migrate_set_state(MIGRATION_STATUS_FAILED);
     notifier_list_notify(&migration_state_notifiers, s);
 }
 
@@ -379,7 +369,7 @@ bool migration_has_finished(MigrationState *s)
 bool migration_has_failed(MigrationState *s)
 {
     return (s->state == MIGRATION_STATUS_CANCELLED ||
-            s->state == MIGRATION_STATUS_ERROR);
+            s->state == MIGRATION_STATUS_FAILED);
 }
 
 static MigrationState *migrate_init(const MigrationParams *params)
@@ -469,7 +459,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 #endif
     } else {
         error_set(errp, QERR_INVALID_PARAMETER_VALUE, "uri", "a valid migration protocol");
-        s->state = MIGRATION_STATUS_ERROR;
+        s->state = MIGRATION_STATUS_FAILED;
         return;
     }
 
@@ -632,7 +622,7 @@ static void *migration_thread(void *opaque)
 
                 if (ret < 0) {
                     migrate_set_state(s, MIGRATION_STATUS_ACTIVE,
-                                      MIGRATION_STATUS_ERROR);
+                                      MIGRATION_STATUS_FAILED);
                     break;
                 }
 
@@ -646,7 +636,7 @@ static void *migration_thread(void *opaque)
 
         if (qemu_file_get_error(s->file)) {
             migrate_set_state(s, MIGRATION_STATUS_ACTIVE,
-                              MIGRATION_STATUS_ERROR);
+                              MIGRATION_STATUS_FAILED);
             break;
         }
         current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
diff --git a/qapi-schema.json b/qapi-schema.json
index e16f8eb..3b5904b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -410,19 +410,43 @@
            'cache-miss': 'int', 'cache-miss-rate': 'number',
            'overflow': 'int' } }
 
+# @MigrationStatus:
+#
+# An enumeration of migration status.
+#
+# @failed: some error occurred during migration process.
+#
+# @none: no migration has ever happened.
+#
+# @setup: migration process has been initiated.
+#
+# @cancelling: in the process of cancelling migration.
+#
+# @cancelled: cancelling migration is finished.
+#
+# @active: in the process of doing migration.
+#
+# @completed: migration is finished.
+#
+# Since: 2.3
+#
+##
+{ 'enum': 'MigrationStatus',
+  'data': [ 'failed', 'none', 'setup', 'cancelling', 'cancelled',
+            'active', 'completed' ] }
+
 ##
 # @MigrationInfo
 #
 # Information about current migration process.
 #
-# @status: #optional string describing the current migration status.
-#          As of 0.14.0 this can be 'setup', 'active', 'completed', 'failed' or
-#          'cancelled'. If this field is not returned, no migration process
+# @status: #optional @MigState describing the current migration status.
+#          If this field is not returned, no migration process
 #          has been initiated
 #
 # @ram: #optional @MigrationStats containing detailed migration
 #       status, only returned if status is 'active' or
-#       'completed'. 'comppleted' (since 1.2)
+#       'completed'. 'completed' (since 1.2)
 #
 # @disk: #optional @MigrationStats containing detailed disk migration
 #        status, only returned if status is 'active' and it is a block
@@ -453,7 +477,7 @@
 # Since: 0.14.0
 ##
 { 'type': 'MigrationInfo',
-  'data': {'*status': 'str', '*ram': 'MigrationStats',
+  'data': {'*status': 'MigrationStatus', '*ram': 'MigrationStats',
            '*disk': 'MigrationStats',
            '*xbzrle-cache': 'XBZRLECacheStats',
            '*total-time': 'int',
-- 
1.7.12.4

[Qemu-devel] [PATCH v3 4/4] migration: Expose 'cancelling' status to user

[zhanghailiang]

'cancelling' status is introduced by commit 51cf4c1a, which is mainly avoid
possible starting a new migration process while the previous one still exist.
But we don't expose this status to user, instead by return a 'active' state.

Here, we expose it to the user (such as libvirt), 'cancelling' status only
occurs for a short window before the migration aborts, so for users,
if they cancell a migration process, it will observe 'cancelling' status
occasionally.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Cc: libvir-list@redhat.com
---
 migration/migration.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 035e005..a57928d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -179,13 +179,11 @@ MigrationInfo *qmp_query_migrate(Error **errp)
         break;
     case MIGRATION_STATUS_SETUP:
         info->has_status = true;
-        info->status = MIGRATION_STATUS_SETUP;
         info->has_total_time = false;
         break;
     case MIGRATION_STATUS_ACTIVE:
     case MIGRATION_STATUS_CANCELLING:
         info->has_status = true;
-        info->status = MIGRATION_STATUS_ACTIVE;
         info->has_total_time = true;
         info->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME)
             - s->total_time;
@@ -221,7 +219,6 @@ MigrationInfo *qmp_query_migrate(Error **errp)
         get_xbzrle_cache_stats(info);
 
         info->has_status = true;
-        info->status = MIGRATION_STATUS_COMPLETED;
         info->has_total_time = true;
         info->total_time = s->total_time;
         info->has_downtime = true;
@@ -243,13 +240,12 @@ MigrationInfo *qmp_query_migrate(Error **errp)
         break;
     case MIGRATION_STATUS_FAILED:
         info->has_status = true;
-        info->status = MIGRATION_STATUS_FAILED;
         break;
     case MIGRATION_STATUS_CANCELLED:
         info->has_status = true;
-        info->status = MIGRATION_STATUS_CANCELLED;
         break;
     }
+    info->status = s->state;
 
     return info;
 }
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v3 3/4] migration: Convert 'status' of MigrationInfo to use an enum type

[Markus Armbruster]

zhanghailiang <zhang.zhanghailiang@huawei.com> writes:

> The original 'status' is an open-coded 'str' type, convert it to use an
> enum type.
> This conversion is backwards compatible, better documented and
> more convenient for future extensibility.
>
> We also rename 'MIGRATION_STATUS_ERROR' to 'MIGRATION_STATUS_FAILED'.
> In addition, Fix a typo for qapi-schema.json: comppleted -> completed
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
[...]
> diff --git a/qapi-schema.json b/qapi-schema.json
> index e16f8eb..3b5904b 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
>  ##
>  # @MigrationInfo
>  #
>  # Information about current migration process.
>  #
> -# @status: #optional string describing the current migration status.
> -#          As of 0.14.0 this can be 'setup', 'active', 'completed', 'failed' or
> -#          'cancelled'. If this field is not returned, no migration process
> +# @status: #optional @MigState describing the current migration status.
> +#          If this field is not returned, no migration process
>  #          has been initiated
>  #
>  # @ram: #optional @MigrationStats containing detailed migration
>  #       status, only returned if status is 'active' or
> -#       'completed'. 'comppleted' (since 1.2)
> +#       'completed'. 'completed' (since 1.2)

Shouldn't this just be

+#       'completed' (since 1.2)

?

>  #
>  # @disk: #optional @MigrationStats containing detailed disk migration
>  #        status, only returned if status is 'active' and it is a block
> @@ -453,7 +477,7 @@
>  # Since: 0.14.0
>  ##
>  { 'type': 'MigrationInfo',
> -  'data': {'*status': 'str', '*ram': 'MigrationStats',
> +  'data': {'*status': 'MigrationStatus', '*ram': 'MigrationStats',
>             '*disk': 'MigrationStats',
>             '*xbzrle-cache': 'XBZRLECacheStats',
>             '*total-time': 'int',

Re: [Qemu-devel] [PATCH v3 3/4] migration: Convert 'status' of MigrationInfo to use an enum type

[zhanghailiang]

On 2015/3/6 2:34, Markus Armbruster wrote:
> zhanghailiang <zhang.zhanghailiang@huawei.com> writes:
>
>> The original 'status' is an open-coded 'str' type, convert it to use an
>> enum type.
>> This conversion is backwards compatible, better documented and
>> more convenient for future extensibility.
>>
>> We also rename 'MIGRATION_STATUS_ERROR' to 'MIGRATION_STATUS_FAILED'.
>> In addition, Fix a typo for qapi-schema.json: comppleted -> completed
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> [...]
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index e16f8eb..3b5904b 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>>   ##
>>   # @MigrationInfo
>>   #
>>   # Information about current migration process.
>>   #
>> -# @status: #optional string describing the current migration status.
>> -#          As of 0.14.0 this can be 'setup', 'active', 'completed', 'failed' or
>> -#          'cancelled'. If this field is not returned, no migration process
>> +# @status: #optional @MigState describing the current migration status.
>> +#          If this field is not returned, no migration process
>>   #          has been initiated
>>   #
>>   # @ram: #optional @MigrationStats containing detailed migration
>>   #       status, only returned if status is 'active' or
>> -#       'completed'. 'comppleted' (since 1.2)
>> +#       'completed'. 'completed' (since 1.2)
>
> Shouldn't this just be
>
> +#       'completed' (since 1.2)
>

Er, Yes, in this way, it is more clean, thanks, will fix in v4 ~

>
>>   #
>>   # @disk: #optional @MigrationStats containing detailed disk migration
>>   #        status, only returned if status is 'active' and it is a block
>> @@ -453,7 +477,7 @@
>>   # Since: 0.14.0
>>   ##
>>   { 'type': 'MigrationInfo',
>> -  'data': {'*status': 'str', '*ram': 'MigrationStats',
>> +  'data': {'*status': 'MigrationStatus', '*ram': 'MigrationStats',
>>              '*disk': 'MigrationStats',
>>              '*xbzrle-cache': 'XBZRLECacheStats',
>>              '*total-time': 'int',
>
> .
>

Re: [Qemu-devel] [PATCH v3 1/4] migration: Rename abbreviated macro MIG_STATE_* to MIGRATION_STATUS_*

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1459 bytes --]

On 03/04/2015 07:09 AM, zhanghailiang wrote:
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  migration/migration.c | 93 +++++++++++++++++++++++++++------------------------
>  1 file changed, 50 insertions(+), 43 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index b3adbc6..0aafbdf 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -27,13 +27,13 @@
>  #include "trace.h"
>  
>  enum {
> -    MIG_STATE_ERROR = -1,
> -    MIG_STATE_NONE,
> -    MIG_STATE_SETUP,
> -    MIG_STATE_CANCELLING,
> -    MIG_STATE_CANCELLED,
> -    MIG_STATE_ACTIVE,
> -    MIG_STATE_COMPLETED,
> +    MIGRATION_STATUS_ERROR = -1,

Please also rename _ERROR to _FAILED in this patch, so that patch 3/4 is
not doing any further renames.  And document that the rename is
intentional in the body of the commit message.


> @@ -251,11 +251,11 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>          info->ram->mbps = s->mbps;
>          info->ram->dirty_sync_count = s->dirty_sync_count;
>          break;
> -    case MIG_STATE_ERROR:
> +    case MIGRATION_STATUS_ERROR:
>          info->has_status = true;
>          info->status = g_strdup("failed");
>          break;

That is, _this_ patch is the one to make the enum name match the public
string.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 2/4] hmp: Rename 'MigrationStatus' to 'HMPMigrationStatus'

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 518 bytes --]

On 03/04/2015 07:09 AM, zhanghailiang wrote:
> We will use the typename 'MigrationStatus' for publicly exported typename,
> So here we rename the internal-only 'MigrationStatus' to
> 'HMPMigrationStatus'.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  hmp.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 3/4] migration: Convert 'status' of MigrationInfo to use an enum type

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 957 bytes --]

On 03/04/2015 07:09 AM, zhanghailiang wrote:
> The original 'status' is an open-coded 'str' type, convert it to use an
> enum type.
> This conversion is backwards compatible, better documented and
> more convenient for future extensibility.
> 
> We also rename 'MIGRATION_STATUS_ERROR' to 'MIGRATION_STATUS_FAILED'.

I argue that this change should be done in 1/4.

> In addition, Fix a typo for qapi-schema.json: comppleted -> completed

but this one can stay here.

> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  hmp.c                 |  7 ++++---
>  migration/migration.c | 34 ++++++++++++----------------------
>  qapi-schema.json      | 34 +++++++++++++++++++++++++++++-----
>  3 files changed, 45 insertions(+), 30 deletions(-)
> 

Otherwise, modulo Markus' review, it's looking better.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 4/4] migration: Expose 'cancelling' status to user

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1419 bytes --]

On 03/04/2015 07:09 AM, zhanghailiang wrote:
> 'cancelling' status is introduced by commit 51cf4c1a, which is mainly avoid

s/is introduced/was introduced/
s/which is mainly avoid/mainly to avoid a/

> possible starting a new migration process while the previous one still exist.

s/starting a new/start of a new/
s/exist/exists/

> But we don't expose this status to user, instead by return a 'active' state.

s/don't/didn't/
s/by return a/we returned the/

> 
> Here, we expose it to the user (such as libvirt), 'cancelling' status only
> occurs for a short window before the migration aborts, so for users,
> if they cancell a migration process, it will observe 'cancelling' status

s/cancell/cancel/

> occasionally.

Add:
Testing revealed that with older libvirt (anything 1.2.13 or less) will
print an odd error message if the state is seen, but that the migration
is still properly cancelled. Newer libvirt will be patched to recognize
the new state without the odd error message.

> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Cc: libvir-list@redhat.com
> ---
>  migration/migration.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)

With the grammar in the commit message fixed,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 1/4] migration: Rename abbreviated macro MIG_STATE_* to MIGRATION_STATUS_*

[zhanghailiang]

On 2015/3/7 0:14, Eric Blake wrote:
> On 03/04/2015 07:09 AM, zhanghailiang wrote:
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>   migration/migration.c | 93 +++++++++++++++++++++++++++------------------------
>>   1 file changed, 50 insertions(+), 43 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index b3adbc6..0aafbdf 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -27,13 +27,13 @@
>>   #include "trace.h"
>>
>>   enum {
>> -    MIG_STATE_ERROR = -1,
>> -    MIG_STATE_NONE,
>> -    MIG_STATE_SETUP,
>> -    MIG_STATE_CANCELLING,
>> -    MIG_STATE_CANCELLED,
>> -    MIG_STATE_ACTIVE,
>> -    MIG_STATE_COMPLETED,
>> +    MIGRATION_STATUS_ERROR = -1,
>
> Please also rename _ERROR to _FAILED in this patch, so that patch 3/4 is
> not doing any further renames.  And document that the rename is
> intentional in the body of the commit message.
>

OK, will fix in v4~

>
>> @@ -251,11 +251,11 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>>           info->ram->mbps = s->mbps;
>>           info->ram->dirty_sync_count = s->dirty_sync_count;
>>           break;
>> -    case MIG_STATE_ERROR:
>> +    case MIGRATION_STATUS_ERROR:
>>           info->has_status = true;
>>           info->status = g_strdup("failed");
>>           break;
>
> That is, _this_ patch is the one to make the enum name match the public
> string.
>

Re: [Qemu-devel] [PATCH v3 4/4] migration: Expose 'cancelling' status to user

[zhanghailiang]

On 2015/3/7 0:24, Eric Blake wrote:
> On 03/04/2015 07:09 AM, zhanghailiang wrote:
>> 'cancelling' status is introduced by commit 51cf4c1a, which is mainly avoid
>
> s/is introduced/was introduced/
> s/which is mainly avoid/mainly to avoid a/
>
>> possible starting a new migration process while the previous one still exist.
>
> s/starting a new/start of a new/
> s/exist/exists/
>
>> But we don't expose this status to user, instead by return a 'active' state.
>
> s/don't/didn't/
> s/by return a/we returned the/
>
>>
>> Here, we expose it to the user (such as libvirt), 'cancelling' status only
>> occurs for a short window before the migration aborts, so for users,
>> if they cancell a migration process, it will observe 'cancelling' status
>
> s/cancell/cancel/
>
>> occasionally.
>
> Add:
> Testing revealed that with older libvirt (anything 1.2.13 or less) will
> print an odd error message if the state is seen, but that the migration
> is still properly cancelled. Newer libvirt will be patched to recognize
> the new state without the odd error message.
>
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Cc: libvir-list@redhat.com
>> ---
>>   migration/migration.c | 6 +-----
>>   1 file changed, 1 insertion(+), 5 deletions(-)
>
> With the grammar in the commit message fixed,
> Reviewed-by: Eric Blake <eblake@redhat.com>
>

OK, will fix that in v4, thanks.