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