From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: qemu-devel@nongnu.org, quintela@redhat.com
Subject: Re: [PATCH 2/6] migration: Fix migrate-set-parameters argument validation
Date: Fri, 13 Nov 2020 11:49:26 +0000 [thread overview]
Message-ID: <20201113114926.GF3251@work-vm> (raw)
In-Reply-To: <20201113065236.2644169-3-armbru@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
next prev parent reply other threads:[~2020-11-13 11:50 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-13 6:52 [PATCH 0/6] migration: Fixes and cleanups aroung migrate-set-parameters Markus Armbruster
2020-11-13 6:52 ` [PATCH 1/6] migration: Fix and clean up around @tls-authz Markus Armbruster
2020-11-13 11:56 ` Dr. David Alan Gilbert
2020-12-10 17:35 ` Dr. David Alan Gilbert
2020-12-10 18:10 ` Daniel P. Berrangé
2020-12-14 10:14 ` Markus Armbruster
2020-12-16 10:55 ` Daniel P. Berrangé
2020-12-17 13:07 ` Markus Armbruster
2020-12-17 14:04 ` Daniel P. Berrangé
2021-01-27 16:01 ` Markus Armbruster
2020-11-13 6:52 ` [PATCH 2/6] migration: Fix migrate-set-parameters argument validation Markus Armbruster
2020-11-13 11:49 ` Dr. David Alan Gilbert [this message]
2020-11-13 13:24 ` Markus Armbruster
2020-11-13 6:52 ` [PATCH 3/6] migration: Clean up signed vs. unsigned XBZRLE cache-size Markus Armbruster
2020-11-13 10:40 ` Dr. David Alan Gilbert
2020-11-13 6:52 ` [PATCH 4/6] migration: Check xbzrle-cache-size more carefully Markus Armbruster
2020-11-13 10:59 ` Dr. David Alan Gilbert
2020-11-13 13:35 ` Markus Armbruster
2020-11-13 16:39 ` Dr. David Alan Gilbert
2020-11-16 7:00 ` Markus Armbruster
2020-11-13 6:52 ` [PATCH 5/6] migration: Fix cache_init()'s "Failed to allocate" error messages Markus Armbruster
2020-11-13 11:01 ` Dr. David Alan Gilbert
2020-11-13 6:52 ` [PATCH 6/6] migration: Fix a few absurdly defective " Markus Armbruster
2020-11-13 11:27 ` Dr. David Alan Gilbert
2020-11-13 11:56 ` [PATCH 0/6] migration: Fixes and cleanups aroung migrate-set-parameters Dr. David Alan Gilbert
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201113114926.GF3251@work-vm \
--to=dgilbert@redhat.com \
--cc=armbru@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=quintela@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.