Re: [PATCH 2/5] migration: Use QAPI_CLONE_MEMBERS in migrate_params_apply

[Fabiano Rosas <farosas@suse.de>]

Prasad Pandit <ppandit@redhat.com> writes:

> On Wed, 21 Jan 2026 at 18:12, Fabiano Rosas <farosas@suse.de> wrote:
>> In general, I think that whenever we determine what protects a data
>> structure from concurrent access we should make it obvious.
>>
>> So this assert is here to provide _some_ assurance that this routine is protected.
>
> * Yes, but it is not obvious that we are checking bql_locked() to
> protect from concurrent access to the global MigrationState object.
> Secondly for concurrent access, other threads should be running.
> ===
> Thread 1 "qemu-system-x86" hit Breakpoint 1, migrate_params_apply
> (params=0x7ffcb6627840) at ../migration/options.c:1392
> 1392    {
> (gdb) n
> 1393        MigrationState *s = migrate_get_current();
> (gdb) p bql_locked()
> $3 = true
> (gdb) p migration_is_running()
> $4 = false
> (gdb)
>
> Thread 1 "qemu-system-x86" hit Breakpoint 1, migrate_params_apply
> (params=0x7ffcb6627840) at ../migration/options.c:1392
> 1392    {
> (gdb) n
> 1393        MigrationState *s = migrate_get_current();
> (gdb) p bql_locked()
> $5 = true
> (gdb) p migration_is_running()
> $6 = false
> (gdb)
> ===
> * migrate_params_apply() is called twice:
>     1. At the beginning when libvirtd(8) initiates migration
>     2. To reset migration options if we kill migration on the
> destination side by killing VM or connection.

This is one usage. People are free to invoke QMP commands whenever they want.

> Both times it is called from the main Thread-1, BQL is locked and
> other migration threads are not running.
>
> * If we are trying to futureproof this function so that no one should
> be able to call migrate_params_apply() from other threads without bql
> - that seems like a long shot. Because it is reachable only from
> qmp_migrate_set_parameters(), which is invoked by an external QMP
> user. Ie. someone would have to explicitly send
> qmp_migrate_set_parameters() command while migration is running, but
> even then it'll still be invoked by the main thread-1, with
> bql_locked=true. no?
>

Setting parameters while running is common, our tests do it all the time
to adjust convergence options.

Where the command is invoked from is a bad assumption to make. There's
coroutines, iocontexts, oob, etc. These things change all the time and
we don't see it. Look at all the work done in the block layer to assert 

In this case, it will be invoked from the main loop and with BQL taken,
yes. That's not code under the migration subsystem's control, we should
not rely on that never changing. If we can check the requirements at the
entry point into the subsystem, we'll be better protected against
changes in other parts of QEMU that we didn't oversee.

Peter is now going through the painful exercise of removing the incoming
coroutine and playing the whack-a-mole of "BQL or no BQL?". Locks should
be fine-grained, protect specific bits of data and be heavily
documented. Analysing where the function is currently invoked from and
inferring about thread-safety is a pitfall.

(note that a simple /*called with BQL taken*/ would already count)

> * I'm thinking maybe we can skip assert(bql_locked()) and related
> header inclusion.
>

Fine, on the basis that it's out of scope for the patch anyway.

> Thank you.
> ---
>   - Prasad