Re: [PATCH 1/6] migration: Set migration status early in incoming side

[Juan Quintela]

Fabiano Rosas <farosas@suse.de> wrote:
> We are sending a migration event of MIGRATION_STATUS_SETUP at
> qemu_start_incoming_migration but never actually setting the state.
>
> This creates a window between qmp_migrate_incoming and
> process_incoming_migration_co where the migration status is still
> MIGRATION_STATUS_NONE. Calling query-migrate during this time will
> return an empty response even though the incoming migration command
> has already been issued.
>
> Commit 7cf1fe6d68 ("migration: Add migration events on target side")
> has added support to the 'events' capability to the incoming part of
> migration, but chose to send the SETUP event without setting the
> state. I'm assuming this was a mistake.
>
> To avoid introducing a change in behavior, we need to keep sending the
> SETUP event, even if the 'events' capability is not set.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/migration.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 7c8292d4d4..562b78261d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -424,13 +424,26 @@ void migrate_add_address(SocketAddress *address)
>  static void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
>      const char *p = NULL;
> +    MigrationIncomingState *mis = migration_incoming_get_current();
>  
>      /* URI is not suitable for migration? */
>      if (!migration_channels_and_uri_compatible(uri, errp)) {
>          return;
>      }
>  
> -    qapi_event_send_migration(MIGRATION_STATUS_SETUP);
> +    migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
> +                      MIGRATION_STATUS_SETUP);
> +    /*
> +     * QMP clients should have set the 'events' migration capability
> +     * if they want to receive this event, in which case the
> +     * migrate_set_state() call above will have already sent the
> +     * event. We still need to send the event for compatibility even
> +     * if migration events are disabled.
> +     */
> +    if (!migrate_events()) {
> +        qapi_event_send_migration(MIGRATION_STATUS_SETUP);
> +    }

Can we add and test for a property here, so we can drop this at some
point in the future?

> +
>      if (strstart(uri, "tcp:", &p) ||
>          strstart(uri, "unix:", NULL) ||
>          strstart(uri, "vsock:", NULL)) {
> @@ -524,7 +537,7 @@ process_incoming_migration_co(void *opaque)
>  
>      mis->largest_page_size = qemu_ram_pagesize_largest();
>      postcopy_state_set(POSTCOPY_INCOMING_NONE);
> -    migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
> +    migrate_set_state(&mis->state, MIGRATION_STATUS_SETUP,
>                        MIGRATION_STATUS_ACTIVE);
>  
>      mis->loadvm_co = qemu_coroutine_self();