From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34919) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1biHnq-00009T-IZ for qemu-devel@nongnu.org; Fri, 09 Sep 2016 05:08:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1biHnm-0007EU-Ue for qemu-devel@nongnu.org; Fri, 09 Sep 2016 05:08:53 -0400 Received: from mail-lf0-x242.google.com ([2a00:1450:4010:c07::242]:36462) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1biHnm-0007E5-Cu for qemu-devel@nongnu.org; Fri, 09 Sep 2016 05:08:50 -0400 Received: by mail-lf0-x242.google.com with SMTP id s29so2391425lfg.3 for ; Fri, 09 Sep 2016 02:08:50 -0700 (PDT) MIME-Version: 1.0 References: <1473390856-4502-1-git-send-email-eblake@redhat.com> <1473390856-4502-3-git-send-email-eblake@redhat.com> In-Reply-To: <1473390856-4502-3-git-send-email-eblake@redhat.com> From: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= Date: Fri, 09 Sep 2016 09:08:38 +0000 Message-ID: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/3] migrate: Share common MigrationParameters struct List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Amit Shah , Luiz Capitulino , Markus Armbruster , ashijeetacharya@gmail.com, quintela@redhat.com On Fri, Sep 9, 2016 at 7:19 AM Eric Blake 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 > Reviewed-by: Marc-Andr=C3=A9 Lureau > --- > 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 mus= t > -# be for a 'client' endpoint, while for the incoming side th= e > -# 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 t= he > +# 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 mus= t > -# be for a 'client' endpoint, while for the incoming side th= e > -# 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, cons= t > 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 =3D migrate_get_current(); > > params =3D g_malloc0(sizeof(*params)); > + params->has_compress_level =3D true; > params->compress_level =3D s->parameters.compress_level; > + params->has_compress_threads =3D true; > params->compress_threads =3D s->parameters.compress_threads; > + params->has_decompress_threads =3D true; > params->decompress_threads =3D s->parameters.decompress_threads; > + params->has_cpu_throttle_initial =3D true; > params->cpu_throttle_initial =3D s->parameters.cpu_throttle_initial; > + params->has_cpu_throttle_increment =3D true; > params->cpu_throttle_increment =3D s->parameters.cpu_throttle_increm= ent; > + params->has_tls_creds =3D !!s->parameters.tls_creds; > params->tls_creds =3D g_strdup(s->parameters.tls_creds); > + params->has_tls_hostname =3D !!s->parameters.tls_hostname; > params->tls_hostname =3D g_strdup(s->parameters.tls_hostname); > > return params; > -- > 2.7.4 > > > -- Marc-Andr=C3=A9 Lureau