Re: [Qemu-devel] [PATCH 17/23] migration: make sure we always have a migration state

[Anthony Liguori]

On 10/04/2011 09:49 AM, Juan Quintela wrote:
> Anthony Liguori<address@hidden>  wrote:
> > On 09/23/2011 07:57 AM, Juan Quintela wrote:
> > > This cleans up a lot the code as we don't have to check anymore if
> > > the variable is NULL or not.
> > >
> > > We don't make it static, because when we integrate fault tolerance, we
> > > can have several migrations at once.
> > >
> > > Signed-off-by: Juan Quintela<address@hidden>
> > > ---
> > >    migration.c |  126 +++++++++++++++++++++++++---------------------------------
> > >    1 files changed, 54 insertions(+), 72 deletions(-)
> > >
> > > diff --git a/migration.c b/migration.c
> > > index 1cc21f0..c382383 100644
> > > --- a/migration.c
> > > +++ b/migration.c
> > > @@ -34,7 +34,13 @@
> > >    /* Migration speed throttling */
> > >    static int64_t max_throttle = (32<<   20);
> > >
> > > -static MigrationState *current_migration;
> > > +/* When we add fault tolerance, we could have several
> > > +   migrations at once.  For now we don't need to add
> > > +   dynamic creation of migration */
> > > +static MigrationState current_migration_static = {
> > > +    .state = MIG_STATE_NONE,
> > > +};
> > > +static MigrationState *current_migration =&current_migration_static;
> >
> > This doesn't make sense to me.  We can have two migration states that
> > are both in the MIG_STATE_NONE?  What does that actually mean?
> >
> > I don't see the point in making migration state nullable via the state
> > member other than avoiding the if (s == NULL) check.
>
> It avoids s==NULL checks,

In favor of s->state == MIG_STATE_NONE.

> and it also avoids having to have new
> variables (max_throotle) just because we don't have a migration state
> handy.

The intention of the global variable is to set a default for new sessions.  I 
can imagine a world where if you had parallel migrations, you could either 
toggle the default (the global variable) or the specific parameter within a 
single migration session.

But it's not about features.  It's a general reluctances to heavily modify the 
code to rely on a global instead of passing the state around.  Here's a pretty 
good short write up of why this is a Bad Thing.

http://stackoverflow.com/questions/1392315/problems-with-singleton-pattern

Ultimately, MIG_STATE_NONE shouldn't be needed because we should not be relying 
on a singleton accessor to get the MigrationState.  A better cleanup would be to 
further pass MigrationState between functions and eliminate the need for a 
global at all.

Regards,

Anthony Liguori

> Obviously the problem can't be the size (the variable is smaller that
> all the code tests for NULL).
>
> For me, it is easier to understand that we have an state (migration
> never attempted) with the same states that the other migrations states.
>
> Later, Juan.