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 <address@hidden>
> > ---
> > 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.