[Qemu-devel] [PATCH v4] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp

[Qemu-devel] [PATCH v4] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp

[Ashijeet Acharya]

Mark old-commands for speed and downtime as deprecated.
Move max-bandwidth and downtime-limit into migrate-set-parameters for
setting maximum migration speed and expected downtime limit parameters
respectively.
Change downtime units to milliseconds (only for new-command) and update
the query part in both hmp and qmp qemu control interfaces.

Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
 hmp.c                         |  26 ++++++++++
 include/migration/migration.h |   1 -
 migration/migration.c         | 116 ++++++++++++++++++++++++++++++++----------
 qapi-schema.json              |  33 ++++++++++--
 qmp-commands.hx               |  14 +++--
 5 files changed, 156 insertions(+), 34 deletions(-)

diff --git a/hmp.c b/hmp.c
index cc2056e..639c3ad 100644
--- a/hmp.c
+++ b/hmp.c
@@ -304,6 +304,12 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, " %s: '%s'",
             MigrationParameter_lookup[MIGRATION_PARAMETER_TLS_HOSTNAME],
             params->tls_hostname ? : "");
+        monitor_printf(mon, " %s: %" PRId64,
+            MigrationParameter_lookup[MIGRATION_PARAMETER_MAX_BANDWIDTH],
+            params->max_bandwidth);
+        monitor_printf(mon, " %s: %" PRId64,
+            MigrationParameter_lookup[MIGRATION_PARAMETER_DOWNTIME_LIMIT],
+            params->downtime_limit);
         monitor_printf(mon, "\n");
     }
 
@@ -1193,6 +1199,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &err);
 }
 
+/* Kept for old-commands compatibility */
 void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict)
 {
     double value = qdict_get_double(qdict, "value");
@@ -1211,6 +1218,7 @@ void hmp_migrate_set_cache_size(Monitor *mon, const QDict *qdict)
     }
 }
 
+/* Kept for old-commands compatibility */
 void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict)
 {
     int64_t value = qdict_get_int(qdict, "value");
@@ -1251,7 +1259,9 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
 {
     const char *param = qdict_get_str(qdict, "parameter");
     const char *valuestr = qdict_get_str(qdict, "value");
+    int64_t valuebw = 0;
     long valueint = 0;
+    char *endp;
     Error *err = NULL;
     bool has_compress_level = false;
     bool has_compress_threads = false;
@@ -1260,6 +1270,8 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
     bool has_cpu_throttle_increment = false;
     bool has_tls_creds = false;
     bool has_tls_hostname = false;
+    bool has_max_bandwidth = false;
+    bool has_downtime_limit = false;
     bool use_int_value = false;
     int i;
 
@@ -1291,6 +1303,18 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
             case MIGRATION_PARAMETER_TLS_HOSTNAME:
                 has_tls_hostname = true;
                 break;
+            case MIGRATION_PARAMETER_MAX_BANDWIDTH:
+                has_max_bandwidth = true;
+                valuebw = qemu_strtosz(valuestr, &endp);
+                if (valuebw < 0 || (size_t)valuebw != valuebw || *endp != '\0') {
+                    error_setg(&err, "Invalid size %s", valuestr);
+                    goto cleanup;
+                }
+                break;
+            case MIGRATION_PARAMETER_DOWNTIME_LIMIT:
+                has_downtime_limit = true;
+                use_int_value = true;
+                break;
             }
 
             if (use_int_value) {
@@ -1308,6 +1332,8 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
                                        has_cpu_throttle_increment, valueint,
                                        has_tls_creds, valuestr,
                                        has_tls_hostname, valuestr,
+                                       has_max_bandwidth, valuebw,
+                                       has_downtime_limit, valueint,
                                        &err);
             break;
         }
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 3c96623..a5429ee 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -129,7 +129,6 @@ struct MigrationSrcPageRequest {
 
 struct MigrationState
 {
-    int64_t bandwidth_limit;
     size_t bytes_xfer;
     size_t xfer_limit;
     QemuThread thread;
diff --git a/migration/migration.c b/migration/migration.c
index 955d5ee..8f620ee 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -44,6 +44,9 @@
 #define BUFFER_DELAY     100
 #define XFER_LIMIT_RATIO (1000 / BUFFER_DELAY)
 
+/* Time in nanoseconds we are allowed to stop the source,
+ * for sending the last part */
+#define DEFAULT_MIGRATE_SET_DOWNTIME 300000000
+
 /* Default compression thread count */
 #define DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT 8
 /* Default decompression thread count, usually decompression is at
@@ -80,7 +83,6 @@ MigrationState *migrate_get_current(void)
     static bool once;
     static MigrationState current_migration = {
         .state = MIGRATION_STATUS_NONE,
-        .bandwidth_limit = MAX_THROTTLE,
         .xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE,
         .mbps = -1,
         .parameters = {
@@ -89,6 +91,8 @@ MigrationState *migrate_get_current(void)
             .decompress_threads = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
             .cpu_throttle_initial = DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL,
             .cpu_throttle_increment = DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT,
+            .max_bandwidth = MAX_THROTTLE,
+            .downtime_limit = DEFAULT_MIGRATE_SET_DOWNTIME,
         },
     };
 
@@ -566,6 +570,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->cpu_throttle_increment = s->parameters.cpu_throttle_increment;
     params->tls_creds = g_strdup(s->parameters.tls_creds);
     params->tls_hostname = g_strdup(s->parameters.tls_hostname);
+    params->max_bandwidth = s->parameters.max_bandwidth;
+    params->downtime_limit = s->parameters.downtime_limit / 1000000;
 
     return params;
 }
@@ -773,6 +779,10 @@ void qmp_migrate_set_parameters(bool has_compress_level,
                                 const char *tls_creds,
                                 bool has_tls_hostname,
                                 const char *tls_hostname,
+                                bool has_max_bandwidth,
+                                int64_t max_bandwidth,
+                                bool has_downtime_limit,
+                                int64_t downtime_limit,
                                 Error **errp)
 {
     MigrationState *s = migrate_get_current();
@@ -808,6 +818,12 @@ void qmp_migrate_set_parameters(bool has_compress_level,
                    "cpu_throttle_increment",
                    "an integer in the range of 1 to 99");
     }
+    if (has_max_bandwidth &&
+            (max_bandwidth < 0 || max_bandwidth > SIZE_MAX)) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                   "max_bandwidth",
+                   "invalid value");
+    }
 
     if (has_compress_level) {
         s->parameters.compress_level = compress_level;
@@ -832,6 +848,21 @@ void qmp_migrate_set_parameters(bool has_compress_level,
         g_free(s->parameters.tls_hostname);
         s->parameters.tls_hostname = g_strdup(tls_hostname);
     }
+    if (has_max_bandwidth) {
+        s->parameters.max_bandwidth = max_bandwidth;
+        if (s->to_dst_file) {
+            qemu_file_set_rate_limit(s->to_dst_file,
+                                s->parameters.max_bandwidth / XFER_LIMIT_RATIO);
+        }
+    }
+    if (has_downtime_limit) {
+        downtime_limit *= 1000000; /* convert to nanoseconds */
+
+        s = migrate_get_current();
+
+        max_downtime = downtime_limit;
+        s->parameters.downtime_limit = max_downtime;
+    }
 }
 
 
@@ -1163,30 +1194,63 @@ int64_t qmp_query_migrate_cache_size(Error **errp)
     return migrate_xbzrle_cache_size();
 }
 
-void qmp_migrate_set_speed(int64_t value, Error **errp)
-{
-    MigrationState *s;
-
-    if (value < 0) {
-        value = 0;
-    }
-    if (value > SIZE_MAX) {
-        value = SIZE_MAX;
-    }
-
-    s = migrate_get_current();
-    s->bandwidth_limit = value;
-    if (s->to_dst_file) {
-        qemu_file_set_rate_limit(s->to_dst_file,
-                                 s->bandwidth_limit / XFER_LIMIT_RATIO);
-    }
-}
-
-void qmp_migrate_set_downtime(double value, Error **errp)
-{
-    value *= 1e9;
-    value = MAX(0, MIN(UINT64_MAX, value));
-    max_downtime = (uint64_t)value;
+void qmp_migrate_set_speed(int64_t valuebw, Error **errp)
+{
+    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 has_max_bandwidth = true;
+    bool has_downtime_limit = false;
+    const char *valuestr = NULL;
+    long valueint = 0;
+    Error *err = NULL;
+
+    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,
+                               has_max_bandwidth, valuebw,
+                               has_downtime_limit, valueint,
+                               &err);
+
+}
+
+void qmp_migrate_set_downtime(double valuedowntime, Error **errp)
+{
+    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 has_max_bandwidth = false;
+    bool has_downtime_limit = true;
+    const char *valuestr = NULL;
+    long valueint = 0;
+    int64_t valuebw = 0;
+    valuedowntime *= 1000; /* Convert to milliseconds */
+    valuedowntime = MAX(0, MIN(INT64_MAX, valuedowntime));
+    Error *err = NULL;
+
+    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,
+                               has_max_bandwidth, valuebw,
+                               has_downtime_limit, valuedowntime,
+                               &err);
 }
 
 bool migrate_postcopy_ram(void)
@@ -1858,7 +1922,7 @@ void migrate_fd_connect(MigrationState *s)
 
     qemu_file_set_blocking(s->to_dst_file, true);
     qemu_file_set_rate_limit(s->to_dst_file,
-                             s->bandwidth_limit / XFER_LIMIT_RATIO);
+                             s->parameters.max_bandwidth / XFER_LIMIT_RATIO);
 
     /* Notify before starting migration thread */
     notifier_list_notify(&migration_state_notifiers, s);
diff --git a/qapi-schema.json b/qapi-schema.json
index 5658723..a8ee2d4 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -637,12 +637,19 @@
 #                hostname must be provided so that the server's x509
 #                certificate identity can be validated. (Since 2.7)
 #
+# @max-bandwidth: to set maximum speed for migration. maximum speed in
+#                 bytes per second. (Since 2.8)
+#
+# @downtime-limit: set maximum tolerated downtime for migration. maximum
+#                  downtime in milliseconds (Since 2.8)
+#
 # Since: 2.4
 ##
 { 'enum': 'MigrationParameter',
   'data': ['compress-level', 'compress-threads', 'decompress-threads',
            'cpu-throttle-initial', 'cpu-throttle-increment',
-           'tls-creds', 'tls-hostname'] }
+           'tls-creds', 'tls-hostname', 'max-bandwidth',
+           'downtime-limit'] }
 
 #
 # @migrate-set-parameters
@@ -678,6 +685,12 @@
 #                hostname must be provided so that the server's x509
 #                certificate identity can be validated. (Since 2.7)
 #
+# @max-bandwidth: to set maximum speed for migration. maximum speed in
+#                 bytes per second. (Since 2.8)
+#
+# @downtime-limit: set maximum tolerated downtime for migration. maximum
+#                  downtime in milliseconds (Since 2.8)
+#
 # Since: 2.4
 ##
 { 'command': 'migrate-set-parameters',
@@ -687,7 +700,9 @@
             '*cpu-throttle-initial': 'int',
             '*cpu-throttle-increment': 'int',
             '*tls-creds': 'str',
-            '*tls-hostname': 'str'} }
+            '*tls-hostname': 'str',
+            '*max-bandwidth': 'int',
+            '*downtime-limit': 'int'} }
 
 #
 # @MigrationParameters
@@ -721,6 +736,12 @@
 #                hostname must be provided so that the server's x509
 #                certificate identity can be validated. (Since 2.7)
 #
+# @max-bandwidth: to set maximum speed for migration. maximum speed in
+#                 bytes per second. (Since 2.8)
+#
+# @downtime-limit: set maximum tolerated downtime for migration. maximum
+#                  downtime in milliseconds (Since 2.8)
+#
 # Since: 2.4
 ##
 { 'struct': 'MigrationParameters',
@@ -730,7 +751,9 @@
             'cpu-throttle-initial': 'int',
             'cpu-throttle-increment': 'int',
             'tls-creds': 'str',
-            'tls-hostname': 'str'} }
+            'tls-hostname': 'str',
+            'max-bandwidth': 'int',
+            'downtime-limit': 'int'} }
 ##
 # @query-migrate-parameters
 #
@@ -1812,6 +1835,8 @@
 #
 # Returns: nothing on success
 #
+# Notes: This command is deprecated in favor of 'migrate-set-parameters'
+#
 # Since: 0.14.0
 ##
 { 'command': 'migrate_set_downtime', 'data': {'value': 'number'} }
@@ -1825,7 +1850,7 @@
 #
 # Returns: nothing on success
 #
-# Notes: A value lesser than zero will be automatically round up to zero.
+# Notes: This command is deprecated in favor of 'migrate-set-parameters'
 #
 # Since: 0.14.0
 ##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6866264..8b050d0 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3790,7 +3790,9 @@ Set migration parameters
                           throttled for auto-converge (json-int)
 - "cpu-throttle-increment": set throttle increasing percentage for
                             auto-converge (json-int)
-
+- "max-bandwidth": set maximum speed for migrations (in bytes) (json-int)
+- "downtime-limit": set maximum tolerated downtime (in milliseconds) for
+                          migrations (json-int)
 Arguments:
 
 Example:
@@ -3803,7 +3805,7 @@ EQMP
     {
         .name       = "migrate-set-parameters",
         .args_type  =
-            "compress-level:i?,compress-threads:i?,decompress-threads:i?,cpu-throttle-initial:i?,cpu-throttle-increment:i?",
+            "compress-level:i?,compress-threads:i?,decompress-threads:i?,cpu-throttle-initial:i?,cpu-throttle-increment:i?,max-bandwidth:i?,downtime-limit:i?",
         .mhandler.cmd_new = qmp_marshal_migrate_set_parameters,
     },
 SQMP
@@ -3820,6 +3822,10 @@ Query current migration parameters
                                     throttled (json-int)
          - "cpu-throttle-increment" : throttle increasing percentage for
                                       auto-converge (json-int)
+         - "max-bandwidth" : maximium migration speed in bytes per second
+                             (json-int)
+         - "downtime-limit" : maximum tolerated downtime of migration in
+                              milliseconds (json-int)
 
 Arguments:
 
@@ -3832,7 +3838,9 @@ Example:
          "cpu-throttle-increment": 10,
          "compress-threads": 8,
          "compress-level": 1,
-         "cpu-throttle-initial": 20
+         "cpu-throttle-initial": 20,
+         "max-bandwidth": 33554432,
+         "downtime-limit": 300
       }
    }
 
-- 
2.6.2

Re: [Qemu-devel] [PATCH v4] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp

[Eric Blake]

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

On 09/08/2016 10:59 AM, Ashijeet Acharya wrote:

Long subject line; we strive to stay below 70 bytes so that you can
prepend commit ids and still stay in an 80-column window.  Also, you can
look at 'git shortlog -30' to get a feel for average length.  Better
might be:

migrate: move max-bandwidth and downtime-limit to migrate_set_parameter

> Mark old-commands for speed and downtime as deprecated.
> Move max-bandwidth and downtime-limit into migrate-set-parameters for
> setting maximum migration speed and expected downtime limit parameters
> respectively.
> Change downtime units to milliseconds (only for new-command) and update
> the query part in both hmp and qmp qemu control interfaces.
> 
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---
>  hmp.c                         |  26 ++++++++++

Between the --- and diffstat is a nice place to tell us how v4 differs
from v3, to focus the attention of reviewers that have already seen this
once.

> +++ b/hmp.c
> @@ -304,6 +304,12 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>          monitor_printf(mon, " %s: '%s'",
>              MigrationParameter_lookup[MIGRATION_PARAMETER_TLS_HOSTNAME],
>              params->tls_hostname ? : "");
> +        monitor_printf(mon, " %s: %" PRId64,
> +            MigrationParameter_lookup[MIGRATION_PARAMETER_MAX_BANDWIDTH],
> +            params->max_bandwidth);

Worth including a unit (bytes/second, right?) in the output here?

> +        monitor_printf(mon, " %s: %" PRId64,
> +            MigrationParameter_lookup[MIGRATION_PARAMETER_DOWNTIME_LIMIT],
> +            params->downtime_limit);

and here (milliseconds, right)? Particularly since the two are on
different scales.

>          monitor_printf(mon, "\n");
>      }
>  
> @@ -1193,6 +1199,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &err);
>  }
>  
> +/* Kept for old-commands compatibility */
>  void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict)

s/old-commands/backwards/

>  {
>      double value = qdict_get_double(qdict, "value");
> @@ -1211,6 +1218,7 @@ void hmp_migrate_set_cache_size(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> +/* Kept for old-commands compatibility */
>  void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict)

and again

> @@ -773,6 +779,10 @@ void qmp_migrate_set_parameters(bool has_compress_level,
>                                  const char *tls_creds,
>                                  bool has_tls_hostname,
>                                  const char *tls_hostname,
> +                                bool has_max_bandwidth,
> +                                int64_t max_bandwidth,
> +                                bool has_downtime_limit,
> +                                int64_t downtime_limit,
>                                  Error **errp)
>  {

> +    if (has_downtime_limit) {
> +        downtime_limit *= 1000000; /* convert to nanoseconds */
> +

Still risks overflow on user-provided input.  You should validate that
the user's input is reasonable, rather than accidentally setting a
too-small limit when the user meant for something so long as to appear
infinite.


> +void qmp_migrate_set_speed(int64_t valuebw, Error **errp)
> +{
> +    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 has_max_bandwidth = true;
> +    bool has_downtime_limit = false;
> +    const char *valuestr = NULL;
> +    long valueint = 0;
> +    Error *err = NULL;
> +
> +    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,
> +                               has_max_bandwidth, valuebw,
> +                               has_downtime_limit, valueint,
> +                               &err);
> +

And you may want to wait for v5 until after I post my patch later today,
so you can see what I meant in the v4 discussion about using the qapi
features for shorter code.


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

Re: [Qemu-devel] [PATCH v4] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp

[Ashijeet Acharya]

On Fri, Sep 9, 2016 at 12:02 AM, Eric Blake <eblake@redhat.com> wrote:
> On 09/08/2016 10:59 AM, Ashijeet Acharya wrote:
>
> Long subject line; we strive to stay below 70 bytes so that you can
> prepend commit ids and still stay in an 80-column window.  Also, you can
> look at 'git shortlog -30' to get a feel for average length.  Better
> might be:
>
> migrate: move max-bandwidth and downtime-limit to migrate_set_parameter

I wasn't aware of that. Will rectify it in v5. Sorry!

>
>> Mark old-commands for speed and downtime as deprecated.
>> Move max-bandwidth and downtime-limit into migrate-set-parameters for
>> setting maximum migration speed and expected downtime limit parameters
>> respectively.
>> Change downtime units to milliseconds (only for new-command) and update
>> the query part in both hmp and qmp qemu control interfaces.
>>
>> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
>> ---
>>  hmp.c                         |  26 ++++++++++
>
> Between the --- and diffstat is a nice place to tell us how v4 differs
> from v3, to focus the attention of reviewers that have already seen this
> once.

Got it. I will make sure I do that next time.

>
>> +++ b/hmp.c
>> @@ -304,6 +304,12 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>>          monitor_printf(mon, " %s: '%s'",
>>              MigrationParameter_lookup[MIGRATION_PARAMETER_TLS_HOSTNAME],
>>              params->tls_hostname ? : "");
>> +        monitor_printf(mon, " %s: %" PRId64,
>> +            MigrationParameter_lookup[MIGRATION_PARAMETER_MAX_BANDWIDTH],
>> +            params->max_bandwidth);
>
> Worth including a unit (bytes/second, right?) in the output here?

Yeah bytes/second. I will make it shorter like 'bps', similar to 'info
migrate' hmp output.

>
>> +        monitor_printf(mon, " %s: %" PRId64,
>> +            MigrationParameter_lookup[MIGRATION_PARAMETER_DOWNTIME_LIMIT],
>> +            params->downtime_limit);
>
> and here (milliseconds, right)? Particularly since the two are on
> different scales.

Yeah, that will make it clearer.

>
>>          monitor_printf(mon, "\n");
>>      }
>>
>> @@ -1193,6 +1199,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
>>      hmp_handle_error(mon, &err);
>>  }
>>
>> +/* Kept for old-commands compatibility */
>>  void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict)
>
> s/old-commands/backwards/

Okay.

>
>>  {
>>      double value = qdict_get_double(qdict, "value");
>> @@ -1211,6 +1218,7 @@ void hmp_migrate_set_cache_size(Monitor *mon, const QDict *qdict)
>>      }
>>  }
>>
>> +/* Kept for old-commands compatibility */
>>  void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict)
>
> and again
>
>> @@ -773,6 +779,10 @@ void qmp_migrate_set_parameters(bool has_compress_level,
>>                                  const char *tls_creds,
>>                                  bool has_tls_hostname,
>>                                  const char *tls_hostname,
>> +                                bool has_max_bandwidth,
>> +                                int64_t max_bandwidth,
>> +                                bool has_downtime_limit,
>> +                                int64_t downtime_limit,
>>                                  Error **errp)
>>  {
>
>> +    if (has_downtime_limit) {
>> +        downtime_limit *= 1000000; /* convert to nanoseconds */
>> +
>
> Still risks overflow on user-provided input.  You should validate that
> the user's input is reasonable, rather than accidentally setting a
> too-small limit when the user meant for something so long as to appear
> infinite.
>

No problem. I will put an error check here.

>
>> +void qmp_migrate_set_speed(int64_t valuebw, Error **errp)
>> +{
>> +    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 has_max_bandwidth = true;
>> +    bool has_downtime_limit = false;
>> +    const char *valuestr = NULL;
>> +    long valueint = 0;
>> +    Error *err = NULL;
>> +
>> +    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,
>> +                               has_max_bandwidth, valuebw,
>> +                               has_downtime_limit, valueint,
>> +                               &err);
>> +
>
> And you may want to wait for v5 until after I post my patch later today,
> so you can see what I meant in the v4 discussion about using the qapi
> features for shorter code.
>

Sure, anything that gets rid of this part and makes it neater will be better.
Mind to include me in the cc list for that one?

Thanks for you help!
Ashijeet
>
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>

Re: [Qemu-devel] [PATCH v4] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp

[Eric Blake]

On 09/08/2016 10:59 AM, Ashijeet Acharya wrote:
> Mark old-commands for speed and downtime as deprecated.
> Move max-bandwidth and downtime-limit into migrate-set-parameters for
> setting maximum migration speed and expected downtime limit parameters
> respectively.
> Change downtime units to milliseconds (only for new-command) and update
> the query part in both hmp and qmp qemu control interfaces.
> 
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---

> +void qmp_migrate_set_speed(int64_t valuebw, Error **errp)
> +{
> +    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 has_max_bandwidth = true;
> +    bool has_downtime_limit = false;
> +    const char *valuestr = NULL;
> +    long valueint = 0;
> +    Error *err = NULL;
> +
> +    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,
> +                               has_max_bandwidth, valuebw,
> +                               has_downtime_limit, valueint,
> +                               &err);
> +
> +}

Oh, and I was so caught up on the grossness of this method that I failed
to overlook that you are silently ignoring and leaking any errors.
Since you aren't making any further decisions, you don't need the 'Error
*err' declaration, and can just pass 'errp' instead of '&err' to
qmp_migrate_set_parameters().  At any rate, I'm also posting my proposal
to make this patch have a simpler back-compat method, by using qapi
boxed parameters of my series.

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

Re: [Qemu-devel] [PATCH v4] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp

[Ashijeet Acharya]

On Fri, Sep 9, 2016 at 8:41 AM, Eric Blake <eblake@redhat.com> wrote:
> On 09/08/2016 10:59 AM, Ashijeet Acharya wrote:
>> Mark old-commands for speed and downtime as deprecated.
>> Move max-bandwidth and downtime-limit into migrate-set-parameters for
>> setting maximum migration speed and expected downtime limit parameters
>> respectively.
>> Change downtime units to milliseconds (only for new-command) and update
>> the query part in both hmp and qmp qemu control interfaces.
>>
>> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
>> ---
>
>> +void qmp_migrate_set_speed(int64_t valuebw, Error **errp)
>> +{
>> +    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 has_max_bandwidth = true;
>> +    bool has_downtime_limit = false;
>> +    const char *valuestr = NULL;
>> +    long valueint = 0;
>> +    Error *err = NULL;
>> +
>> +    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,
>> +                               has_max_bandwidth, valuebw,
>> +                               has_downtime_limit, valueint,
>> +                               &err);
>> +
>> +}
>
> Oh, and I was so caught up on the grossness of this method that I failed
> to overlook that you are silently ignoring and leaking any errors.
> Since you aren't making any further decisions, you don't need the 'Error
> *err' declaration, and can just pass 'errp' instead of '&err' to
> qmp_migrate_set_parameters().  At any rate, I'm also posting my proposal
> to make this patch have a simpler back-compat method, by using qapi
> boxed parameters of my series.
>

Should I send v5 in reply to your series since it uses your new qapi
boxed parameters method or as an independent new thread?

Ashijeet

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