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

[zhanghailiang <zhang.zhanghailiang@huawei.com>]

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