Peter Xu <pet...@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 <faro...@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 >>
