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

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

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