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

[Eric Blake <eblake@redhat.com>]

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