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

[Qemu-devel] [PATCH v4 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 MIGRATION_STATUS_FAILED,
and it begin from 0, not its original -1. I think it has no side effect.

Please review.

v4:
- Rename _ERROR to _FAILED in patch 1 instead of patch 4 (Eric)
- Add Reviewd-by for patch 2 and 4
- Fix some grammar in commit message of patch 4 (Eric)

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

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

Thanks for their comments. ;)

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 v4 1/4] migration: Rename abbreviated macro MIG_STATE_* to MIGRATION_STATUS_*

[zhanghailiang]

Rename all macros MIG_STATE_* to MIGRATION_STATUS_* except "MIG_STATE_ERROR",
we rename it to "MIGRATION_STATUS_FAILED" which will match the migration status
string 'failed'.

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..f6c998b 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_FAILED = -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_FAILED:
         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_FAILED;
+    trace_migrate_set_state(MIGRATION_STATUS_FAILED);
     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_FAILED);
 }
 
 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_FAILED;
         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_FAILED);
                     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_FAILED);
             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

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

[Eric Blake]

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

On 03/09/2015 12:45 AM, zhanghailiang wrote:
> Rename all macros MIG_STATE_* to MIGRATION_STATUS_* except "MIG_STATE_ERROR",
> we rename it to "MIGRATION_STATUS_FAILED" which will match the migration status
> string 'failed'.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  migration/migration.c | 93 +++++++++++++++++++++++++++------------------------
>  1 file changed, 50 insertions(+), 43 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 --]

[Qemu-devel] [PATCH v4 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 hmp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hmp.c b/hmp.c
index 71c28bc..7300ddc 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1331,16 +1331,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);
@@ -1388,7 +1388,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 v4 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.

In addition, Fix a typo for qapi-schema.json (just remove the typo) :
s/'completed'. 'comppleted' (since 1.2)/'completed' (since 1.2)

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

diff --git a/hmp.c b/hmp.c
index 7300ddc..cffc94f 100644
--- a/hmp.c
+++ b/hmp.c
@@ -162,7 +162,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) {
@@ -1344,8 +1345,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 f6c998b..035e005 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -26,16 +26,6 @@
 #include "qmp-commands.h"
 #include "trace.h"
 
-enum {
-    MIGRATION_STATUS_FAILED = -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;
@@ -253,11 +243,11 @@ MigrationInfo *qmp_query_migrate(Error **errp)
         break;
     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;
     }
 
diff --git a/qapi-schema.json b/qapi-schema.json
index e16f8eb..2b93b27 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'(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

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

[Eric Blake]

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

On 03/09/2015 12:45 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.
> 
> In addition, Fix a typo for qapi-schema.json (just remove the typo) :
> s/'completed'. 'comppleted' (since 1.2)/'completed' (since 1.2)
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  hmp.c                 |  7 ++++---
>  migration/migration.c | 20 +++++---------------
>  qapi-schema.json      | 34 +++++++++++++++++++++++++++++-----
>  3 files changed, 38 insertions(+), 23 deletions(-)

>  
> -enum {
> -    MIGRATION_STATUS_FAILED = -1,
> -    MIGRATION_STATUS_NONE,

Note that we are changing any 0-initialized struct from having
STATUS_NONE...


> +##
> +{ 'enum': 'MigrationStatus',
> +  'data': [ 'failed', 'none', 'setup', 'cancelling', 'cancelled',
> +            'active', 'completed' ] }

...to now having status _FAILED.  Fortunately, in migration/migration.c
(the only caller prior to this conversion), I didn't spot any
zero-initialization (migrate_get_current() returns a struct that is
explicitly initialized to _NONE).  So it looks correct.

> +
>  ##
>  # @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.

s/MigState/MigrationStatus/


> +  'data': {'*status': 'MigrationStatus', '*ram': 'MigrationStats',
>             '*disk': 'MigrationStats',

Hmm. "Status" and "Stats" look awfully close to one another.  But I can
live with the two names (it's not worth a respin just for the rename).
With the typo fix pointed out in the documentation,

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 v4 3/4] migration: Convert 'status' of MigrationInfo to use an enum type

[zhanghailiang]

On 2015/3/13 3:37, Eric Blake wrote:
> On 03/09/2015 12:45 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.
>>
>> In addition, Fix a typo for qapi-schema.json (just remove the typo) :
>> s/'completed'. 'comppleted' (since 1.2)/'completed' (since 1.2)
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>   hmp.c                 |  7 ++++---
>>   migration/migration.c | 20 +++++---------------
>>   qapi-schema.json      | 34 +++++++++++++++++++++++++++++-----
>>   3 files changed, 38 insertions(+), 23 deletions(-)
>
>>
>> -enum {
>> -    MIGRATION_STATUS_FAILED = -1,
>> -    MIGRATION_STATUS_NONE,
>
> Note that we are changing any 0-initialized struct from having
> STATUS_NONE...
>
>
>> +##
>> +{ 'enum': 'MigrationStatus',
>> +  'data': [ 'failed', 'none', 'setup', 'cancelling', 'cancelled',
>> +            'active', 'completed' ] }
>
> ...to now having status _FAILED.  Fortunately, in migration/migration.c
> (the only caller prior to this conversion), I didn't spot any
> zero-initialization (migrate_get_current() returns a struct that is
> explicitly initialized to _NONE).  So it looks correct.
>

Hmm, however, i think it deserves a small modification, i will adjust the place of
'failed', which i will move it to the behind of 'completed' in v5. In this way,
all of these enum type macros except 'failed' will be consistent with there original value.

>> +
>>   ##
>>   # @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.
>
> s/MigState/MigrationStatus/
>

fix in v5.

>
>> +  'data': {'*status': 'MigrationStatus', '*ram': 'MigrationStats',
>>              '*disk': 'MigrationStats',
>
> Hmm. "Status" and "Stats" look awfully close to one another.  But I can
> live with the two names (it's not worth a respin just for the rename).
> With the typo fix pointed out in the documentation,
>

I will keep it like that. Thanks.

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

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

[zhanghailiang]

'cancelling' status was introduced by commit 51cf4c1a, mainly to avoid a 
possible start of a new migration process while the previous one still exists.
But we didn't expose this status to user, instead we returned the '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 cancel a migration process, it will observe 'cancelling' status
occasionally.

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>
Reviewed-by: Eric Blake <eblake@redhat.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 v4 4/4] migration: Expose 'cancelling' status to user

[Paolo Bonzini]

On 09/03/2015 07:45, zhanghailiang wrote:
> 'cancelling' status was introduced by commit 51cf4c1a, mainly to avoid a 
> possible start of a new migration process while the previous one still exists.
> But we didn't expose this status to user, instead we returned the '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 cancel a migration process, it will observe 'cancelling' status
> occasionally.
> 
> 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>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Cc: libvir-list@redhat.com

Why is this necessary?

Paolo

> ---
>  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;
>  }
> 

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

[Eric Blake]

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

On 03/13/2015 04:49 AM, Paolo Bonzini wrote:
> 
> 
> On 09/03/2015 07:45, zhanghailiang wrote:
>> 'cancelling' status was introduced by commit 51cf4c1a, mainly to avoid a 
>> possible start of a new migration process while the previous one still exists.
>> But we didn't expose this status to user, instead we returned the '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 cancel a migration process, it will observe 'cancelling' status
>> occasionally.
>>
>> 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>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Cc: libvir-list@redhat.com
> 
> Why is this necessary?

It simplifies qemu's job of reporting migration status information (qemu
is no longer maintaining one set of states internally and a different
set of states externally), and I already have the libvirt counterpart
patch ready to go to gracefully accept the new state name.

-- 
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 v4 4/4] migration: Expose 'cancelling' status to user

[Dr. David Alan Gilbert]

* Eric Blake (eblake@redhat.com) wrote:
> On 03/13/2015 04:49 AM, Paolo Bonzini wrote:
> > 
> > 
> > On 09/03/2015 07:45, zhanghailiang wrote:
> >> 'cancelling' status was introduced by commit 51cf4c1a, mainly to avoid a 
> >> possible start of a new migration process while the previous one still exists.
> >> But we didn't expose this status to user, instead we returned the '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 cancel a migration process, it will observe 'cancelling' status
> >> occasionally.
> >>
> >> 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>
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> Cc: libvir-list@redhat.com
> > 
> > Why is this necessary?
> 
> It simplifies qemu's job of reporting migration status information (qemu
> is no longer maintaining one set of states internally and a different
> set of states externally), and I already have the libvirt counterpart
> patch ready to go to gracefully accept the new state name.

Yes, it does make life simpler in the long run.
(It does worry me a bit what happens to new qemu on old libvirt)

Dave

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


--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Eric Blake]

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

On 03/13/2015 06:28 AM, Dr. David Alan Gilbert wrote:

>>
>> It simplifies qemu's job of reporting migration status information (qemu
>> is no longer maintaining one set of states internally and a different
>> set of states externally), and I already have the libvirt counterpart
>> patch ready to go to gracefully accept the new state name.
> 
> Yes, it does make life simpler in the long run.
> (It does worry me a bit what happens to new qemu on old libvirt)

In the past, we've already stated that it is okay for new qemu to
require new libvirt.  What is not okay is for new libvirt to require new
qemu.  This change is okay given those rules.

-- 
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 v4 0/4] Convert 'status' of MigrationInfo from open-coded 'str' to enum type

[zhanghailiang]

ping...

Should this series be merged in 2.3 version? I noticed that 3.17 is hardfreeze time.

On 2015/3/9 14:45, zhanghailiang wrote:
> 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 MIGRATION_STATUS_FAILED,
> and it begin from 0, not its original -1. I think it has no side effect.
>
> Please review.
>
> v4:
> - Rename _ERROR to _FAILED in patch 1 instead of patch 4 (Eric)
> - Add Reviewd-by for patch 2 and 4
> - Fix some grammar in commit message of patch 4 (Eric)
>
> v3:
> - Use longer name for Migration status macro. (Eric Blake)
> - Rename internal-only typename 'MigrationStatus' (Eric, Dave, Markus)
> - Expose 'cancelling' state (Eric Blake)
>
> v2:
> - Remove '(since xyz)' strings. (Eric Blake)
>
> Thanks for their comments. ;)
>
> 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(-)
>

Re: [Qemu-devel] [for 2.3? PATCH v4 0/4] Convert 'status' of MigrationInfo from open-coded 'str' to enum type

[Eric Blake]

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

On 03/11/2015 07:06 PM, zhanghailiang wrote:
> ping...
> 
> Should this series be merged in 2.3 version? I noticed that 3.17 is
> hardfreeze time.

Yes, I'd be in favor of including it in 2.3; it's up to the maintainer
to make final decision, though.

-- 
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 --]