[Qemu-devel] [PATCH v2] migration: Convert 'status' of MigrationInfo to use an enum type

[Qemu-devel] [PATCH v2] 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: comppleted -> completed

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
v2:
- Remove '(since xyz)' strings. (Eric Blake)
---
 hmp.c                 |  7 ++++---
 migration/migration.c | 37 +++++++++++++++----------------------
 qapi-schema.json      | 37 ++++++++++++++++++++++++++++++++-----
 3 files changed, 51 insertions(+), 30 deletions(-)

diff --git a/hmp.c b/hmp.c
index 735097c..71539b3 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..533f4af 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -411,18 +411,45 @@
            'overflow': 'int' } }
 
 ##
+# @MigState:
+#
+# 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
+#
+# Notes: @cancelling is only used internally, and return @active to user
+#        instead of @cancelling, it make no difference for users.
+##
+{ '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 +480,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',
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v2] migration: Convert 'status' of MigrationInfo to use an enum type

[Eric Blake]

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

On 02/26/2015 11:19 PM, 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>
> ---
> v2:
> - Remove '(since xyz)' strings. (Eric Blake)
> ---
>  hmp.c                 |  7 ++++---
>  migration/migration.c | 37 +++++++++++++++----------------------
>  qapi-schema.json      | 37 ++++++++++++++++++++++++++++++++-----
>  3 files changed, 51 insertions(+), 30 deletions(-)

>      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. */

Rather than pollute the user-exposed enum with a state that we will
never report, can we come up with some internal-only method for tracking
cancelling separate from the enum?


> +++ b/qapi-schema.json
> @@ -411,18 +411,45 @@
>             'overflow': 'int' } }
>  
>  ##
> +# @MigState:

Do we have to abbreviate?  I guess leaving it like this makes the rest
of the existing code base have less churn (since it matches the spelling
of the enum that was previous interanl only), but it might look nicer as
MigrationState.  If you do decide to go with a longer name, it might be
nice to split this into a series, one patch that does only renames to
migration.c (but no code additions outside of that file), and the other
that moves the (now-correctly-named) enum into the public qapi file.

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

If the user can never see this state, I'd rather we think about a
solution that avoids advertising the state in the public api...

> +# @cancelled: cancelling migration is finished.
> +#
> +# @active: in the process of doing migration.
> +#
> +# @completed: migration is finished.
> +#
> +# Since: 2.3
> +#
> +# Notes: @cancelling is only used internally, and return @active to user
> +#        instead of @cancelling, it make no difference for users.

...rather than needing this note to air our dirty laundry.

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

[Dr. David Alan Gilbert]

* Eric Blake (eblake@redhat.com) wrote:
> On 02/26/2015 11:19 PM, 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>
> > ---
> > v2:
> > - Remove '(since xyz)' strings. (Eric Blake)
> > ---
> >  hmp.c                 |  7 ++++---
> >  migration/migration.c | 37 +++++++++++++++----------------------
> >  qapi-schema.json      | 37 ++++++++++++++++++++++++++++++++-----
> >  3 files changed, 51 insertions(+), 30 deletions(-)
> 
> >      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. */
> 
> Rather than pollute the user-exposed enum with a state that we will
> never report, can we come up with some internal-only method for tracking
> cancelling separate from the enum?

Well I guess we could just report it; but would that break any external tools?

IMHO this change is a sensible change, the legacy of 'cancelling' is already
there and it doesn't seem right to fix that at the same time as cleaning this
up.

Dave

> 
> 
> > +++ b/qapi-schema.json
> > @@ -411,18 +411,45 @@
> >             'overflow': 'int' } }
> >  
> >  ##
> > +# @MigState:
> 
> Do we have to abbreviate?  I guess leaving it like this makes the rest
> of the existing code base have less churn (since it matches the spelling
> of the enum that was previous interanl only), but it might look nicer as
> MigrationState.  If you do decide to go with a longer name, it might be
> nice to split this into a series, one patch that does only renames to
> migration.c (but no code additions outside of that file), and the other
> that moves the (now-correctly-named) enum into the public qapi file.
> 
> > +#
> > +# 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.
> > +#
> 
> If the user can never see this state, I'd rather we think about a
> solution that avoids advertising the state in the public api...
> 
> > +# @cancelled: cancelling migration is finished.
> > +#
> > +# @active: in the process of doing migration.
> > +#
> > +# @completed: migration is finished.
> > +#
> > +# Since: 2.3
> > +#
> > +# Notes: @cancelling is only used internally, and return @active to user
> > +#        instead of @cancelling, it make no difference for users.
> 
> ...rather than needing this note to air our dirty laundry.
> 
> -- 
> 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 v2] migration: Convert 'status' of MigrationInfo to use an enum type

[Eric Blake]

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

On 02/27/2015 10:07 AM, Dr. David Alan Gilbert wrote:
>>
>> Rather than pollute the user-exposed enum with a state that we will
>> never report, can we come up with some internal-only method for tracking
>> cancelling separate from the enum?
> 
> Well I guess we could just report it; but would that break any external tools?

It might - I seem to recall in the past that when we added a new state
string, that at least libvirt choked when encountering the unknown
string (but I don't recall if it was migration or something else).  At
least libvirt already has an enum tracking:

enum {
    QEMU_MONITOR_MIGRATION_STATUS_INACTIVE,
    QEMU_MONITOR_MIGRATION_STATUS_ACTIVE,
    QEMU_MONITOR_MIGRATION_STATUS_COMPLETED,
    QEMU_MONITOR_MIGRATION_STATUS_ERROR,
    QEMU_MONITOR_MIGRATION_STATUS_CANCELLED,
    QEMU_MONITOR_MIGRATION_STATUS_SETUP,

    QEMU_MONITOR_MIGRATION_STATUS_LAST
};

and a string mapping of just the following states:

VIR_ENUM_IMPL(qemuMonitorMigrationStatus,
              QEMU_MONITOR_MIGRATION_STATUS_LAST,
              "inactive", "active", "completed", "failed", "cancelled",
"setup")

Furthermore, the function qemuMigrationUpdateJobStatus() is doing
various things depending on the observed state, where the current trick
of treating 'cancelling' like 'active' would mean that changing
'cancelling' to be an independent state WOULD have observable behavior
change in libvirt.  But I don't know if the change would break things,
or if it would still end up resolving nicely (after all, cancelling only
occurs for a short window before the migration aborts anyway, so it
might just sort itself out when it finally gets to cancelled).

On the other hand, we can argue that clients that are unprepared to
handle new enum states gracefully are broken, and we also have the
argument that it is okay for a new qemu to require a new libvirt release
(the other direction is not okay - a new libvirt must not require
upgrading to a new qemu).  So exposing 'cancelling' may make this patch
easier.

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

[zhanghailiang]

On 2015/2/28 0:48, Eric Blake wrote:
> On 02/26/2015 11:19 PM, 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>
>> ---
>> v2:
>> - Remove '(since xyz)' strings. (Eric Blake)
>> ---
>>   hmp.c                 |  7 ++++---
>>   migration/migration.c | 37 +++++++++++++++----------------------
>>   qapi-schema.json      | 37 ++++++++++++++++++++++++++++++++-----
>>   3 files changed, 51 insertions(+), 30 deletions(-)
>
>>       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. */
>
> Rather than pollute the user-exposed enum with a state that we will
> never report, can we come up with some internal-only method for tracking
> cancelling separate from the enum?
>
>
>> +++ b/qapi-schema.json
>> @@ -411,18 +411,45 @@
>>              'overflow': 'int' } }
>>
>>   ##
>> +# @MigState:
>
> Do we have to abbreviate?  I guess leaving it like this makes the rest
> of the existing code base have less churn (since it matches the spelling
> of the enum that was previous interanl only), but it might look nicer as

Yes, this is the reason ..., agreed, i don't like the abbreviate,
But there is already a 'MigrationState' type defined:

typedef struct MigrationState MigrationState;

struct MigrationState
{
     int64_t bandwidth_limit;
     size_t bytes_xfer;
     size_t xfer_limit;
     QemuThread thread;
     QEMUBH *cleanup_bh;
     QEMUFile *file;

     ...

So, what about MigrationStatus ? ;)

> MigrationState.  If you do decide to go with a longer name, it might be
> nice to split this into a series, one patch that does only renames to
> migration.c (but no code additions outside of that file), and the other
> that moves the (now-correctly-named) enum into the public qapi file.
>
>> +#
>> +# 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.
>> +#
>
> If the user can never see this state, I'd rather we think about a
> solution that avoids advertising the state in the public api...
>
>> +# @cancelled: cancelling migration is finished.
>> +#
>> +# @active: in the process of doing migration.
>> +#
>> +# @completed: migration is finished.
>> +#
>> +# Since: 2.3
>> +#
>> +# Notes: @cancelling is only used internally, and return @active to user
>> +#        instead of @cancelling, it make no difference for users.
>
> ...rather than needing this note to air our dirty laundry.
>

Re: [Qemu-devel] [PATCH v2] migration: Convert 'status' of MigrationInfo to use an enum type

[zhanghailiang]

On 2015/2/28 1:42, Eric Blake wrote:
> On 02/27/2015 10:07 AM, Dr. David Alan Gilbert wrote:
>>>
>>> Rather than pollute the user-exposed enum with a state that we will
>>> never report, can we come up with some internal-only method for tracking
>>> cancelling separate from the enum?
>>
>> Well I guess we could just report it; but would that break any external tools?
>
> It might - I seem to recall in the past that when we added a new state
> string, that at least libvirt choked when encountering the unknown
> string (but I don't recall if it was migration or something else).  At
> least libvirt already has an enum tracking:
>
> enum {
>      QEMU_MONITOR_MIGRATION_STATUS_INACTIVE,
>      QEMU_MONITOR_MIGRATION_STATUS_ACTIVE,
>      QEMU_MONITOR_MIGRATION_STATUS_COMPLETED,
>      QEMU_MONITOR_MIGRATION_STATUS_ERROR,
>      QEMU_MONITOR_MIGRATION_STATUS_CANCELLED,
>      QEMU_MONITOR_MIGRATION_STATUS_SETUP,
>
>      QEMU_MONITOR_MIGRATION_STATUS_LAST
> };
>
> and a string mapping of just the following states:
>
> VIR_ENUM_IMPL(qemuMonitorMigrationStatus,
>                QEMU_MONITOR_MIGRATION_STATUS_LAST,
>                "inactive", "active", "completed", "failed", "cancelled",
> "setup")
>
> Furthermore, the function qemuMigrationUpdateJobStatus() is doing
> various things depending on the observed state, where the current trick
> of treating 'cancelling' like 'active' would mean that changing
> 'cancelling' to be an independent state WOULD have observable behavior
> change in libvirt.  But I don't know if the change would break things,

Er, i have tested with returning 'cancelling' to users, and
only when we try to cancel a migration, libvirt sometimes will report :
Migration: [ 69 %]^Cerror: internal error: unexpected migration status in cancelling.
But the cancelling process is still completed.

And, yes, it is very rare, depend on the time window. (In my test, i add a sleep of 5s
to extend the time between cancelling and cancelled.)

> or if it would still end up resolving nicely (after all, cancelling only
> occurs for a short window before the migration aborts anyway, so it
> might just sort itself out when it finally gets to cancelled).
>

> On the other hand, we can argue that clients that are unprepared to
> handle new enum states gracefully are broken, and we also have the
> argument that it is okay for a new qemu to require a new libvirt release
> (the other direction is not okay - a new libvirt must not require

Agreed, upgrade libvirt is reasonable.
So, should i send v3 with exposing 'cancelling'to user, and CC libvirt ?

Thanks.

> upgrading to a new qemu).  So exposing 'cancelling' may make this patch
> easier.
>

Re: [Qemu-devel] [PATCH v2] migration: Convert 'status' of MigrationInfo to use an enum type

[Eric Blake]

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

On 02/27/2015 07:54 PM, zhanghailiang wrote:
> On 2015/2/28 0:48, Eric Blake wrote:
>> On 02/26/2015 11:19 PM, zhanghailiang wrote:
>>> The original 'status' is an open-coded 'str' type, convert it to use an
>>> enum type.

>>> +# @MigState:
>>
>> Do we have to abbreviate?  I guess leaving it like this makes the rest
>> of the existing code base have less churn (since it matches the spelling
>> of the enum that was previous interanl only), but it might look nicer as
> 
> Yes, this is the reason ..., agreed, i don't like the abbreviate,
> But there is already a 'MigrationState' type defined:
> 

> 
> So, what about MigrationStatus ? ;)

That would be fine with me.

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

[Eric Blake]

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

On 02/27/2015 08:09 PM, zhanghailiang wrote:
> On 2015/2/28 1:42, Eric Blake wrote:
>> On 02/27/2015 10:07 AM, Dr. David Alan Gilbert wrote:
>>>>
>>>> Rather than pollute the user-exposed enum with a state that we will
>>>> never report, can we come up with some internal-only method for
>>>> tracking
>>>> cancelling separate from the enum?
>>>
>>> Well I guess we could just report it; but would that break any
>>> external tools?
>>
>> It might - I seem to recall in the past that when we added a new state
>> string, that at least libvirt choked when encountering the unknown
>> string (but I don't recall if it was migration or something else).


> 
> Er, i have tested with returning 'cancelling' to users, and
> only when we try to cancel a migration, libvirt sometimes will report :
> Migration: [ 69 %]^Cerror: internal error: unexpected migration status
> in cancelling.
> But the cancelling process is still completed.
> 
> And, yes, it is very rare, depend on the time window. (In my test, i add
> a sleep of 5s
> to extend the time between cancelling and cancelled.)
> 
>> or if it would still end up resolving nicely (after all, cancelling only
>> occurs for a short window before the migration aborts anyway, so it
>> might just sort itself out when it finally gets to cancelled).
>>
> 
>> On the other hand, we can argue that clients that are unprepared to
>> handle new enum states gracefully are broken, and we also have the
>> argument that it is okay for a new qemu to require a new libvirt release
>> (the other direction is not okay - a new libvirt must not require
> 
> Agreed, upgrade libvirt is reasonable.
> So, should i send v3 with exposing 'cancelling'to user, and CC libvirt ?

Yes, that sounds reasonable to me


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

[zhanghailiang]

On 2015/3/2 23:56, Eric Blake wrote:
> On 02/27/2015 07:54 PM, zhanghailiang wrote:
>> On 2015/2/28 0:48, Eric Blake wrote:
>>> On 02/26/2015 11:19 PM, zhanghailiang wrote:
>>>> The original 'status' is an open-coded 'str' type, convert it to use an
>>>> enum type.
>
>>>> +# @MigState:
>>>
>>> Do we have to abbreviate?  I guess leaving it like this makes the rest
>>> of the existing code base have less churn (since it matches the spelling
>>> of the enum that was previous interanl only), but it might look nicer as
>>
>> Yes, this is the reason ..., agreed, i don't like the abbreviate,
>> But there is already a 'MigrationState' type defined:
>>
>
>>
>> So, what about MigrationStatus ? ;)
>
> That would be fine with me.
>

Bad news, this name has also been used :(

In hmp.c:

typedef struct MigrationStatus
{
     QEMUTimer *timer;
     Monitor *mon;
     bool is_block_migration;
} MigrationStatus

Re: [Qemu-devel] [PATCH v2] migration: Convert 'status' of MigrationInfo to use an enum type

[Dr. David Alan Gilbert]

* zhanghailiang (zhang.zhanghailiang@huawei.com) wrote:
> On 2015/3/2 23:56, Eric Blake wrote:
> >On 02/27/2015 07:54 PM, zhanghailiang wrote:
> >>On 2015/2/28 0:48, Eric Blake wrote:
> >>>On 02/26/2015 11:19 PM, zhanghailiang wrote:
> >>>>The original 'status' is an open-coded 'str' type, convert it to use an
> >>>>enum type.
> >
> >>>>+# @MigState:
> >>>
> >>>Do we have to abbreviate?  I guess leaving it like this makes the rest
> >>>of the existing code base have less churn (since it matches the spelling
> >>>of the enum that was previous interanl only), but it might look nicer as
> >>
> >>Yes, this is the reason ..., agreed, i don't like the abbreviate,
> >>But there is already a 'MigrationState' type defined:
> >>
> >
> >>
> >>So, what about MigrationStatus ? ;)
> >
> >That would be fine with me.
> >
> 
> Bad news, this name has also been used :(

Hmm, how about 'MigrationStage'   ?

Dave

> 
> In hmp.c:
> 
> typedef struct MigrationStatus
> {
>     QEMUTimer *timer;
>     Monitor *mon;
>     bool is_block_migration;
> } MigrationStatus
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2] migration: Convert 'status' of MigrationInfo to use an enum type

[Eric Blake]

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

On 03/03/2015 12:15 AM, zhanghailiang wrote:

>>>
>>> Yes, this is the reason ..., agreed, i don't like the abbreviate,
>>> But there is already a 'MigrationState' type defined:
>>>
>>
>>>
>>> So, what about MigrationStatus ? ;)
>>
>> That would be fine with me.
>>
> 
> Bad news, this name has also been used :(
> 
> In hmp.c:
> 
> typedef struct MigrationStatus

You know, you could always rename the internal-only conflict into
something else so that the publicly exported typename is nice.  Yeah,
that makes the series longer, but it should be all mechanical
conversions, right?  I'm not going to be too picky about what color we
paint this bikeshed, 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 --]

Re: [Qemu-devel] [PATCH v2] migration: Convert 'status' of MigrationInfo to use an enum type

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 03/03/2015 12:15 AM, zhanghailiang wrote:
>
>>>>
>>>> Yes, this is the reason ..., agreed, i don't like the abbreviate,
>>>> But there is already a 'MigrationState' type defined:
>>>>
>>>
>>>>
>>>> So, what about MigrationStatus ? ;)
>>>
>>> That would be fine with me.
>>>
>> 
>> Bad news, this name has also been used :(
>> 
>> In hmp.c:
>> 
>> typedef struct MigrationStatus
>
> You know, you could always rename the internal-only conflict into
> something else so that the publicly exported typename is nice.  Yeah,
> that makes the series longer,

by *two* patch hunks updating the four occurences of MigrationStatus,

>                               but it should be all mechanical
> conversions, right?  I'm not going to be too picky about what color we
> paint this bikeshed, though.

Me neither, but we shouldn't compromise on external interfaces just to
avoid a bit of internal churn.  Pick a good name, then do what it takes.

Re: [Qemu-devel] [PATCH v2] migration: Convert 'status' of MigrationInfo to use an enum type

[zhanghailiang]

On 2015/3/4 1:21, Eric Blake wrote:
> On 03/03/2015 12:15 AM, zhanghailiang wrote:
>
>>>>
>>>> Yes, this is the reason ..., agreed, i don't like the abbreviate,
>>>> But there is already a 'MigrationState' type defined:
>>>>
>>>
>>>>
>>>> So, what about MigrationStatus ? ;)
>>>
>>> That would be fine with me.
>>>
>>
>> Bad news, this name has also been used :(
>>
>> In hmp.c:
>>
>> typedef struct MigrationStatus
>
> You know, you could always rename the internal-only conflict into
> something else so that the publicly exported typename is nice.  Yeah,
> that makes the series longer, but it should be all mechanical

OK, I will try ..., Thanks. :)

> conversions, right?  I'm not going to be too picky about what color we
> paint this bikeshed, though.
>

Re: [Qemu-devel] [PATCH v2] migration: Convert 'status' of MigrationInfo to use an enum type

[zhanghailiang]

On 2015/3/4 15:48, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 03/03/2015 12:15 AM, zhanghailiang wrote:
>>
>>>>>
>>>>> Yes, this is the reason ..., agreed, i don't like the abbreviate,
>>>>> But there is already a 'MigrationState' type defined:
>>>>>
>>>>
>>>>>
>>>>> So, what about MigrationStatus ? ;)
>>>>
>>>> That would be fine with me.
>>>>
>>>
>>> Bad news, this name has also been used :(
>>>
>>> In hmp.c:
>>>
>>> typedef struct MigrationStatus
>>
>> You know, you could always rename the internal-only conflict into
>> something else so that the publicly exported typename is nice.  Yeah,
>> that makes the series longer,
>
> by *two* patch hunks updating the four occurences of MigrationStatus,
>
>>                                but it should be all mechanical
>> conversions, right?  I'm not going to be too picky about what color we
>> paint this bikeshed, though.
>
> Me neither, but we shouldn't compromise on external interfaces just to
> avoid a bit of internal churn.  Pick a good name, then do what it takes.
>
>

OK, will do that, Thanks.

Re: [Qemu-devel] [PATCH v2] migration: Convert 'status' of MigrationInfo to use an enum type

[zhanghailiang]

On 2015/3/3 16:59, Dr. David Alan Gilbert wrote:
> * zhanghailiang (zhang.zhanghailiang@huawei.com) wrote:
>> On 2015/3/2 23:56, Eric Blake wrote:
>>> On 02/27/2015 07:54 PM, zhanghailiang wrote:
>>>> On 2015/2/28 0:48, Eric Blake wrote:
>>>>> On 02/26/2015 11:19 PM, zhanghailiang wrote:
>>>>>> The original 'status' is an open-coded 'str' type, convert it to use an
>>>>>> enum type.
>>>
>>>>>> +# @MigState:
>>>>>
>>>>> Do we have to abbreviate?  I guess leaving it like this makes the rest
>>>>> of the existing code base have less churn (since it matches the spelling
>>>>> of the enum that was previous interanl only), but it might look nicer as
>>>>
>>>> Yes, this is the reason ..., agreed, i don't like the abbreviate,
>>>> But there is already a 'MigrationState' type defined:
>>>>
>>>
>>>>
>>>> So, what about MigrationStatus ? ;)
>>>
>>> That would be fine with me.
>>>
>>
>> Bad news, this name has also been used :(
>
> Hmm, how about 'MigrationStage'   ?

Er, thanks for your reply, i will follow the suggestion of Eric and Markus,
Rename the original 'MigrationStatus' which is used internally-only.
Keep using MigrationStatus in the new places.

I will rename 'MigrationStatus' (only used for updating status of migration periodically)
to 'MigrationDynamicStatus'. :)

>> In hmp.c:
>>
>> typedef struct MigrationStatus
>> {
>>      QEMUTimer *timer;
>>      Monitor *mon;
>>      bool is_block_migration;
>> } MigrationStatus
>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
> .
>

Re: [Qemu-devel] [PATCH v2] migration: Convert 'status' of MigrationInfo to use an enum type

[Dr. David Alan Gilbert]

* zhanghailiang (zhang.zhanghailiang@huawei.com) wrote:
> On 2015/3/3 16:59, Dr. David Alan Gilbert wrote:
> >* zhanghailiang (zhang.zhanghailiang@huawei.com) wrote:
> >>On 2015/3/2 23:56, Eric Blake wrote:
> >>>On 02/27/2015 07:54 PM, zhanghailiang wrote:
> >>>>On 2015/2/28 0:48, Eric Blake wrote:
> >>>>>On 02/26/2015 11:19 PM, zhanghailiang wrote:
> >>>>>>The original 'status' is an open-coded 'str' type, convert it to use an
> >>>>>>enum type.
> >>>
> >>>>>>+# @MigState:
> >>>>>
> >>>>>Do we have to abbreviate?  I guess leaving it like this makes the rest
> >>>>>of the existing code base have less churn (since it matches the spelling
> >>>>>of the enum that was previous interanl only), but it might look nicer as
> >>>>
> >>>>Yes, this is the reason ..., agreed, i don't like the abbreviate,
> >>>>But there is already a 'MigrationState' type defined:
> >>>>
> >>>
> >>>>
> >>>>So, what about MigrationStatus ? ;)
> >>>
> >>>That would be fine with me.
> >>>
> >>
> >>Bad news, this name has also been used :(
> >
> >Hmm, how about 'MigrationStage'   ?
> 
> Er, thanks for your reply, i will follow the suggestion of Eric and Markus,
> Rename the original 'MigrationStatus' which is used internally-only.
> Keep using MigrationStatus in the new places.
> 
> I will rename 'MigrationStatus' (only used for updating status of migration periodically)
> to 'MigrationDynamicStatus'. :)

OK, Since it's only used in HMP you could rename it to HMPMigrationStatus.

Dave

> 
> >>In hmp.c:
> >>
> >>typedef struct MigrationStatus
> >>{
> >>     QEMUTimer *timer;
> >>     Monitor *mon;
> >>     bool is_block_migration;
> >>} MigrationStatus
> >>
> >>
> >--
> >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
> >.
> >
> 
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2] migration: Convert 'status' of MigrationInfo to use an enum type

[zhanghailiang]

On 2015/3/4 20:50, Dr. David Alan Gilbert wrote:
> * zhanghailiang (zhang.zhanghailiang@huawei.com) wrote:
>> On 2015/3/3 16:59, Dr. David Alan Gilbert wrote:
>>> * zhanghailiang (zhang.zhanghailiang@huawei.com) wrote:
>>>> On 2015/3/2 23:56, Eric Blake wrote:
>>>>> On 02/27/2015 07:54 PM, zhanghailiang wrote:
>>>>>> On 2015/2/28 0:48, Eric Blake wrote:
>>>>>>> On 02/26/2015 11:19 PM, zhanghailiang wrote:
>>>>>>>> The original 'status' is an open-coded 'str' type, convert it to use an
>>>>>>>> enum type.
>>>>>
>>>>>>>> +# @MigState:
>>>>>>>
>>>>>>> Do we have to abbreviate?  I guess leaving it like this makes the rest
>>>>>>> of the existing code base have less churn (since it matches the spelling
>>>>>>> of the enum that was previous interanl only), but it might look nicer as
>>>>>>
>>>>>> Yes, this is the reason ..., agreed, i don't like the abbreviate,
>>>>>> But there is already a 'MigrationState' type defined:
>>>>>>
>>>>>
>>>>>>
>>>>>> So, what about MigrationStatus ? ;)
>>>>>
>>>>> That would be fine with me.
>>>>>
>>>>
>>>> Bad news, this name has also been used :(
>>>
>>> Hmm, how about 'MigrationStage'   ?
>>
>> Er, thanks for your reply, i will follow the suggestion of Eric and Markus,
>> Rename the original 'MigrationStatus' which is used internally-only.
>> Keep using MigrationStatus in the new places.
>>
>> I will rename 'MigrationStatus' (only used for updating status of migration periodically)
>> to 'MigrationDynamicStatus'. :)
>
> OK, Since it's only used in HMP you could rename it to HMPMigrationStatus.

OK, Thanks.

>>>> In hmp.c:
>>>>
>>>> typedef struct MigrationStatus
>>>> {
>>>>      QEMUTimer *timer;
>>>>      Monitor *mon;
>>>>      bool is_block_migration;
>>>> } MigrationStatus
>>>>
>>>>
>>> --
>>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>>
>>> .
>>>
>>
>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
> .
>