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

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Jun 28, 2023 at 01:55:37PM -0300, Fabiano Rosas 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. Add the
>> force-emit-setup-event migration property to enable it.
>
> This is so unfortunate... since qemu 2.4.....
>
> Does it mean that when cap-events is set we can send duplicated events?
>

Not with current code because this event was the only one sent directly
without setting the state first, so migrate_generate_event() never runs.

And not with this patch because I'm not sending the event if
migrate_events() is true because it will already be sent by
migrate_generate_event().

> The fix makes sense to me in general, butt I'm curious whether we can fix
> it without having a compat bit doing the wrong thing, even if having the
> risk of breaking someone, with the hope that the only thing he/she needs to
> do is to enable the cap-events if didn't.  I'd consider that if e.g. as
> long as libvirt is fine.  Does anyone know how libvirt handles this?
>

I agree that it would be cleaner for us to just break compatibility and
hope for the best. Any process waiting for the event would hang, but
simply enabling the capability would fix it.

I see libvirt knows about the 'events' capability but I couldn't
determine if it is enabled by default. I'll have to take a deeper look.