Re: [RFC PATCH 04/13] migration: Fix parameter validation

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Thu, May 22, 2025 at 02:39:48PM -0300, Fabiano Rosas wrote:
>> Actually, this doesn't work...
>> 
>> The migrate-set-* commands have optional fields, so we need some form of
>> checking has_* to know which fields the user is setting. Otherwise
>> MigrationSetParameters will have zeros all over that will trip the
>> check.
>> 
>> Then, we need some form of checking has_* to be able to enventually get
>> the values into s->config (former s->parameters/capabilities), otherwise
>> we'll overwrite the already-set values with the potentially empty ones
>> coming from QAPI.
>> 
>> Then there's also the issue of knowing whether a value is 0 because the
>> user set it 0 or because it was never set.
>> 
>> We also can't apply an invalid value to s->config and validate it after
>> because some parameters are allowed to be changed during migration.
>
> What I meant was we only conditionally ignore the has_* fields in below:
>
>   (1) migrate_params_check(), so that QEMU always checks all parameters in
>       the MigrationParameters* specified when invoking the function.
>

Yes, I realised later that's what you meant. I'm looking into
it. Hitting some issues with the block_bitmap_mapping option, which
seems to be able to become NULL even if has_block_bitmap_mapping is
true. According to commit 3cba22c9ad ("migration: Fix
block_bitmap_mapping migration").

>   (2) MigrationState.parameters, so that as long as the parameters are
>       applied (it should only happen after sanity check all pass..) then we
>       ignore these has_* fields (until MigrationState.parameters can have a
>       better struct to not include these has_* fields).
>
> We can keep the has_* checks in migrate_params_test_apply() and
> migrate_params_apply(), so that we won't touch the ones the user didn't
> specify in the QMP commands as you said.
>
> The benefits of having above 1/2 ignoring has_* is some code removal where
> we assume has_* always are TRUEs.
>
> This can be still a bit confusing, but at least we don't need to init has_*
> fields in migrate_params_init() anymore as they'll be all ignored, then
> there's no chance we forget set TRUEs to any new params either.