diff mbox

[RFC] migration: Convert 'status' of MigrationInfo to use an enum type

Message ID 1424937492-10720-1-git-send-email-zhang.zhanghailiang@huawei.com
State New
Headers show

Commit Message

Zhanghailiang Feb. 26, 2015, 7:58 a.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.

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

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
Hi,

This conversion from open-coded 'str' type to enum type for 'status'
is suggested by Eric, thanks for his comment.

Actually, I will add a MIG_STATE_COLO state for COLO, and i also
saw David added MIG_STATE_POSTCOPY_ACTIVE for postcopy, after
this conversion, it will be more convenient for us to add a new state.

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.

Thanks
---
 hmp.c                 |  7 ++++---
 migration/migration.c | 37 +++++++++++++++----------------------
 qapi-schema.json      | 34 +++++++++++++++++++++++++++++-----
 3 files changed, 48 insertions(+), 30 deletions(-)

Comments

Eric Blake Feb. 26, 2015, 3:24 p.m. UTC | #1 
On 02/26/2015 12:58 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: comppleted -> completed
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---


> +++ b/qapi-schema.json
> @@ -411,18 +411,42 @@
>             'overflow': 'int' } }
>  
>  ##
> +# @MigState:
> +#
> +# An enumeration of migration status.
> +#
> +# @failed: some error occurred during migration process. (since 0.14.0)
> +#
> +# @none: no migration has happened ever.

s/happened ever/ever happened/

> +#
> +# @setup: migration process has been initiated. (since 0.14.0)
> +#
> +# @cancelling: in the process of cancelling migration. (since 2.0)

It's weird to advertise this enum value when the code goes to lengths to
hide it behind 'active', but I guess it's okay.

> +#
> +# @cancelled: cancelling migration is finished. (since 0.14.0)
> +#
> +# @active: in the process of doing migration. (since 0.14.0)
> +#
> +# @completed: migration is finished. (since 0.14.0)
> +#
> +# Since: 2.3

It's weird to see various enum values stating (since xyz) that is older
than 2.3.  Generally, we only use that for an enum value that is newer
than the enumeration.  I'd be just fine if you got rid of all of the
per-value '(since xyz)' strings.

Otherwise, looks good to me.
Zhanghailiang Feb. 27, 2015, 1:23 a.m. UTC | #2 
On 2015/2/26 23:24, Eric Blake wrote:
> On 02/26/2015 12:58 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: comppleted -> completed
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>
>
>> +++ b/qapi-schema.json
>> @@ -411,18 +411,42 @@
>>              'overflow': 'int' } }
>>
>>   ##
>> +# @MigState:
>> +#
>> +# An enumeration of migration status.
>> +#
>> +# @failed: some error occurred during migration process. (since 0.14.0)
>> +#
>> +# @none: no migration has happened ever.
>
> s/happened ever/ever happened/
>

>> +#
>> +# @setup: migration process has been initiated. (since 0.14.0)
>> +#
>> +# @cancelling: in the process of cancelling migration. (since 2.0)
>
> It's weird to advertise this enum value when the code goes to lengths to
> hide it behind 'active', but I guess it's okay.
>

Hmm, yes, this state is only used internally, and will return 'active' to user
instead, it is a problem left over by history, which introduced by 51cf4c1a commit.
It is no side effect, maybe a NOTE should be added.

>> +#
>> +# @cancelled: cancelling migration is finished. (since 0.14.0)
>> +#
>> +# @active: in the process of doing migration. (since 0.14.0)
>> +#
>> +# @completed: migration is finished. (since 0.14.0)
>> +#
>> +# Since: 2.3
>
> It's weird to see various enum values stating (since xyz) that is older
> than 2.3.  Generally, we only use that for an enum value that is newer
> than the enumeration.  I'd be just fine if you got rid of all of the
> per-value '(since xyz)' strings.
>

OK, will fix in v2.

> Otherwise, looks good to me.
>

Thanks.
zhanghailiang
diff mbox

Patch

diff --git a/hmp.c b/hmp.c
index 3825b29..8cbd135 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",
+                       MigState_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 == MIG_STATE_ACTIVE ||
+        info->status == MIG_STATE_SETUP) {
         if (info->has_disk) {
             int progress;
 
diff --git a/migration/migration.c b/migration/migration.c
index b3adbc6..8b8fbbf 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -26,16 +26,6 @@ 
 #include "qmp-commands.h"
 #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,
-};
-
 #define MAX_THROTTLE  (32 << 20)      /* Migration speed throttling */
 
 /* Amount of time to allocate to each "chunk" of bandwidth-throttled
@@ -189,13 +179,16 @@  MigrationInfo *qmp_query_migrate(Error **errp)
         break;
     case MIG_STATE_SETUP:
         info->has_status = true;
-        info->status = g_strdup("setup");
+        info->status = MIG_STATE_SETUP;
         info->has_total_time = false;
         break;
     case MIG_STATE_ACTIVE:
     case MIG_STATE_CANCELLING:
         info->has_status = true;
-        info->status = g_strdup("active");
+        /* Note: when the real state of migration is 'cancelling',
+         we still return 'active' status to user, it makes no difference
+         for user. */
+        info->status = MIG_STATE_ACTIVE;
         info->has_total_time = true;
         info->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME)
             - s->total_time;
@@ -231,7 +224,7 @@  MigrationInfo *qmp_query_migrate(Error **errp)
         get_xbzrle_cache_stats(info);
 
         info->has_status = true;
-        info->status = g_strdup("completed");
+        info->status = MIG_STATE_COMPLETED;
         info->has_total_time = true;
         info->total_time = s->total_time;
         info->has_downtime = true;
@@ -251,13 +244,13 @@  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 MIG_STATE_FAILED:
         info->has_status = true;
-        info->status = g_strdup("failed");
+        info->status = MIG_STATE_FAILED;
         break;
     case MIG_STATE_CANCELLED:
         info->has_status = true;
-        info->status = g_strdup("cancelled");
+        info->status = MIG_STATE_CANCELLED;
         break;
     }
 
@@ -322,8 +315,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 = MIG_STATE_FAILED;
+    trace_migrate_set_state(MIG_STATE_FAILED);
     notifier_list_notify(&migration_state_notifiers, s);
 }
 
@@ -376,7 +369,7 @@  bool migration_has_finished(MigrationState *s)
 bool migration_has_failed(MigrationState *s)
 {
     return (s->state == MIG_STATE_CANCELLED ||
-            s->state == MIG_STATE_ERROR);
+            s->state == MIG_STATE_FAILED);
 }
 
 static MigrationState *migrate_init(const MigrationParams *params)
@@ -465,7 +458,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 = MIG_STATE_FAILED;
         return;
     }
 
@@ -627,7 +620,7 @@  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, MIG_STATE_ACTIVE, MIG_STATE_FAILED);
                     break;
                 }
 
@@ -639,7 +632,7 @@  static void *migration_thread(void *opaque)
         }
 
         if (qemu_file_get_error(s->file)) {
-            migrate_set_state(s, MIG_STATE_ACTIVE, MIG_STATE_ERROR);
+            migrate_set_state(s, MIG_STATE_ACTIVE, MIG_STATE_FAILED);
             break;
         }
         current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
diff --git a/qapi-schema.json b/qapi-schema.json
index e16f8eb..89b14a9 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -411,18 +411,42 @@ 
            'overflow': 'int' } }
 
 ##
+# @MigState:
+#
+# An enumeration of migration status.
+#
+# @failed: some error occurred during migration process. (since 0.14.0)
+#
+# @none: no migration has happened ever.
+#
+# @setup: migration process has been initiated. (since 0.14.0)
+#
+# @cancelling: in the process of cancelling migration. (since 2.0)
+#
+# @cancelled: cancelling migration is finished. (since 0.14.0)
+#
+# @active: in the process of doing migration. (since 0.14.0)
+#
+# @completed: migration is finished. (since 0.14.0)
+#
+# Since: 2.3
+##
+{ 'enum': 'MigState',
+  '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': 'MigState', '*ram': 'MigrationStats',
            '*disk': 'MigrationStats',
            '*xbzrle-cache': 'XBZRLECacheStats',
            '*total-time': 'int',