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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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