[PATCH v1] migration: fix migrate_params_test_apply to set the dest param correctly

[PATCH v1] migration: fix migrate_params_test_apply to set the dest param correctly

[Wei Wang]

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(-)

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

Re: [PATCH v1] migration: fix migrate_params_test_apply to set the dest param correctly

[Peter Xu]

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
> 

-- 
Peter Xu

RE: [PATCH v1] migration: fix migrate_params_test_apply to set the dest param correctly

[Wang, Wei W]

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.

Re: [PATCH v1] migration: fix migrate_params_test_apply to set the dest param correctly

[Peter Xu]

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,

-- 
Peter Xu

RE: [PATCH v1] migration: fix migrate_params_test_apply to set the dest param correctly

[Wang, Wei W]

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)