| Message ID | 20230524080157.530968-1-wei.w.wang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] migration: fix migrate_params_test_apply to set the dest param correctly | expand |
On Wed, May 24, 2023 at 04:01:57PM +0800, Wei Wang wrote: > qmp_migrate_set_parameters expects to use tmp for parameters check, > so migrate_params_test_apply is expected to copy the related fields from > params to tmp. So fix migrate_params_test_apply to use the function > parameter, *dest, rather than the global one. The dest->has_xxx (xxx is > the feature name) related fields need to be set, as they will be checked > by migrate_params_check. I think it's fine to do as what you suggested, but I don't see much benefit either.. the old code IIUC will check all params even if 1 param changed, while after your change it only checks the modified ones. There's slight benefits but not so much, especially "22+, 2-" LOCs, because we don't really do this a lot; some more sanity check also makes sense to me even if everything is always checked, so we'll hit errors if anything accidentally goes wrong too. Is there a real bug somewhere? > > Signed-off-by: Wei Wang <wei.w.wang@intel.com> > --- > migration/options.c | 24 ++++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > > diff --git a/migration/options.c b/migration/options.c > index b62ab30cd5..a560483871 100644 > --- a/migration/options.c > +++ b/migration/options.c > @@ -1089,39 +1089,45 @@ bool migrate_params_check(MigrationParameters *params, Error **errp) > static void migrate_params_test_apply(MigrateSetParameters *params, > MigrationParameters *dest) > { > - *dest = migrate_get_current()->parameters; > - > /* TODO use QAPI_CLONE() instead of duplicating it inline */ > > if (params->has_compress_level) { > + dest->has_compress_level = true; > dest->compress_level = params->compress_level; > } > > if (params->has_compress_threads) { > + dest->has_compress_threads = true; > dest->compress_threads = params->compress_threads; > } > > if (params->has_compress_wait_thread) { > + dest->has_compress_wait_thread = true; > dest->compress_wait_thread = params->compress_wait_thread; > } > > if (params->has_decompress_threads) { > + dest->has_decompress_threads = true; > dest->decompress_threads = params->decompress_threads; > } > > if (params->has_throttle_trigger_threshold) { > + dest->has_throttle_trigger_threshold = true; > dest->throttle_trigger_threshold = params->throttle_trigger_threshold; > } > > if (params->has_cpu_throttle_initial) { > + dest->has_cpu_throttle_initial = true; > dest->cpu_throttle_initial = params->cpu_throttle_initial; > } > > if (params->has_cpu_throttle_increment) { > + dest->has_cpu_throttle_increment = true; > dest->cpu_throttle_increment = params->cpu_throttle_increment; > } > > if (params->has_cpu_throttle_tailslow) { > + dest->has_cpu_throttle_tailslow = true; > dest->cpu_throttle_tailslow = params->cpu_throttle_tailslow; > } > > @@ -1136,45 +1142,58 @@ static void migrate_params_test_apply(MigrateSetParameters *params, > } > > if (params->has_max_bandwidth) { > + dest->has_max_bandwidth = true; > dest->max_bandwidth = params->max_bandwidth; > } > > if (params->has_downtime_limit) { > + dest->has_downtime_limit = true; > dest->downtime_limit = params->downtime_limit; > } > > if (params->has_x_checkpoint_delay) { > + dest->has_x_checkpoint_delay = true; > dest->x_checkpoint_delay = params->x_checkpoint_delay; > } > > if (params->has_block_incremental) { > + dest->has_block_incremental = true; > dest->block_incremental = params->block_incremental; > } > if (params->has_multifd_channels) { > + dest->has_multifd_channels = true; > dest->multifd_channels = params->multifd_channels; > } > if (params->has_multifd_compression) { > + dest->has_multifd_compression = true; > dest->multifd_compression = params->multifd_compression; > } > if (params->has_xbzrle_cache_size) { > + dest->has_xbzrle_cache_size = true; > dest->xbzrle_cache_size = params->xbzrle_cache_size; > } > if (params->has_max_postcopy_bandwidth) { > + dest->has_max_postcopy_bandwidth = true; > dest->max_postcopy_bandwidth = params->max_postcopy_bandwidth; > } > if (params->has_max_cpu_throttle) { > + dest->has_max_cpu_throttle = true; > dest->max_cpu_throttle = params->max_cpu_throttle; > } > if (params->has_announce_initial) { > + dest->has_announce_initial = true; > dest->announce_initial = params->announce_initial; > } > if (params->has_announce_max) { > + dest->has_announce_max = true; > dest->announce_max = params->announce_max; > } > if (params->has_announce_rounds) { > + dest->has_announce_rounds = true; > dest->announce_rounds = params->announce_rounds; > } > if (params->has_announce_step) { > + dest->has_announce_step = true; > dest->announce_step = params->announce_step; > } > > @@ -1321,6 +1340,7 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp) > params->tls_hostname->u.s = strdup(""); > } > > + memset(&tmp, 0, sizeof(MigrationParameters)); > migrate_params_test_apply(params, &tmp); > > if (!migrate_params_check(&tmp, errp)) { > -- > 2.27.0 >
On Saturday, May 27, 2023 5:49 AM, Peter Xu wrote: > On Wed, May 24, 2023 at 04:01:57PM +0800, Wei Wang wrote: > > qmp_migrate_set_parameters expects to use tmp for parameters check, so > > migrate_params_test_apply is expected to copy the related fields from > > params to tmp. So fix migrate_params_test_apply to use the function > > parameter, *dest, rather than the global one. The dest->has_xxx (xxx > > is the feature name) related fields need to be set, as they will be > > checked by migrate_params_check. > > I think it's fine to do as what you suggested, but I don't see much benefit > either.. the old code IIUC will check all params even if 1 param changed, > while after your change it only checks the modified ones. > > There's slight benefits but not so much, especially "22+, 2-" LOCs, because > we don't really do this a lot; some more sanity check also makes sense to me > even if everything is always checked, so we'll hit errors if anything > accidentally goes wrong too. > > Is there a real bug somewhere? Yes. Please see qmp_migrate_set_parameters: #1 migrate_params_test_apply(params, &tmp); #2 if (!migrate_params_check(&tmp, errp)) { /* Invalid parameter */ return; } #3 migrate_params_apply(params, errp); #2 tries to do params check using tmp, which is expected to be set up by #1, but #1 didn't use "&tmp", so "tmp" doesn’t seem to store the valid values as expected for the check (that is, #2 above isn’t effectively doing any check for the user input params) The alternative fix would be to remove the intermediate "tmp" params, but this might break the usage from commit 1bda8b3c6950, so need thoughts from Markus if we want go for this approach.
On Mon, May 29, 2023 at 12:55:30PM +0000, Wang, Wei W wrote: > On Saturday, May 27, 2023 5:49 AM, Peter Xu wrote: > > On Wed, May 24, 2023 at 04:01:57PM +0800, Wei Wang wrote: > > > qmp_migrate_set_parameters expects to use tmp for parameters check, so > > > migrate_params_test_apply is expected to copy the related fields from > > > params to tmp. So fix migrate_params_test_apply to use the function > > > parameter, *dest, rather than the global one. The dest->has_xxx (xxx > > > is the feature name) related fields need to be set, as they will be > > > checked by migrate_params_check. > > > > I think it's fine to do as what you suggested, but I don't see much benefit > > either.. the old code IIUC will check all params even if 1 param changed, > > while after your change it only checks the modified ones. > > > > There's slight benefits but not so much, especially "22+, 2-" LOCs, because > > we don't really do this a lot; some more sanity check also makes sense to me > > even if everything is always checked, so we'll hit errors if anything > > accidentally goes wrong too. > > > > Is there a real bug somewhere? > > Yes. Please see qmp_migrate_set_parameters: > > #1 migrate_params_test_apply(params, &tmp); > > #2 if (!migrate_params_check(&tmp, errp)) { > /* Invalid parameter */ > return; > } > #3 migrate_params_apply(params, errp); > > #2 tries to do params check using tmp, which is expected to be set up > by #1, but #1 didn't use "&tmp", #1 initialized "&tmp" with current parameters, here: *dest = migrate_get_current()->parameters; ? > so "tmp" doesn’t seem to store the > valid values as expected for the check (that is, #2 above isn’t effectively > doing any check for the user input params) Do you have a reproducer where qmp set param will not check properly on user input? > > The alternative fix would be to remove the intermediate "tmp" params, > but this might break the usage from commit 1bda8b3c6950, so need thoughts > from Markus if we want go for this approach. Thanks,
On Monday, May 29, 2023 10:58 PM, Peter Xu wrote: > > > > #1 migrate_params_test_apply(params, &tmp); > > > > #2 if (!migrate_params_check(&tmp, errp)) { > > /* Invalid parameter */ > > return; > > } > > #3 migrate_params_apply(params, errp); > > > > #2 tries to do params check using tmp, which is expected to be set up > > by #1, but #1 didn't use "&tmp", > > #1 initialized "&tmp" with current parameters, here: > > *dest = migrate_get_current()->parameters; > > ? Yes. Sorry, I had a misunderstanding of this one. All the has_* of the current params has been initialized to true at the beginning. (I once dumped tmp after migrate_params_test_apply, it were all 0, which drove me to make the changes, but couldn't reproduce it now - things appear to be correct without this patch)
diff --git a/migration/options.c b/migration/options.c index b62ab30cd5..a560483871 100644 --- a/migration/options.c +++ b/migration/options.c @@ -1089,39 +1089,45 @@ bool migrate_params_check(MigrationParameters *params, Error **errp) static void migrate_params_test_apply(MigrateSetParameters *params, MigrationParameters *dest) { - *dest = migrate_get_current()->parameters; - /* TODO use QAPI_CLONE() instead of duplicating it inline */ if (params->has_compress_level) { + dest->has_compress_level = true; dest->compress_level = params->compress_level; } if (params->has_compress_threads) { + dest->has_compress_threads = true; dest->compress_threads = params->compress_threads; } if (params->has_compress_wait_thread) { + dest->has_compress_wait_thread = true; dest->compress_wait_thread = params->compress_wait_thread; } if (params->has_decompress_threads) { + dest->has_decompress_threads = true; dest->decompress_threads = params->decompress_threads; } if (params->has_throttle_trigger_threshold) { + dest->has_throttle_trigger_threshold = true; dest->throttle_trigger_threshold = params->throttle_trigger_threshold; } if (params->has_cpu_throttle_initial) { + dest->has_cpu_throttle_initial = true; dest->cpu_throttle_initial = params->cpu_throttle_initial; } if (params->has_cpu_throttle_increment) { + dest->has_cpu_throttle_increment = true; dest->cpu_throttle_increment = params->cpu_throttle_increment; } if (params->has_cpu_throttle_tailslow) { + dest->has_cpu_throttle_tailslow = true; dest->cpu_throttle_tailslow = params->cpu_throttle_tailslow; } @@ -1136,45 +1142,58 @@ static void migrate_params_test_apply(MigrateSetParameters *params, } if (params->has_max_bandwidth) { + dest->has_max_bandwidth = true; dest->max_bandwidth = params->max_bandwidth; } if (params->has_downtime_limit) { + dest->has_downtime_limit = true; dest->downtime_limit = params->downtime_limit; } if (params->has_x_checkpoint_delay) { + dest->has_x_checkpoint_delay = true; dest->x_checkpoint_delay = params->x_checkpoint_delay; } if (params->has_block_incremental) { + dest->has_block_incremental = true; dest->block_incremental = params->block_incremental; } if (params->has_multifd_channels) { + dest->has_multifd_channels = true; dest->multifd_channels = params->multifd_channels; } if (params->has_multifd_compression) { + dest->has_multifd_compression = true; dest->multifd_compression = params->multifd_compression; } if (params->has_xbzrle_cache_size) { + dest->has_xbzrle_cache_size = true; dest->xbzrle_cache_size = params->xbzrle_cache_size; } if (params->has_max_postcopy_bandwidth) { + dest->has_max_postcopy_bandwidth = true; dest->max_postcopy_bandwidth = params->max_postcopy_bandwidth; } if (params->has_max_cpu_throttle) { + dest->has_max_cpu_throttle = true; dest->max_cpu_throttle = params->max_cpu_throttle; } if (params->has_announce_initial) { + dest->has_announce_initial = true; dest->announce_initial = params->announce_initial; } if (params->has_announce_max) { + dest->has_announce_max = true; dest->announce_max = params->announce_max; } if (params->has_announce_rounds) { + dest->has_announce_rounds = true; dest->announce_rounds = params->announce_rounds; } if (params->has_announce_step) { + dest->has_announce_step = true; dest->announce_step = params->announce_step; } @@ -1321,6 +1340,7 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp) params->tls_hostname->u.s = strdup(""); } + memset(&tmp, 0, sizeof(MigrationParameters)); migrate_params_test_apply(params, &tmp); if (!migrate_params_check(&tmp, errp)) {
qmp_migrate_set_parameters expects to use tmp for parameters check, so migrate_params_test_apply is expected to copy the related fields from params to tmp. So fix migrate_params_test_apply to use the function parameter, *dest, rather than the global one. The dest->has_xxx (xxx is the feature name) related fields need to be set, as they will be checked by migrate_params_check. Signed-off-by: Wei Wang <wei.w.wang@intel.com> --- migration/options.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)