qemu-devel
[Top][All Lists]
Advanced

[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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]