diff mbox series

[24/29] migration, vl: start migration via qmp_migrate_incoming

Message ID 20201027182144.3315885-25-pbonzini@redhat.com
State Accepted
Commit e69d50d621ccc1ab8b1048a3075ad944afebfed5
Headers show
Series cleanup qemu_init and make sense of command line processing | expand

Commit Message

Paolo Bonzini Oct. 27, 2020, 6:21 p.m. UTC
Make qemu_start_incoming_migration local to migration/migration.c.
By using the runstate instead of a separate flag, vl need not do
anything to setup deferred incoming migration.

qmp_migrate_incoming also does not need the deferred_incoming flag
anymore, because "-incoming PROTOCOL" will clear the "once" flag
before the main loop starts.  Therefore, later invocations of
the migrate-incoming command will fail with the existing
"The incoming migration has already been started" error message.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/migration/misc.h |  1 -
 migration/migration.c    | 33 ++++++++-------------------------
 softmmu/vl.c             | 11 +++++++----
 3 files changed, 15 insertions(+), 30 deletions(-)

Comments

Igor Mammedov Nov. 20, 2020, 3:34 p.m. UTC | #1
On Tue, 27 Oct 2020 14:21:39 -0400
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Make qemu_start_incoming_migration local to migration/migration.c.

> By using the runstate instead of a separate flag, vl need not do

> anything to setup deferred incoming migration.

> 

> qmp_migrate_incoming also does not need the deferred_incoming flag

> anymore, because "-incoming PROTOCOL" will clear the "once" flag

> before the main loop starts.  Therefore, later invocations of

> the migrate-incoming command will fail with the existing

> "The incoming migration has already been started" error message.

> 

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

> ---

>  include/migration/misc.h |  1 -

>  migration/migration.c    | 33 ++++++++-------------------------

>  softmmu/vl.c             | 11 +++++++----

>  3 files changed, 15 insertions(+), 30 deletions(-)

> 

> diff --git a/include/migration/misc.h b/include/migration/misc.h

> index 34e7d75713..bccc1b6b44 100644

> --- a/include/migration/misc.h

> +++ b/include/migration/misc.h

> @@ -58,7 +58,6 @@ void dump_vmstate_json_to_file(FILE *out_fp);

>  /* migration/migration.c */

>  void migration_object_init(void);

>  void migration_shutdown(void);

> -void qemu_start_incoming_migration(const char *uri, Error **errp);

>  bool migration_is_idle(void);

>  bool migration_is_active(MigrationState *);

>  void add_migration_state_change_notifier(Notifier *notify);

> diff --git a/migration/migration.c b/migration/migration.c

> index f48b03cac2..d078094c56 100644

> --- a/migration/migration.c

> +++ b/migration/migration.c

> @@ -114,8 +114,6 @@

>  static NotifierList migration_state_notifiers =

>      NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);

>  

> -static bool deferred_incoming;

> -

>  /* Messages sent on the return path from destination to source */

>  enum mig_rp_message_type {

>      MIG_RP_MSG_INVALID = 0,  /* Must be 0 */

> @@ -257,19 +255,6 @@ static bool migrate_late_block_activate(void)

>          MIGRATION_CAPABILITY_LATE_BLOCK_ACTIVATE];

>  }

>  

> -/*

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

> -}

> -

>  /*

>   * Send a message on the return channel back to the source

>   * of the migration.

> @@ -380,16 +365,14 @@ void migrate_add_address(SocketAddress *address)

>      addrs->value = QAPI_CLONE(SocketAddress, address);

>  }

>  

> -void qemu_start_incoming_migration(const char *uri, Error **errp)

> +static void qemu_start_incoming_migration(const char *uri, Error **errp)

>  {

>      const char *p = NULL;

>  

>      qapi_event_send_migration(MIGRATION_STATUS_SETUP);

> -    if (!strcmp(uri, "defer")) {

> -        deferred_incoming_migration(errp);

> -    } else if (strstart(uri, "tcp:", &p) ||

> -               strstart(uri, "unix:", NULL) ||

> -               strstart(uri, "vsock:", NULL)) {

considering the last hunk does won't call qmp_migrate_incoming
if 'defer' was used, wouldn't we will lose QAPI event here?
not sure how important it to users,
Ccing David

> +    if (strstart(uri, "tcp:", &p) ||

> +        strstart(uri, "unix:", NULL) ||

> +        strstart(uri, "vsock:", NULL)) {

>          socket_start_incoming_migration(p ? p : uri, errp);

>  #ifdef CONFIG_RDMA

>      } else if (strstart(uri, "rdma:", &p)) {

> @@ -1926,14 +1909,14 @@ void qmp_migrate_incoming(const char *uri, Error **errp)

>      Error *local_err = NULL;

>      static bool once = true;

>  

> -    if (!deferred_incoming) {

> -        error_setg(errp, "For use with '-incoming defer'");

> -        return;

> -    }

>      if (!once) {

>          error_setg(errp, "The incoming migration has already been started");

>          return;

>      }

> +    if (!runstate_check(RUN_STATE_INMIGRATE)) {

> +        error_setg(errp, "'-incoming' was not specified on the command line");

> +        return;

> +    }

>  

>      qemu_start_incoming_migration(uri, &local_err);

>  

> diff --git a/softmmu/vl.c b/softmmu/vl.c

> index ae2854d8af..583366510b 100644

> --- a/softmmu/vl.c

> +++ b/softmmu/vl.c

> @@ -109,6 +109,7 @@

>  #include "qapi/qapi-visit-block-core.h"

>  #include "qapi/qapi-visit-ui.h"

>  #include "qapi/qapi-commands-block-core.h"

> +#include "qapi/qapi-commands-migration.h"

>  #include "qapi/qapi-commands-run-state.h"

>  #include "qapi/qapi-commands-ui.h"

>  #include "qapi/qmp/qerror.h"

> @@ -4557,10 +4558,12 @@ void qemu_init(int argc, char **argv, char **envp)

>      }

>      if (incoming) {

>          Error *local_err = NULL;

> -        qemu_start_incoming_migration(incoming, &local_err);

> -        if (local_err) {

> -            error_reportf_err(local_err, "-incoming %s: ", incoming);

> -            exit(1);

> +        if (strcmp(incoming, "defer") != 0) {

> +            qmp_migrate_incoming(incoming, &local_err);

> +            if (local_err) {

> +                error_reportf_err(local_err, "-incoming %s: ", incoming);

> +                exit(1);

> +            }

>          }

>      } else if (autostart) {

>          vm_start();
Paolo Bonzini Nov. 20, 2020, 4:02 p.m. UTC | #2
On 20/11/20 16:34, Igor Mammedov wrote:
>>       qapi_event_send_migration(MIGRATION_STATUS_SETUP);

>> -    if (!strcmp(uri, "defer")) {

>> -        deferred_incoming_migration(errp);

>> -    } else if (strstart(uri, "tcp:", &p) ||

>> -               strstart(uri, "unix:", NULL) ||

>> -               strstart(uri, "vsock:", NULL)) {

> considering the last hunk does won't call qmp_migrate_incoming

> if 'defer' was used, wouldn't we will lose QAPI event here?

> not sure how important it to users,


Hmm yeah that's true.  That might even be considered a bugfix (no setup 
is done until the "real" migrate-incoming command), but I can also add 
the event manually in qemu_init.

(Libvirt doesn't use the SETUP case of the event but that's of course 
only part of the story).

Paolo
Dr. David Alan Gilbert Dec. 2, 2020, 1:10 p.m. UTC | #3
* Paolo Bonzini (pbonzini@redhat.com) wrote:
> On 20/11/20 16:34, Igor Mammedov wrote:

> > >       qapi_event_send_migration(MIGRATION_STATUS_SETUP);

> > > -    if (!strcmp(uri, "defer")) {

> > > -        deferred_incoming_migration(errp);

> > > -    } else if (strstart(uri, "tcp:", &p) ||

> > > -               strstart(uri, "unix:", NULL) ||

> > > -               strstart(uri, "vsock:", NULL)) {

> > considering the last hunk does won't call qmp_migrate_incoming

> > if 'defer' was used, wouldn't we will lose QAPI event here?

> > not sure how important it to users,

> 

> Hmm yeah that's true.  That might even be considered a bugfix (no setup is

> done until the "real" migrate-incoming command), but I can also add the

> event manually in qemu_init.

> 

> (Libvirt doesn't use the SETUP case of the event but that's of course only

> part of the story).


I'm more worried about how this stops a repeated 'migrate incoming'
or a 'migrate_incoming' that's issued following a qemu that's been
started with -incoming tcp:... but which a socket hasn't yet connected
to.

Dave

> Paolo

> 

-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Daniel P. Berrangé Dec. 2, 2020, 1:15 p.m. UTC | #4
On Wed, Dec 02, 2020 at 01:10:37PM +0000, Dr. David Alan Gilbert wrote:
> * Paolo Bonzini (pbonzini@redhat.com) wrote:

> > On 20/11/20 16:34, Igor Mammedov wrote:

> > > >       qapi_event_send_migration(MIGRATION_STATUS_SETUP);

> > > > -    if (!strcmp(uri, "defer")) {

> > > > -        deferred_incoming_migration(errp);

> > > > -    } else if (strstart(uri, "tcp:", &p) ||

> > > > -               strstart(uri, "unix:", NULL) ||

> > > > -               strstart(uri, "vsock:", NULL)) {

> > > considering the last hunk does won't call qmp_migrate_incoming

> > > if 'defer' was used, wouldn't we will lose QAPI event here?

> > > not sure how important it to users,

> > 

> > Hmm yeah that's true.  That might even be considered a bugfix (no setup is

> > done until the "real" migrate-incoming command), but I can also add the

> > event manually in qemu_init.

> > 

> > (Libvirt doesn't use the SETUP case of the event but that's of course only

> > part of the story).

> 

> I'm more worried about how this stops a repeated 'migrate incoming'

> or a 'migrate_incoming' that's issued following a qemu that's been

> started with -incoming tcp:... but which a socket hasn't yet connected

> to.


Can someone remind me why we need to have an -incoming arg at all ?

With snapshots, we can just start QEMU normally, using -S if desired,
and then invoke "loadvm" to restore from a snapshot at any time.

What is different thet means we can't just run "migrate_incoming" on
any QEMU at any time, ignoring -incoming entirely ?


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Paolo Bonzini Dec. 2, 2020, 1:29 p.m. UTC | #5
On 02/12/20 14:15, Daniel P. Berrangé wrote:
> Can someone remind me why we need to have an -incoming arg at all ?

> 

> With snapshots, we can just start QEMU normally, using -S if desired,

> and then invoke "loadvm" to restore from a snapshot at any time.

> 

> What is different thet means we can't just run "migrate_incoming" on

> any QEMU at any time, ignoring -incoming entirely ?


There are some parts of QEMU that operate differently based on 
RUN_STATE_INCOMING.  Removing those is one of the things that these 
patches should enable, though there are also some uses in Xen that I'm 
more worried about.

Paolo
Paolo Bonzini Dec. 2, 2020, 1:36 p.m. UTC | #6
On 02/12/20 14:10, Dr. David Alan Gilbert wrote:
> I'm more worried about how this stops a repeated 'migrate incoming'

> or a 'migrate_incoming' that's issued following a qemu that's been

> started with -incoming tcp:... but which a socket hasn't yet connected

> to.


Good question, fortunately it is simply handled answer:

void qmp_migrate_incoming(const char *uri, Error **errp)
{
     Error *local_err = NULL;
     static bool once = true;

     if (!once) {
         error_setg(errp, "The incoming migration has already been 
started");
         return;
     }
     if (!runstate_check(RUN_STATE_INMIGRATE)) {
         error_setg(errp, "'-incoming' was not specified on the command 
line");
         return;
     }

     qemu_start_incoming_migration(uri, &local_err);

     if (local_err) {
         error_propagate(errp, local_err);
         return;
     }

     once = false;
}

This patch can simplify things because every incoming migrations (no 
matter if '-incoming defer' or '-incoming tcp:...') goes through the 
qmp_migrate_incoming function above.

Paolo
Dr. David Alan Gilbert Dec. 2, 2020, 3:08 p.m. UTC | #7
* Paolo Bonzini (pbonzini@redhat.com) wrote:
> On 02/12/20 14:10, Dr. David Alan Gilbert wrote:

> > I'm more worried about how this stops a repeated 'migrate incoming'

> > or a 'migrate_incoming' that's issued following a qemu that's been

> > started with -incoming tcp:... but which a socket hasn't yet connected

> > to.

> 

> Good question, fortunately it is simply handled answer:

> 

> void qmp_migrate_incoming(const char *uri, Error **errp)

> {

>     Error *local_err = NULL;

>     static bool once = true;

> 

>     if (!once) {

>         error_setg(errp, "The incoming migration has already been started");

>         return;

>     }

>     if (!runstate_check(RUN_STATE_INMIGRATE)) {

>         error_setg(errp, "'-incoming' was not specified on the command

> line");

>         return;

>     }

> 

>     qemu_start_incoming_migration(uri, &local_err);

> 

>     if (local_err) {

>         error_propagate(errp, local_err);

>         return;

>     }

> 

>     once = false;

> }

> 

> This patch can simplify things because every incoming migrations (no matter

> if '-incoming defer' or '-incoming tcp:...') goes through the

> qmp_migrate_incoming function above.


Yeh I think that's OK.

Dave

> Paolo

> 

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

Patch

diff --git a/include/migration/misc.h b/include/migration/misc.h
index 34e7d75713..bccc1b6b44 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -58,7 +58,6 @@  void dump_vmstate_json_to_file(FILE *out_fp);
 /* migration/migration.c */
 void migration_object_init(void);
 void migration_shutdown(void);
-void qemu_start_incoming_migration(const char *uri, Error **errp);
 bool migration_is_idle(void);
 bool migration_is_active(MigrationState *);
 void add_migration_state_change_notifier(Notifier *notify);
diff --git a/migration/migration.c b/migration/migration.c
index f48b03cac2..d078094c56 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -114,8 +114,6 @@ 
 static NotifierList migration_state_notifiers =
     NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
 
-static bool deferred_incoming;
-
 /* Messages sent on the return path from destination to source */
 enum mig_rp_message_type {
     MIG_RP_MSG_INVALID = 0,  /* Must be 0 */
@@ -257,19 +255,6 @@  static bool migrate_late_block_activate(void)
         MIGRATION_CAPABILITY_LATE_BLOCK_ACTIVATE];
 }
 
-/*
- * 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;
-}
-
 /*
  * Send a message on the return channel back to the source
  * of the migration.
@@ -380,16 +365,14 @@  void migrate_add_address(SocketAddress *address)
     addrs->value = QAPI_CLONE(SocketAddress, address);
 }
 
-void qemu_start_incoming_migration(const char *uri, Error **errp)
+static void qemu_start_incoming_migration(const char *uri, Error **errp)
 {
     const char *p = NULL;
 
     qapi_event_send_migration(MIGRATION_STATUS_SETUP);
-    if (!strcmp(uri, "defer")) {
-        deferred_incoming_migration(errp);
-    } else if (strstart(uri, "tcp:", &p) ||
-               strstart(uri, "unix:", NULL) ||
-               strstart(uri, "vsock:", NULL)) {
+    if (strstart(uri, "tcp:", &p) ||
+        strstart(uri, "unix:", NULL) ||
+        strstart(uri, "vsock:", NULL)) {
         socket_start_incoming_migration(p ? p : uri, errp);
 #ifdef CONFIG_RDMA
     } else if (strstart(uri, "rdma:", &p)) {
@@ -1926,14 +1909,14 @@  void qmp_migrate_incoming(const char *uri, Error **errp)
     Error *local_err = NULL;
     static bool once = true;
 
-    if (!deferred_incoming) {
-        error_setg(errp, "For use with '-incoming defer'");
-        return;
-    }
     if (!once) {
         error_setg(errp, "The incoming migration has already been started");
         return;
     }
+    if (!runstate_check(RUN_STATE_INMIGRATE)) {
+        error_setg(errp, "'-incoming' was not specified on the command line");
+        return;
+    }
 
     qemu_start_incoming_migration(uri, &local_err);
 
diff --git a/softmmu/vl.c b/softmmu/vl.c
index ae2854d8af..583366510b 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -109,6 +109,7 @@ 
 #include "qapi/qapi-visit-block-core.h"
 #include "qapi/qapi-visit-ui.h"
 #include "qapi/qapi-commands-block-core.h"
+#include "qapi/qapi-commands-migration.h"
 #include "qapi/qapi-commands-run-state.h"
 #include "qapi/qapi-commands-ui.h"
 #include "qapi/qmp/qerror.h"
@@ -4557,10 +4558,12 @@  void qemu_init(int argc, char **argv, char **envp)
     }
     if (incoming) {
         Error *local_err = NULL;
-        qemu_start_incoming_migration(incoming, &local_err);
-        if (local_err) {
-            error_reportf_err(local_err, "-incoming %s: ", incoming);
-            exit(1);
+        if (strcmp(incoming, "defer") != 0) {
+            qmp_migrate_incoming(incoming, &local_err);
+            if (local_err) {
+                error_reportf_err(local_err, "-incoming %s: ", incoming);
+                exit(1);
+            }
         }
     } else if (autostart) {
         vm_start();