Hi, just a few more patches until options.c is fully using QAPI_CLONE
instead of open-coding options handling, we're almost there.

In this v2:

- [NEW] patch 1/9 - Cleanup of a previous mistake.

- Properly freeing ptr members in patch 2/9 as spotted by Peter.

- Moved has_block_bitmap_mapping=true from patch 2 to patch 3.

- [NEW] patch 7/9 - Prasad's suggestion to keep the entire process of
  applying parameters in one place.

- [NEW] patch 8/9 - Replaced calls to qapi_free_* with
  qapi_dealloc_visitor.

- [NEW] patch 9/9 - Used the new migrate_params_free() for the
  instance finalize as well.

CI run: https://gitlab.com/farosas/qemu/-/pipelines/2301775498

v1:
https://lore.kernel.org/r/20260114132309.5832-1-farosas@suse.de

I split this series from the original[0] because these changes are
more QAPI-related and self-contained.

The idea is to use QAPI_CLONE_MEMBERS and (the new) QAPI_MERGE to
reduce the amount of open-coded parameters handling in options.c, such
as checks to QAPI's has_* fields and manual inclusion of future
parameters.

- Patches 1 & 2 are unchanged

- Patch 3 adds a new variant of the dealloc visitor to free the to-be
  unreferenced memory when the input visitor eventually allocates new
  memory and overwrites the pointers.

- Patches 4 & 5 are mostly unchanged, but now use the new visitor.

0 - migration: Unify capabilities and parameters
https://lore.kernel.org/r/20251215220041.12657-1-farosas@suse.de

Fabiano Rosas (9):
  migration/options.c: Don't export migrate_tls_opts_free
  migration: Use QAPI_CLONE_MEMBERS in migrate_params_test_apply
  migration: Use QAPI_CLONE_MEMBERS in migrate_params_apply
  qapi: Implement qapi_dealloc_present_visitor
  qapi: Add QAPI_MERGE
  migration/options: Use QAPI_MERGE in migrate_params_test_apply
  migration/options: Open code migrate_params_apply
  migration/options: Stop freeing s->parameters members individually
  migration: Use migrate_params_free during finalize

 include/qapi/dealloc-visitor.h |   6 +
 include/qapi/type-helpers.h    |  40 +++++
 migration/migration.c          |   3 +-
 migration/options.c            | 304 ++++-----------------------------
 migration/options.h            |   2 +-
 qapi/qapi-dealloc-visitor.c    | 173 ++++++++++++++++++-
 6 files changed, 257 insertions(+), 271 deletions(-)

-- 
2.51.0

On Mon, Feb 02, 2026 at 07:40:52PM -0300, Fabiano Rosas wrote:
> Hi, just a few more patches until options.c is fully using QAPI_CLONE
> instead of open-coding options handling, we're almost there.

While I'm reading your series, I found one thing that migration might be
racy against for a while, and this series should enlarge that window of
this issue: I don't think it's safe to free elements in MigrationParameters
in the main thread, even if we hold BQL. Because migration thread can
access parameters anytime without BQL... logically it can read any
parameter, then if it's not a scalar we need some luck not crashing.

IMHO we may need to move faster on the "whitelist that can be dynamically
set during migration" plan on migration parameters and capabilities.

I don't think any cap (after converted to parameters) should be on this
whitelist... for parameters, I'm trying to be conservative but I believe
the list should look like this:

CPU throttle knobs:

        throttle-trigger-threshold
        cpu-throttle-increment
        cpu-throttle-tailslow
        max-cpu-throttle
        x-vcpu-dirty-limit-period
        vcpu-dirty-limit

Bandwith knobs:

        max-bandwidth
        avail-switchover-bandwidth
        downtime-limit
        max-postcopy-bandwidth

COLO knob:

        x-checkpoint-delay

That really should be all parameters we allow to change..  Luckily all of
them are scalars, so it's fine to keep the current migrate_params_free()
and logic to "free then update".

I wonder if we should even consider having this whitelist alongside with
your this series if we want to be extra safe, but I'll let you decide.. I'm
also OK we do it later, but maybe we don't want it to be too late either.

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Mon, Feb 02, 2026 at 07:40:52PM -0300, Fabiano Rosas wrote:
>> Hi, just a few more patches until options.c is fully using QAPI_CLONE
>> instead of open-coding options handling, we're almost there.
>
> While I'm reading your series, I found one thing that migration might be
> racy against for a while, and this series should enlarge that window of
> this issue: I don't think it's safe to free elements in MigrationParameters
> in the main thread, even if we hold BQL. Because migration thread can
> access parameters anytime without BQL... logically it can read any
> parameter, then if it's not a scalar we need some luck not crashing.
>

Good point, I had thought of it and convinced myself it was ok
somehow. I'll take another look.

> IMHO we may need to move faster on the "whitelist that can be dynamically
> set during migration" plan on migration parameters and capabilities.
>

Or we make set-parameters always work on top of temporary
MigrationParameter objects and switch the pointer atomically. Might be a
good idea regardless.

> I don't think any cap (after converted to parameters) should be on this
> whitelist... for parameters, I'm trying to be conservative but I believe
> the list should look like this:
>
> CPU throttle knobs:
>
>         throttle-trigger-threshold
>         cpu-throttle-increment
>         cpu-throttle-tailslow
>         max-cpu-throttle
>         x-vcpu-dirty-limit-period
>         vcpu-dirty-limit
>
> Bandwith knobs:
>
>         max-bandwidth
>         avail-switchover-bandwidth
>         downtime-limit
>         max-postcopy-bandwidth
>
> COLO knob:
>
>         x-checkpoint-delay
>
> That really should be all parameters we allow to change..  Luckily all of
> them are scalars, so it's fine to keep the current migrate_params_free()
> and logic to "free then update".
>
> I wonder if we should even consider having this whitelist alongside with
> your this series if we want to be extra safe, but I'll let you decide.. I'm
> also OK we do it later, but maybe we don't want it to be too late either.

Yeah.. I'll see what I can do. Thanks for the reviews this far!

On Fri, Feb 06, 2026 at 09:34:12AM -0300, Fabiano Rosas wrote:
> Or we make set-parameters always work on top of temporary
> MigrationParameter objects and switch the pointer atomically. Might be a
> good idea regardless.

Currently I recall you mentioned the parameters struct can't be an pointer,
so maybe there's some challenge there on making it atomic.  But yeah, I
know you'll figure things out! :-D

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Fri, Feb 06, 2026 at 09:34:12AM -0300, Fabiano Rosas wrote:
>> Or we make set-parameters always work on top of temporary
>> MigrationParameter objects and switch the pointer atomically. Might be a
>> good idea regardless.
>
> Currently I recall you mentioned the parameters struct can't be an pointer,
> so maybe there's some challenge there on making it atomic.  But yeah, I
> know you'll figure things out! :-D

So _that_ is why I haven't done it yet! Sometimes I doubt myself too
much =D