Re: [PATCH 2/6] migration: Fix migrate-set-parameters argument validation

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Markus Armbruster (armbru@redhat.com) wrote:
> Commit 741d4086c8 "migration: Use proper types in json" (v2.12.0)
> switched MigrationParameters to narrower integer types, and removed
> the simplified qmp_migrate_set_parameters()'s argument checking
> accordingly.
> 
> Good idea, except qmp_migrate_set_parameters() takes
> MigrateSetParameters, not MigrationParameters.  Its job is updating
> migrate_get_current()->parameters (which *is* of type
> MigrationParameters) according to its argument.  The integers now get
> truncated silently.  Reproducer:
> 
>     ---> {'execute': 'query-migrate-parameters'}
>     <--- {"return": {[...] "compress-threads": 8, [...]}}
>     ---> {"execute": "migrate-set-parameters", "arguments": {"compress-threads": 257}}
>     <--- {"return": {}}
>     ---> {'execute': 'query-migrate-parameters'}
>     <--- {"return": {[...] "compress-threads": 1, [...]}}
> 
> Fix by resynchronizing MigrateSetParameters with MigrationParameters.

Having those two separate types is a pain!

> Fixes: 741d4086c856320807a2575389d7c0505578270b
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  qapi/migration.json | 28 ++++++++++++++--------------
>  monitor/hmp-cmds.c  | 24 ++++++++++++------------
>  2 files changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 688e8da749..3ad3720cf0 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -885,28 +885,28 @@
>              '*announce-max': 'size',
>              '*announce-rounds': 'size',
>              '*announce-step': 'size',
> -            '*compress-level': 'int',
> -            '*compress-threads': 'int',
> +            '*compress-level': 'uint8',
> +            '*compress-threads': 'uint8',
>              '*compress-wait-thread': 'bool',
> -            '*decompress-threads': 'int',
> -            '*throttle-trigger-threshold': 'int',
> -            '*cpu-throttle-initial': 'int',
> -            '*cpu-throttle-increment': 'int',
> +            '*decompress-threads': 'uint8',
> +            '*throttle-trigger-threshold': 'uint8',
> +            '*cpu-throttle-initial': 'uint8',
> +            '*cpu-throttle-increment': 'uint8',
>              '*cpu-throttle-tailslow': 'bool',
>              '*tls-creds': 'StrOrNull',
>              '*tls-hostname': 'StrOrNull',
>              '*tls-authz': 'StrOrNull',
> -            '*max-bandwidth': 'int',
> -            '*downtime-limit': 'int',
> -            '*x-checkpoint-delay': 'int',
> +            '*max-bandwidth': 'size',
> +            '*downtime-limit': 'uint64',
> +            '*x-checkpoint-delay': 'uint32',
>              '*block-incremental': 'bool',
> -            '*multifd-channels': 'int',
> +            '*multifd-channels': 'uint8',
>              '*xbzrle-cache-size': 'size',
>              '*max-postcopy-bandwidth': 'size',
> -            '*max-cpu-throttle': 'int',
> +            '*max-cpu-throttle': 'uint8',
>              '*multifd-compression': 'MultiFDCompression',
> -            '*multifd-zlib-level': 'int',
> -            '*multifd-zstd-level': 'int',
> +            '*multifd-zlib-level': 'uint8',
> +            '*multifd-zstd-level': 'uint8',
>              '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
>  
>  ##
> @@ -1093,7 +1093,7 @@
>              '*max-bandwidth': 'size',
>              '*downtime-limit': 'uint64',
>              '*x-checkpoint-delay': 'uint32',
> -            '*block-incremental': 'bool' ,
> +            '*block-incremental': 'bool',
>              '*multifd-channels': 'uint8',
>              '*xbzrle-cache-size': 'size',
>              '*max-postcopy-bandwidth': 'size',
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 492789248f..f8ef061510 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1292,11 +1292,11 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>      switch (val) {
>      case MIGRATION_PARAMETER_COMPRESS_LEVEL:
>          p->has_compress_level = true;
> -        visit_type_int(v, param, &p->compress_level, &err);
> +        visit_type_uint8(v, param, &p->compress_level, &err);
>          break;
>      case MIGRATION_PARAMETER_COMPRESS_THREADS:
>          p->has_compress_threads = true;
> -        visit_type_int(v, param, &p->compress_threads, &err);
> +        visit_type_uint8(v, param, &p->compress_threads, &err);
>          break;
>      case MIGRATION_PARAMETER_COMPRESS_WAIT_THREAD:
>          p->has_compress_wait_thread = true;
> @@ -1304,19 +1304,19 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>          break;
>      case MIGRATION_PARAMETER_DECOMPRESS_THREADS:
>          p->has_decompress_threads = true;
> -        visit_type_int(v, param, &p->decompress_threads, &err);
> +        visit_type_uint8(v, param, &p->decompress_threads, &err);
>          break;
>      case MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD:
>          p->has_throttle_trigger_threshold = true;
> -        visit_type_int(v, param, &p->throttle_trigger_threshold, &err);
> +        visit_type_uint8(v, param, &p->throttle_trigger_threshold, &err);
>          break;
>      case MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL:
>          p->has_cpu_throttle_initial = true;
> -        visit_type_int(v, param, &p->cpu_throttle_initial, &err);
> +        visit_type_uint8(v, param, &p->cpu_throttle_initial, &err);
>          break;
>      case MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT:
>          p->has_cpu_throttle_increment = true;
> -        visit_type_int(v, param, &p->cpu_throttle_increment, &err);
> +        visit_type_uint8(v, param, &p->cpu_throttle_increment, &err);
>          break;
>      case MIGRATION_PARAMETER_CPU_THROTTLE_TAILSLOW:
>          p->has_cpu_throttle_tailslow = true;
> @@ -1324,7 +1324,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>          break;
>      case MIGRATION_PARAMETER_MAX_CPU_THROTTLE:
>          p->has_max_cpu_throttle = true;
> -        visit_type_int(v, param, &p->max_cpu_throttle, &err);
> +        visit_type_uint8(v, param, &p->max_cpu_throttle, &err);
>          break;
>      case MIGRATION_PARAMETER_TLS_CREDS:
>          p->has_tls_creds = true;
> @@ -1360,11 +1360,11 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>          break;
>      case MIGRATION_PARAMETER_DOWNTIME_LIMIT:
>          p->has_downtime_limit = true;
> -        visit_type_int(v, param, &p->downtime_limit, &err);
> +        visit_type_size(v, param, &p->downtime_limit, &err);
>          break;
>      case MIGRATION_PARAMETER_X_CHECKPOINT_DELAY:
>          p->has_x_checkpoint_delay = true;
> -        visit_type_int(v, param, &p->x_checkpoint_delay, &err);
> +        visit_type_uint32(v, param, &p->x_checkpoint_delay, &err);
>          break;
>      case MIGRATION_PARAMETER_BLOCK_INCREMENTAL:
>          p->has_block_incremental = true;
> @@ -1372,7 +1372,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>          break;
>      case MIGRATION_PARAMETER_MULTIFD_CHANNELS:
>          p->has_multifd_channels = true;
> -        visit_type_int(v, param, &p->multifd_channels, &err);
> +        visit_type_uint8(v, param, &p->multifd_channels, &err);
>          break;
>      case MIGRATION_PARAMETER_MULTIFD_COMPRESSION:
>          p->has_multifd_compression = true;
> @@ -1381,11 +1381,11 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>          break;
>      case MIGRATION_PARAMETER_MULTIFD_ZLIB_LEVEL:
>          p->has_multifd_zlib_level = true;
> -        visit_type_int(v, param, &p->multifd_zlib_level, &err);
> +        visit_type_uint8(v, param, &p->multifd_zlib_level, &err);
>          break;
>      case MIGRATION_PARAMETER_MULTIFD_ZSTD_LEVEL:
>          p->has_multifd_zstd_level = true;
> -        visit_type_int(v, param, &p->multifd_zstd_level, &err);
> +        visit_type_uint8(v, param, &p->multifd_zstd_level, &err);
>          break;
>      case MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE:
>          p->has_xbzrle_cache_size = true;
> -- 
> 2.26.2
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK