[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/3] migrate: Share common MigrationParameters s
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH 2/3] migrate: Share common MigrationParameters struct |
Date: |
Fri, 09 Sep 2016 09:08:38 +0000 |
On Fri, Sep 9, 2016 at 7:19 AM Eric Blake <address@hidden> wrote:
> It is rather verbose, and slightly error-prone, to repeat
> the same set of parameters for input (migrate-set-parameters)
> as for output (query-migrate-parameters), where the only
> difference is whether the members are optional. We can just
> document that the optional members will always be present
> on output, and then share a common struct between both
> commands. The next patch can then reduce the amount of
> code needed on input.
>
> Also, we made a mistake in qemu 2.7 of returning an empty
> string during 'query-migrate-parameters' when there is no
> TLS, rather than omitting TLS details entirely. Technically,
> this change risks breaking any 2.7 client that is hard-coded
> to expect the parameter's existence; on the other hand, clients
> that are portable to 2.6 already must be prepared for those
> members to not be present.
>
> And this gets rid of yet one more place where the QMP output
> visitor is silently converting a NULL string into "" (which
> is a hack I ultimately want to kill off).
>
> Signed-off-by: Eric Blake <address@hidden>
>
Reviewed-by: Marc-André Lureau <address@hidden>
> ---
> qapi-schema.json | 116
> +++++++++++++++++++-------------------------------
> hmp.c | 9 +++-
> migration/migration.c | 7 +++
> 3 files changed, 57 insertions(+), 75 deletions(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index c4f3674..4a51e5b 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -647,40 +647,53 @@
> #
> # @migrate-set-parameters
> #
> -# Set the following migration parameters
> -#
> -# @compress-level: compression level
> -#
> -# @compress-threads: compression thread count
> -#
> -# @decompress-threads: decompression thread count
> -#
> -# @cpu-throttle-initial: Initial percentage of time guest cpus are
> throttled
> -# when migration auto-converge is activated. The
> -# default value is 20. (Since 2.7)
> -#
> -# @cpu-throttle-increment: throttle percentage increase each time
> -# auto-converge detects that migration is not
> making
> -# progress. The default value is 10. (Since 2.7)
> -#
> -# @tls-creds: ID of the 'tls-creds' object that provides credentials for
> -# establishing a TLS connection over the migration data
> channel.
> -# On the outgoing side of the migration, the credentials must
> -# be for a 'client' endpoint, while for the incoming side the
> -# credentials must be for a 'server' endpoint. Setting this
> -# will enable TLS for all migrations. The default is unset,
> -# resulting in unsecured migration at the QEMU level. (Since
> 2.7)
> -#
> -# @tls-hostname: hostname of the target host for the migration. This is
> -# required when using x509 based TLS credentials and the
> -# migration URI does not already include a hostname. For
> -# example if using fd: or exec: based migration, the
> -# hostname must be provided so that the server's x509
> -# certificate identity can be validated. (Since 2.7)
> +# Set various migration parameters. See MigrationParameters for details.
> #
> # Since: 2.4
> ##
> { 'command': 'migrate-set-parameters',
> + 'data': 'MigrationParameters' }
> +
> +#
> +# @MigrationParameters
> +#
> +# Optional members can be omitted on input ('migrate-set-parameters')
> +# but most members will always be present on output
> +# ('query-migrate-parameters'), with the exception of tls-creds and
> +# tls-hostname.
> +#
> +# @compress-level: #optional compression level
> +#
> +# @compress-threads: #optional compression thread count
> +#
> +# @decompress-threads: #optional decompression thread count
> +#
> +# @cpu-throttle-initial: #optional Initial percentage of time guest cpus
> are
> +# throttledwhen migration auto-converge is
> activated.
> +# The default value is 20. (Since 2.7)
> +#
> +# @cpu-throttle-increment: #optional throttle percentage increase each
> time
> +# auto-converge detects that migration is not
> making
> +# progress. The default value is 10. (Since 2.7)
> +#
> +# @tls-creds: #optional ID of the 'tls-creds' object that provides
> credentials
> +# for establishing a TLS connection over the migration data
> +# channel. On the outgoing side of the migration, the
> credentials
> +# must be for a 'client' endpoint, while for the incoming
> side the
> +# credentials must be for a 'server' endpoint. Setting this
> +# will enable TLS for all migrations. The default is unset,
> +# resulting in unsecured migration at the QEMU level. (Since
> 2.7)
> +#
> +# @tls-hostname: #optional hostname of the target host for the migration.
> This
> +# is required when using x509 based TLS credentials and the
> +# migration URI does not already include a hostname. For
> +# example if using fd: or exec: based migration, the
> +# hostname must be provided so that the server's x509
> +# certificate identity can be validated. (Since 2.7)
> +#
> +# Since: 2.4
> +##
> +{ 'struct': 'MigrationParameters',
> 'data': { '*compress-level': 'int',
> '*compress-threads': 'int',
> '*decompress-threads': 'int',
> @@ -688,49 +701,6 @@
> '*cpu-throttle-increment': 'int',
> '*tls-creds': 'str',
> '*tls-hostname': 'str'} }
> -
> -#
> -# @MigrationParameters
> -#
> -# @compress-level: compression level
> -#
> -# @compress-threads: compression thread count
> -#
> -# @decompress-threads: decompression thread count
> -#
> -# @cpu-throttle-initial: Initial percentage of time guest cpus are
> throttled
> -# when migration auto-converge is activated. The
> -# default value is 20. (Since 2.7)
> -#
> -# @cpu-throttle-increment: throttle percentage increase each time
> -# auto-converge detects that migration is not
> making
> -# progress. The default value is 10. (Since 2.7)
> -#
> -# @tls-creds: ID of the 'tls-creds' object that provides credentials for
> -# establishing a TLS connection over the migration data
> channel.
> -# On the outgoing side of the migration, the credentials must
> -# be for a 'client' endpoint, while for the incoming side the
> -# credentials must be for a 'server' endpoint. Setting this
> -# will enable TLS for all migrations. The default is unset,
> -# resulting in unsecured migration at the QEMU level. (Since
> 2.7)
> -#
> -# @tls-hostname: hostname of the target host for the migration. This is
> -# required when using x509 based TLS credentials and the
> -# migration URI does not already include a hostname. For
> -# example if using fd: or exec: based migration, the
> -# hostname must be provided so that the server's x509
> -# certificate identity can be validated. (Since 2.7)
> -#
> -# Since: 2.4
> -##
> -{ 'struct': 'MigrationParameters',
> - 'data': { 'compress-level': 'int',
> - 'compress-threads': 'int',
> - 'decompress-threads': 'int',
> - 'cpu-throttle-initial': 'int',
> - 'cpu-throttle-increment': 'int',
> - 'tls-creds': 'str',
> - 'tls-hostname': 'str'} }
> ##
> # @query-migrate-parameters
> #
> diff --git a/hmp.c b/hmp.c
> index d6c6c01..1e4094a 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -283,27 +283,32 @@ void hmp_info_migrate_parameters(Monitor *mon, const
> QDict *qdict)
>
> if (params) {
> monitor_printf(mon, "parameters:");
> + assert(params->has_compress_level);
> monitor_printf(mon, " %s: %" PRId64,
> MigrationParameter_lookup[MIGRATION_PARAMETER_COMPRESS_LEVEL],
> params->compress_level);
> + assert(params->has_compress_threads);
> monitor_printf(mon, " %s: %" PRId64,
>
> MigrationParameter_lookup[MIGRATION_PARAMETER_COMPRESS_THREADS],
> params->compress_threads);
> + assert(params->has_decompress_threads);
> monitor_printf(mon, " %s: %" PRId64,
>
> MigrationParameter_lookup[MIGRATION_PARAMETER_DECOMPRESS_THREADS],
> params->decompress_threads);
> + assert(params->has_cpu_throttle_initial);
> monitor_printf(mon, " %s: %" PRId64,
>
> MigrationParameter_lookup[MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL],
> params->cpu_throttle_initial);
> + assert(params->has_cpu_throttle_increment);
> monitor_printf(mon, " %s: %" PRId64,
>
> MigrationParameter_lookup[MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT],
> params->cpu_throttle_increment);
> monitor_printf(mon, " %s: '%s'",
> MigrationParameter_lookup[MIGRATION_PARAMETER_TLS_CREDS],
> - params->tls_creds ? : "");
> + params->has_tls_creds ? params->tls_creds : "");
> monitor_printf(mon, " %s: '%s'",
> MigrationParameter_lookup[MIGRATION_PARAMETER_TLS_HOSTNAME],
> - params->tls_hostname ? : "");
> + params->has_tls_hostname ? params->tls_hostname : "");
> monitor_printf(mon, "\n");
> }
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 955d5ee..1a8f26b 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -559,12 +559,19 @@ MigrationParameters
> *qmp_query_migrate_parameters(Error **errp)
> MigrationState *s = migrate_get_current();
>
> params = g_malloc0(sizeof(*params));
> + params->has_compress_level = true;
> params->compress_level = s->parameters.compress_level;
> + params->has_compress_threads = true;
> params->compress_threads = s->parameters.compress_threads;
> + params->has_decompress_threads = true;
> params->decompress_threads = s->parameters.decompress_threads;
> + params->has_cpu_throttle_initial = true;
> params->cpu_throttle_initial = s->parameters.cpu_throttle_initial;
> + params->has_cpu_throttle_increment = true;
> params->cpu_throttle_increment = s->parameters.cpu_throttle_increment;
> + params->has_tls_creds = !!s->parameters.tls_creds;
> params->tls_creds = g_strdup(s->parameters.tls_creds);
> + params->has_tls_hostname = !!s->parameters.tls_hostname;
> params->tls_hostname = g_strdup(s->parameters.tls_hostname);
>
> return params;
> --
> 2.7.4
>
>
> --
Marc-André Lureau