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

[Markus Armbruster]

"Dr. David Alan Gilbert" <address@hidden> writes:

> * 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.

Makes sense.

> 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.

One man's action is another man's value.

There's ample precedence for "last one wins" when command line options
conflict.  For instance, ls -l conflicts with -C, -m and -x, and POSIX
explicitly specifies "last one wins".  Rather convenient when you almost
always want one option, and use alias to get it without having to type
it all the time, but occasionally want a conflicting option, and can
just give it despite your alias.