Re: [Qemu-devel] [PATCH v3 1/3] Add -incoming defer

[Dr. David Alan Gilbert]

* Markus Armbruster (address@hidden) wrote:
> "Dr. David Alan Gilbert (git)" <address@hidden> writes:
> 
> > From: "Dr. David Alan Gilbert" <address@hidden>
> >
> > -incoming defer causes qemu to wait for an incoming migration
> > to be specified later.  The monitor can be used to set migration
> > capabilities that may affect the incoming connection process.
> >
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > Reviewed-by: Juan Quintela <address@hidden>
> > ---
> >  migration/migration.c | 29 +++++++++++++++++++++++------
> >  1 file changed, 23 insertions(+), 6 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index b3adbc6..f3d49d5 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -49,6 +49,8 @@ enum {
> >  static NotifierList migration_state_notifiers =
> >      NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
> >  
> > +static bool deferred_incoming;
> > +
> >  /* When we add fault tolerance, we could have several
> >     migrations at once.  For now we don't need to add
> >     dynamic creation of migration */
> > @@ -65,25 +67,40 @@ MigrationState *migrate_get_current(void)
> >      return &current_migration;
> >  }
> >  
> > +/*
> > + * Called on -incoming with a defer: uri.
> 
> The colon in "defer:" is inaccurate, because...

True, but probably not worth the respin.

> > + * The migration can be started later after any parameters have been
> > + * changed.
> > + */
> > +static void deferred_incoming_migration(Error **errp)
> > +{
> > +    if (deferred_incoming) {
> > +        error_setg(errp, "Incoming migration already deferred");
> > +    }
> > +    deferred_incoming = true;
> > +}
> > +
> >  void qemu_start_incoming_migration(const char *uri, Error **errp)
> >  {
> >      const char *p;
> >  
> > -    if (strstart(uri, "tcp:", &p))
> > +    if (!strcmp(uri, "defer")) {
> 
> ... you recognize exactly "defer" here.  <pedantic>Which makes it not an
> URI</pedantic>.
> 
> > +        deferred_incoming_migration(errp);
> > +    } else if (strstart(uri, "tcp:", &p)) {
> >          tcp_start_incoming_migration(p, errp);
> >  #ifdef CONFIG_RDMA
> > -    else if (strstart(uri, "rdma:", &p))
> > +    } else if (strstart(uri, "rdma:", &p)) {
> >          rdma_start_incoming_migration(p, errp);
> >  #endif
> >  #if !defined(WIN32)
> > -    else if (strstart(uri, "exec:", &p))
> > +    } else if (strstart(uri, "exec:", &p)) {
> >          exec_start_incoming_migration(p, errp);
> > -    else if (strstart(uri, "unix:", &p))
> > +    } else if (strstart(uri, "unix:", &p)) {
> >          unix_start_incoming_migration(p, errp);
> > -    else if (strstart(uri, "fd:", &p))
> > +    } else if (strstart(uri, "fd:", &p)) {
> >          fd_start_incoming_migration(p, errp);
> >  #endif
> > -    else {
> > +    } else {
> >          error_setg(errp, "unknown migration protocol: %s", uri);
> >      }
> >  }
> 
> How did you test the new error?
> 
> I tried, but ran into this preexisting bug:
> 
>     $ qemu-system-x86_64 -nodefaults -S -display none -incoming defer 
> -incoming defer
>     ERROR: invalid runstate transition: 'inmigrate' -> 'inmigrate'
>     Aborted (core dumped)

Fun; yes that's an existing bug that triggers with -incoming tcp::4444 
-incoming tcp::4445 
I'll think about that separately.

The way I tested the error was like this:

$ bin/qemu-system-x86_64 -nographic -incoming defer
QEMU 2.2.50 monitor - type 'help' for more information                          
                                                                                
                                                                             
(qemu) migrate_incoming defer
Incoming migration already deferred                                             
                                                                                
                                                                             
(qemu)

> In my opinion, multiple -incoming should behave like command line
> options usually do: last one wins silently.

Either that or error; to me this feels like it should probably
error; I'd make the distinction between options that set a value
(which works as a last one wins), or options that invoke an action,
for options that invoke an action it feels wrong to me to allow
incompatible options.

Dave



--
Dr. David Alan Gilbert / address@hidden / Manchester, UK