[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/6] migration: Fix migrate-set-parameters argument validatio
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [PATCH 2/6] migration: Fix migrate-set-parameters argument validation |
Date: |
Fri, 13 Nov 2020 11:49:26 +0000 |
User-agent: |
Mutt/1.14.6 (2020-07-11) |
* 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
- [PATCH 0/6] migration: Fixes and cleanups aroung migrate-set-parameters, Markus Armbruster, 2020/11/13
- [PATCH 4/6] migration: Check xbzrle-cache-size more carefully, Markus Armbruster, 2020/11/13
- [PATCH 2/6] migration: Fix migrate-set-parameters argument validation, Markus Armbruster, 2020/11/13
- Re: [PATCH 2/6] migration: Fix migrate-set-parameters argument validation,
Dr. David Alan Gilbert <=
- [PATCH 6/6] migration: Fix a few absurdly defective error messages, Markus Armbruster, 2020/11/13
- [PATCH 3/6] migration: Clean up signed vs. unsigned XBZRLE cache-size, Markus Armbruster, 2020/11/13
- [PATCH 1/6] migration: Fix and clean up around @tls-authz, Markus Armbruster, 2020/11/13
- [PATCH 5/6] migration: Fix cache_init()'s "Failed to allocate" error messages, Markus Armbruster, 2020/11/13
- Re: [PATCH 0/6] migration: Fixes and cleanups aroung migrate-set-parameters, Dr. David Alan Gilbert, 2020/11/13