Re: [PATCH 1/2] migration: move wait-unplug loop to its own function

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

* Juan Quintela (quintela@redhat.com) wrote:
> Laurent Vivier <lvivier@redhat.com> wrote:
> > The loop is used in migration_thread() and bg_migration_thread(),
> > so we can move it to its own function and call it from these both places.
> >
> > Moreover, in migration_thread() we have a wrong state transition from
> > SETUP to ACTIVE while state could be WAIT_UNPLUG. This is correctly
> > managed in bg_migration_thread() so use this code instead.
> >
> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> If you have to repost:
> 
> 
> > +/*
> > + * if failover devices are present, wait they are completely
> > + * unplugged
> > + */
> > +
> > +static void qemu_savevm_wait_unplug(MigrationState *s, int old_state,
> > +                                    int new_state)
> 
> old_state and new state are always the same. SETUP -> ACTIVE.  I think
> we can hardcode them.
> 
> 
> > +{
> > +    if (qemu_savevm_state_guest_unplug_pending()) {
> > +        migrate_set_state(&s->state, old_state, MIGRATION_STATUS_WAIT_UNPLUG);
> > +
> > +        while (s->state == MIGRATION_STATUS_WAIT_UNPLUG &&
> > +               qemu_savevm_state_guest_unplug_pending()) {
> > +            qemu_sem_timedwait(&s->wait_unplug_sem, 250);
> 
> I still don't understand why are we using a semaphore when we just want
> a timer :-(
> 
> Yes, this is independent of this patch.

So yes I was going to ask on the 2nd patch; no one anywhere seems to set
that semaphore?

Dave

-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK