Re: [Qemu-devel] [PATCH 2/3] Add migrate -u option for -incoming pause

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

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Once a qemu has been started with -incoming pause   the
> > migration can be started by issuing:
> >
> > migrate -u uri
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> > -		      "(base image shared between src and destination)",
> > +		      "(base image shared between src and destination)"
> > +                      "\n\t\t\t -u unpauses an incoming migration started with "
> > +                      "-incoming pause using the given uri.",
> 
> Spaces vs tabs.
> 
> > +        -u to unpause an incoming migration started with -incoming pause
> 
> more spaces

All this is reworked anyway since I split it out.

> > -    qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err);
> > +    qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, !!unpause, unpause,
> > +                &err);
> 
> I don't claim to understand QMP, but this whole bussines of !!foo, foo
> is getting confusing, no?

Yes, and it's very very easy to screw up and get them in the wrong order.

> No, this is not relaced to this patch.
> 
> >  {
> >      Error *local_err = NULL;
> >      MigrationState *s = migrate_get_current();
> > @@ -450,6 +450,25 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> >          return;
> >      }
> >  
> 
> I would preffer something like:
> 
>     if (runstate_check(RUN_STATE_INMIGRATE)) {
>         if (unpause) {
>             ... unpause code
>         }
>     } else {
>         error_setg(errp, "Guest is waiting for an incoming migration");
>         return;
>     }
> 
>     if (unpause) {
>         error_setg(errp, "Guest is waiting for an incoming migration");
>         return;
>     }
> 
>     if (s->state == MIG_STATE_ACTIVE || s->state == MIG_STATE_SETUP ||
>         s->state == MIG_STATE_CANCELLING) {
>         error_set(errp, QERR_MIGRATION_ACTIVE);
>         return;
>     }
> 
>     if (qemu_savevm_state_blocked(errp)) {
>         return;
>     }
> 
> .... and now continue with the rest ...

Again, all gone in the new version.

> Thinking more about this problem, I am not sure this is the "cleanest
> approach".   What do you think of:
> 
> - create RUN_STATE_INMIGRATE_PAUSED
>   bonus: no need of paused_incoming variable

That we could do separately; doing that means carefully looking at
the existing users of RUN_STATE_INMIGRATE, and since it's a
visible state that includes anything that uses the interface.

> - create a new migrate_incoming command
> 
> And then we have cleaner separation of what we are doing?

Done.

Dave

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