| Message ID | 1424346029-5410-2-git-send-email-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > -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 <dgilbert@redhat.com> > Reviewed-by: Juan Quintela <quintela@redhat.com> > --- > 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 ¤t_migration; > } > > +/* > + * Called on -incoming with a defer: uri. The colon in "defer:" is inaccurate, because... > + * 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) In my opinion, multiple -incoming should behave like command line options usually do: last one wins silently.
* Markus Armbruster (armbru@redhat.com) wrote: > "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: > > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > -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 <dgilbert@redhat.com> > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > --- > > 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 ¤t_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 / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > * Markus Armbruster (armbru@redhat.com) wrote: >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes: >> >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >> > >> > -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 <dgilbert@redhat.com> >> > Reviewed-by: Juan Quintela <quintela@redhat.com> >> > --- >> > 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 ¤t_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.
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 ¤t_migration; } +/* + * Called on -incoming with a defer: uri. + * 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")) { + 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); } }