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

[Fabiano Rosas <farosas@suse.de>]

Juan Quintela <quintela@redhat.com> writes:

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

Yes, I'll add for v2.

Thanks