diff mbox

[v3,3/4] migration: Convert 'status' of MigrationInfo to use an enum type

Message ID 1425478176-12044-4-git-send-email-zhang.zhanghailiang@huawei.com
State New
Headers show

Commit Message

Zhanghailiang March 4, 2015, 2:09 p.m. UTC
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(-)

Comments

Markus Armbruster March 5, 2015, 6:34 p.m. UTC | #1
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',
Zhanghailiang March 6, 2015, 8:45 a.m. UTC | #2
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',
>
> .
>
Eric Blake March 6, 2015, 4:17 p.m. UTC | #3
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.
diff mbox

Patch

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',