[Qemu-devel] [PATCH 0/3] migrate: simplify migrate-set-parameters

[Qemu-devel] [PATCH 0/3] migrate: simplify migrate-set-parameters

[Eric Blake]

As promised earlier today, here's a patch series to simplify how one
calls into qmp_migrate_set_parameters().  With this in place,
Ashijeet's patches for the back-compat functions would look like:

void qmp_migrate_set_speed(int64_t valuebw, Error **errp)
{
    MigrationParameters *p = {
        .has_max_bandwidth = true,
        .max_bandwidth = valuebw,
    };
    qmp_migrate_set_parameters(&p, errp);
}

This series is a net reduction in lines, so it has to be good, right? :)

Oh, and I found (and fixed) a 2.7 regression while touching this stuff.

Eric Blake (3):
  migrate: Fix cpu-throttle-increment regression in HMP
  migrate: Share common MigrationParameters struct
  migrate: Use boxed qapi for migrate-set-parameters

 qapi-schema.json      | 86 +++++++++++++++++----------------------------------
 hmp.c                 | 50 ++++++++++++++++--------------
 migration/migration.c | 72 ++++++++++++++++++++----------------------
 3 files changed, 88 insertions(+), 120 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/3] migrate: Fix cpu-throttle-increment regression in HMP

[Eric Blake]

Commit 69ef1f3 accidentally broke migrate_set_parameter's ability
to set the cpu-throttle-increment to anything other than the
default, because it forgot to parse the user's string into an
integer.

CC: qemu-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 hmp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hmp.c b/hmp.c
index ad33b44..d6c6c01 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1286,6 +1286,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
                 break;
             case MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT:
                 has_cpu_throttle_increment = true;
+                use_int_value = true;
                 break;
             case MIGRATION_PARAMETER_TLS_CREDS:
                 has_tls_creds = true;
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/3] migrate: Fix cpu-throttle-increment regression in HMP

[Marc-André Lureau]

On Fri, Sep 9, 2016 at 7:16 AM Eric Blake <eblake@redhat.com> wrote:

> Commit 69ef1f3 accidentally broke migrate_set_parameter's ability
> to set the cpu-throttle-increment to anything other than the
> default, because it forgot to parse the user's string into an
> integer.
>
> CC: qemu-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
>  hmp.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hmp.c b/hmp.c
> index ad33b44..d6c6c01 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1286,6 +1286,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const
> QDict *qdict)
>                  break;
>              case MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT:
>                  has_cpu_throttle_increment = true;
> +                use_int_value = true;
>                  break;
>              case MIGRATION_PARAMETER_TLS_CREDS:
>                  has_tls_creds = true;
> --
> 2.7.4
>
>
> --
Marc-André Lureau

Re: [Qemu-devel] [PATCH 1/3] migrate: Fix cpu-throttle-increment regression in HMP

[Juan Quintela]

Eric Blake <eblake@redhat.com> wrote:
> Commit 69ef1f3 accidentally broke migrate_set_parameter's ability
> to set the cpu-throttle-increment to anything other than the
> default, because it forgot to parse the user's string into an
> integer.
>
> CC: qemu-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  hmp.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hmp.c b/hmp.c
> index ad33b44..d6c6c01 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1286,6 +1286,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>                  break;
>              case MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT:
>                  has_cpu_throttle_increment = true;
> +                use_int_value = true;
>                  break;
>              case MIGRATION_PARAMETER_TLS_CREDS:
>                  has_tls_creds = true;

Reviewed-by: Juan Quintela <quintela@redhat.com>

[Qemu-devel] [PATCH 2/3] migrate: Share common MigrationParameters struct

[Eric Blake]

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 <eblake@redhat.com>
---
 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

Re: [Qemu-devel] [PATCH 2/3] migrate: Share common MigrationParameters struct

[Marc-André Lureau]

On Fri, Sep 9, 2016 at 7:19 AM Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com>
>


Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  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

Re: [Qemu-devel] [PATCH 2/3] migrate: Share common MigrationParameters struct

[Juan Quintela]

Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [Qemu-devel] [PATCH 2/3] migrate: Share common MigrationParameters struct

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1294 bytes --]

On 09/08/2016 10:14 PM, 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.

>              '*tls-hostname': 'str'} }
> -
> -#
> -# @MigrationParameters
> -#
...
> -##
> -{ '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

Pre-existing - there was no blank line before the docs for
query-migrate-parameters.  Commit a43edcf fixed the blank line, then the
merge conflict resolution undid things; so I've submitted a followup.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 3/3] migrate: Use boxed qapi for migrate-set-parameters

[Eric Blake]

Now that QAPI makes it easy to pass a struct around, we don't
have to declare as many parameters or local variables.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qapi-schema.json      |  2 +-
 hmp.c                 | 40 ++++++++++++++-----------------
 migration/migration.c | 65 +++++++++++++++++++++------------------------------
 3 files changed, 46 insertions(+), 61 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 4a51e5b..88b4888 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -651,7 +651,7 @@
 #
 # Since: 2.4
 ##
-{ 'command': 'migrate-set-parameters',
+{ 'command': 'migrate-set-parameters', 'boxed': true,
   'data': 'MigrationParameters' }

 #
diff --git a/hmp.c b/hmp.c
index 1e4094a..0893b8e 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1260,44 +1260,40 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
     const char *valuestr = qdict_get_str(qdict, "value");
     long valueint = 0;
     Error *err = NULL;
-    bool has_compress_level = false;
-    bool has_compress_threads = false;
-    bool has_decompress_threads = false;
-    bool has_cpu_throttle_initial = false;
-    bool has_cpu_throttle_increment = false;
-    bool has_tls_creds = false;
-    bool has_tls_hostname = false;
     bool use_int_value = false;
     int i;

     for (i = 0; i < MIGRATION_PARAMETER__MAX; i++) {
         if (strcmp(param, MigrationParameter_lookup[i]) == 0) {
+            MigrationParameters p = { 0 };
             switch (i) {
             case MIGRATION_PARAMETER_COMPRESS_LEVEL:
-                has_compress_level = true;
+                p.has_compress_level = true;
                 use_int_value = true;
                 break;
             case MIGRATION_PARAMETER_COMPRESS_THREADS:
-                has_compress_threads = true;
+                p.has_compress_threads = true;
                 use_int_value = true;
                 break;
             case MIGRATION_PARAMETER_DECOMPRESS_THREADS:
-                has_decompress_threads = true;
+                p.has_decompress_threads = true;
                 use_int_value = true;
                 break;
             case MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL:
-                has_cpu_throttle_initial = true;
+                p.has_cpu_throttle_initial = true;
                 use_int_value = true;
                 break;
             case MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT:
-                has_cpu_throttle_increment = true;
+                p.has_cpu_throttle_increment = true;
                 use_int_value = true;
                 break;
             case MIGRATION_PARAMETER_TLS_CREDS:
-                has_tls_creds = true;
+                p.has_tls_creds = true;
+                p.tls_creds = (char *) valuestr;
                 break;
             case MIGRATION_PARAMETER_TLS_HOSTNAME:
-                has_tls_hostname = true;
+                p.has_tls_hostname = true;
+                p.tls_hostname = (char *) valuestr;
                 break;
             }

@@ -1307,16 +1303,16 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
                                valuestr);
                     goto cleanup;
                 }
+                /* Set all integers; only one has_FOO will be set, and
+                 * the code ignores the remaining values */
+                p.compress_level = valueint;
+                p.compress_threads = valueint;
+                p.decompress_threads = valueint;
+                p.cpu_throttle_initial = valueint;
+                p.cpu_throttle_increment = valueint;
             }

-            qmp_migrate_set_parameters(has_compress_level, valueint,
-                                       has_compress_threads, valueint,
-                                       has_decompress_threads, valueint,
-                                       has_cpu_throttle_initial, valueint,
-                                       has_cpu_throttle_increment, valueint,
-                                       has_tls_creds, valuestr,
-                                       has_tls_hostname, valuestr,
-                                       &err);
+            qmp_migrate_set_parameters(&p, &err);
             break;
         }
     }
diff --git a/migration/migration.c b/migration/migration.c
index 1a8f26b..42336e3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -766,78 +766,67 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
     }
 }

-void qmp_migrate_set_parameters(bool has_compress_level,
-                                int64_t compress_level,
-                                bool has_compress_threads,
-                                int64_t compress_threads,
-                                bool has_decompress_threads,
-                                int64_t decompress_threads,
-                                bool has_cpu_throttle_initial,
-                                int64_t cpu_throttle_initial,
-                                bool has_cpu_throttle_increment,
-                                int64_t cpu_throttle_increment,
-                                bool has_tls_creds,
-                                const char *tls_creds,
-                                bool has_tls_hostname,
-                                const char *tls_hostname,
-                                Error **errp)
+void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
 {
     MigrationState *s = migrate_get_current();

-    if (has_compress_level && (compress_level < 0 || compress_level > 9)) {
+    if (params->has_compress_level &&
+        (params->compress_level < 0 || params->compress_level > 9)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
                    "is invalid, it should be in the range of 0 to 9");
         return;
     }
-    if (has_compress_threads &&
-            (compress_threads < 1 || compress_threads > 255)) {
+    if (params->has_compress_threads &&
+        (params->compress_threads < 1 || params->compress_threads > 255)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    "compress_threads",
                    "is invalid, it should be in the range of 1 to 255");
         return;
     }
-    if (has_decompress_threads &&
-            (decompress_threads < 1 || decompress_threads > 255)) {
+    if (params->has_decompress_threads &&
+        (params->decompress_threads < 1 || params->decompress_threads > 255)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    "decompress_threads",
                    "is invalid, it should be in the range of 1 to 255");
         return;
     }
-    if (has_cpu_throttle_initial &&
-            (cpu_throttle_initial < 1 || cpu_throttle_initial > 99)) {
+    if (params->has_cpu_throttle_initial &&
+        (params->cpu_throttle_initial < 1 ||
+         params->cpu_throttle_initial > 99)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    "cpu_throttle_initial",
                    "an integer in the range of 1 to 99");
     }
-    if (has_cpu_throttle_increment &&
-            (cpu_throttle_increment < 1 || cpu_throttle_increment > 99)) {
+    if (params->has_cpu_throttle_increment &&
+        (params->cpu_throttle_increment < 1 ||
+         params->cpu_throttle_increment > 99)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                    "cpu_throttle_increment",
                    "an integer in the range of 1 to 99");
     }

-    if (has_compress_level) {
-        s->parameters.compress_level = compress_level;
+    if (params->has_compress_level) {
+        s->parameters.compress_level = params->compress_level;
     }
-    if (has_compress_threads) {
-        s->parameters.compress_threads = compress_threads;
+    if (params->has_compress_threads) {
+        s->parameters.compress_threads = params->compress_threads;
     }
-    if (has_decompress_threads) {
-        s->parameters.decompress_threads = decompress_threads;
+    if (params->has_decompress_threads) {
+        s->parameters.decompress_threads = params->decompress_threads;
     }
-    if (has_cpu_throttle_initial) {
-        s->parameters.cpu_throttle_initial = cpu_throttle_initial;
+    if (params->has_cpu_throttle_initial) {
+        s->parameters.cpu_throttle_initial = params->cpu_throttle_initial;
     }
-    if (has_cpu_throttle_increment) {
-        s->parameters.cpu_throttle_increment = cpu_throttle_increment;
+    if (params->has_cpu_throttle_increment) {
+        s->parameters.cpu_throttle_increment = params->cpu_throttle_increment;
     }
-    if (has_tls_creds) {
+    if (params->has_tls_creds) {
         g_free(s->parameters.tls_creds);
-        s->parameters.tls_creds = g_strdup(tls_creds);
+        s->parameters.tls_creds = g_strdup(params->tls_creds);
     }
-    if (has_tls_hostname) {
+    if (params->has_tls_hostname) {
         g_free(s->parameters.tls_hostname);
-        s->parameters.tls_hostname = g_strdup(tls_hostname);
+        s->parameters.tls_hostname = g_strdup(params->tls_hostname);
     }
 }

-- 
2.7.4

Re: [Qemu-devel] [PATCH 3/3] migrate: Use boxed qapi for migrate-set-parameters

[Marc-André Lureau]

On Fri, Sep 9, 2016 at 7:16 AM Eric Blake <eblake@redhat.com> wrote:

> Now that QAPI makes it easy to pass a struct around, we don't
> have to declare as many parameters or local variables.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  qapi-schema.json      |  2 +-
>  hmp.c                 | 40 ++++++++++++++-----------------
>  migration/migration.c | 65
> +++++++++++++++++++++------------------------------
>  3 files changed, 46 insertions(+), 61 deletions(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 4a51e5b..88b4888 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -651,7 +651,7 @@
>  #
>  # Since: 2.4
>  ##
> -{ 'command': 'migrate-set-parameters',
> +{ 'command': 'migrate-set-parameters', 'boxed': true,
>    'data': 'MigrationParameters' }
>
>  #
> diff --git a/hmp.c b/hmp.c
> index 1e4094a..0893b8e 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1260,44 +1260,40 @@ void hmp_migrate_set_parameter(Monitor *mon, const
> QDict *qdict)
>      const char *valuestr = qdict_get_str(qdict, "value");
>      long valueint = 0;
>      Error *err = NULL;
> -    bool has_compress_level = false;
> -    bool has_compress_threads = false;
> -    bool has_decompress_threads = false;
> -    bool has_cpu_throttle_initial = false;
> -    bool has_cpu_throttle_increment = false;
> -    bool has_tls_creds = false;
> -    bool has_tls_hostname = false;
>      bool use_int_value = false;
>      int i;
>
>      for (i = 0; i < MIGRATION_PARAMETER__MAX; i++) {
>          if (strcmp(param, MigrationParameter_lookup[i]) == 0) {
> +            MigrationParameters p = { 0 };
>              switch (i) {
>              case MIGRATION_PARAMETER_COMPRESS_LEVEL:
> -                has_compress_level = true;
> +                p.has_compress_level = true;
>                  use_int_value = true;
>                  break;
>              case MIGRATION_PARAMETER_COMPRESS_THREADS:
> -                has_compress_threads = true;
> +                p.has_compress_threads = true;
>                  use_int_value = true;
>                  break;
>              case MIGRATION_PARAMETER_DECOMPRESS_THREADS:
> -                has_decompress_threads = true;
> +                p.has_decompress_threads = true;
>                  use_int_value = true;
>                  break;
>              case MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL:
> -                has_cpu_throttle_initial = true;
> +                p.has_cpu_throttle_initial = true;
>                  use_int_value = true;
>                  break;
>              case MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT:
> -                has_cpu_throttle_increment = true;
> +                p.has_cpu_throttle_increment = true;
>                  use_int_value = true;
>                  break;
>              case MIGRATION_PARAMETER_TLS_CREDS:
> -                has_tls_creds = true;
> +                p.has_tls_creds = true;
> +                p.tls_creds = (char *) valuestr;
>                  break;
>              case MIGRATION_PARAMETER_TLS_HOSTNAME:
> -                has_tls_hostname = true;
> +                p.has_tls_hostname = true;
> +                p.tls_hostname = (char *) valuestr;
>                  break;
>              }
>
> @@ -1307,16 +1303,16 @@ void hmp_migrate_set_parameter(Monitor *mon, const
> QDict *qdict)
>                                 valuestr);
>                      goto cleanup;
>                  }
> +                /* Set all integers; only one has_FOO will be set, and
> +                 * the code ignores the remaining values */
> +                p.compress_level = valueint;
> +                p.compress_threads = valueint;
> +                p.decompress_threads = valueint;
> +                p.cpu_throttle_initial = valueint;
> +                p.cpu_throttle_increment = valueint;
>              }
>
> -            qmp_migrate_set_parameters(has_compress_level, valueint,
> -                                       has_compress_threads, valueint,
> -                                       has_decompress_threads, valueint,
> -                                       has_cpu_throttle_initial, valueint,
> -                                       has_cpu_throttle_increment,
> valueint,
> -                                       has_tls_creds, valuestr,
> -                                       has_tls_hostname, valuestr,
> -                                       &err);
> +            qmp_migrate_set_parameters(&p, &err);
>              break;
>          }
>      }
> diff --git a/migration/migration.c b/migration/migration.c
> index 1a8f26b..42336e3 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -766,78 +766,67 @@ void
> qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
>      }
>  }
>
> -void qmp_migrate_set_parameters(bool has_compress_level,
> -                                int64_t compress_level,
> -                                bool has_compress_threads,
> -                                int64_t compress_threads,
> -                                bool has_decompress_threads,
> -                                int64_t decompress_threads,
> -                                bool has_cpu_throttle_initial,
> -                                int64_t cpu_throttle_initial,
> -                                bool has_cpu_throttle_increment,
> -                                int64_t cpu_throttle_increment,
> -                                bool has_tls_creds,
> -                                const char *tls_creds,
> -                                bool has_tls_hostname,
> -                                const char *tls_hostname,
> -                                Error **errp)
> +void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>  {
>      MigrationState *s = migrate_get_current();
>
> -    if (has_compress_level && (compress_level < 0 || compress_level > 9))
> {
> +    if (params->has_compress_level &&
> +        (params->compress_level < 0 || params->compress_level > 9)) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
>                     "is invalid, it should be in the range of 0 to 9");
>          return;
>      }
> -    if (has_compress_threads &&
> -            (compress_threads < 1 || compress_threads > 255)) {
> +    if (params->has_compress_threads &&
> +        (params->compress_threads < 1 || params->compress_threads > 255))
> {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                     "compress_threads",
>                     "is invalid, it should be in the range of 1 to 255");
>          return;
>      }
> -    if (has_decompress_threads &&
> -            (decompress_threads < 1 || decompress_threads > 255)) {
> +    if (params->has_decompress_threads &&
> +        (params->decompress_threads < 1 || params->decompress_threads >
> 255)) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                     "decompress_threads",
>                     "is invalid, it should be in the range of 1 to 255");
>          return;
>      }
> -    if (has_cpu_throttle_initial &&
> -            (cpu_throttle_initial < 1 || cpu_throttle_initial > 99)) {
> +    if (params->has_cpu_throttle_initial &&
> +        (params->cpu_throttle_initial < 1 ||
> +         params->cpu_throttle_initial > 99)) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                     "cpu_throttle_initial",
>                     "an integer in the range of 1 to 99");
>      }
> -    if (has_cpu_throttle_increment &&
> -            (cpu_throttle_increment < 1 || cpu_throttle_increment > 99)) {
> +    if (params->has_cpu_throttle_increment &&
> +        (params->cpu_throttle_increment < 1 ||
> +         params->cpu_throttle_increment > 99)) {
>          error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>                     "cpu_throttle_increment",
>                     "an integer in the range of 1 to 99");
>      }
>
> -    if (has_compress_level) {
> -        s->parameters.compress_level = compress_level;
> +    if (params->has_compress_level) {
> +        s->parameters.compress_level = params->compress_level;
>      }
> -    if (has_compress_threads) {
> -        s->parameters.compress_threads = compress_threads;
> +    if (params->has_compress_threads) {
> +        s->parameters.compress_threads = params->compress_threads;
>      }
> -    if (has_decompress_threads) {
> -        s->parameters.decompress_threads = decompress_threads;
> +    if (params->has_decompress_threads) {
> +        s->parameters.decompress_threads = params->decompress_threads;
>      }
> -    if (has_cpu_throttle_initial) {
> -        s->parameters.cpu_throttle_initial = cpu_throttle_initial;
> +    if (params->has_cpu_throttle_initial) {
> +        s->parameters.cpu_throttle_initial = params->cpu_throttle_initial;
>      }
> -    if (has_cpu_throttle_increment) {
> -        s->parameters.cpu_throttle_increment = cpu_throttle_increment;
> +    if (params->has_cpu_throttle_increment) {
> +        s->parameters.cpu_throttle_increment =
> params->cpu_throttle_increment;
>      }
> -    if (has_tls_creds) {
> +    if (params->has_tls_creds) {
>          g_free(s->parameters.tls_creds);
> -        s->parameters.tls_creds = g_strdup(tls_creds);
> +        s->parameters.tls_creds = g_strdup(params->tls_creds);
>      }
> -    if (has_tls_hostname) {
> +    if (params->has_tls_hostname) {
>          g_free(s->parameters.tls_hostname);
> -        s->parameters.tls_hostname = g_strdup(tls_hostname);
> +        s->parameters.tls_hostname = g_strdup(params->tls_hostname);
>      }
>  }
>
> --
> 2.7.4
>
>
> --
Marc-André Lureau

Re: [Qemu-devel] [PATCH 3/3] migrate: Use boxed qapi for migrate-set-parameters

[Juan Quintela]

Eric Blake <eblake@redhat.com> wrote:
> Now that QAPI makes it easy to pass a struct around, we don't
> have to declare as many parameters or local variables.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

NIIIICEEEEEEE

I hated that function parameters list.  Thanks very much.

Re: [Qemu-devel] [PATCH 0/3] migrate: simplify migrate-set-parameters

[Juan Quintela]

Eric Blake <eblake@redhat.com> wrote:
> As promised earlier today, here's a patch series to simplify how one
> calls into qmp_migrate_set_parameters().  With this in place,
> Ashijeet's patches for the back-compat functions would look like:
>
> void qmp_migrate_set_speed(int64_t valuebw, Error **errp)
> {
>     MigrationParameters *p = {
>         .has_max_bandwidth = true,
>         .max_bandwidth = valuebw,
>     };
>     qmp_migrate_set_parameters(&p, errp);
> }
>
> This series is a net reduction in lines, so it has to be good, right? :)

it is always nice.  Especially because they were ugly lines.

Applied.

>
> Oh, and I found (and fixed) a 2.7 regression while touching this stuff.
>
> Eric Blake (3):
>   migrate: Fix cpu-throttle-increment regression in HMP
>   migrate: Share common MigrationParameters struct
>   migrate: Use boxed qapi for migrate-set-parameters
>
>  qapi-schema.json      | 86 +++++++++++++++++----------------------------------
>  hmp.c                 | 50 ++++++++++++++++--------------
>  migration/migration.c | 72 ++++++++++++++++++++----------------------
>  3 files changed, 88 insertions(+), 120 deletions(-)