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

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Apr 11, 2025 at 04:14:34PM -0300, Fabiano Rosas wrote:
>> The migration parameters validation involves producing a temporary
>> structure which merges the current parameter values with the new
>> parameters set by the user.
>> 
>> The has_ boolean fields of MigrateSetParameter are taken into
>> consideration when writing the temporary structure, however the copy
>> of the current parameters also copies all the has_ fields of
>> s->parameters and those are (almost) all true due to being initialized
>> by migrate_params_init().
>> 
>> Since the temporary structure copy does not carry over the has_ fields
>> from MigrateSetParameters, only the values which were initialized in
>> migrate_params_init() will end up being validated. This causes
>> (almost) all of the migration parameters to be validated again every
>> time a parameter is set, which could be considered a bug. But it also
>> skips validation of those values which are not set in
>> migrate_params_init(), which is a worse issue.
>
> IMHO it's ok to double check all parameters in slow path.  Definitely not
> ok to skip them.. So now the question is, if migrate_params_test_apply() so
> far should check all params anyway...
>

Well, either way is fine by me. In the current code, I can't tell what
the intention was, unfortunately.

We could check all params, but then we need to make sure they never
change in between calls to migrate-set-params. Looking at the code I
don't see any place where we allow s->parameters to change.

But then I worry about checks that:
- might be too costly
- only make sense the first time
- depend on the order of setting (a param/cap that should only be set
before/after some other param/cap)

> Shall we drop the checking for all has_ there, then IIUC we also don't need
> any initializations for has_* in migrate_params_init() here?
>

It'd be nice to not have to touch the has_ fields, yes. I'll experiment
with it.

> So, admittedly s->parameters.has_* is still ugly to be present.. we declare
> all of them not used and ignore them always at least in s->parameters.
>
>> 
>> Fix by initializing the missing values in migrate_params_init().
>> Currently 'avail_switchover_bandwidth' and 'block_bitmap_mapping' are
>> affected.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/options.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/migration/options.c b/migration/options.c
>> index cac28540dd..625d597a85 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -987,6 +987,8 @@ void migrate_params_init(MigrationParameters *params)
>>      params->has_mode = true;
>>      params->has_zero_page_detection = true;
>>      params->has_direct_io = true;
>> +    params->has_avail_switchover_bandwidth = true;
>> +    params->has_block_bitmap_mapping = true;
>>  }
>>  
>>  /*
>> -- 
>> 2.35.3
>>