[RFC,18/56] migration: Make parameter max-bandwidth unsigned in QAPI/QMP

Commit Message

Markus Armbruster Aug. 7, 2017, 2:45 p.m. UTC

Byte rates should use QAPI type 'size' (uint64_t).
migrate_set_speed's parameter @value and member @max-bandwidth of
MigrationParameters and MigrateSetParameters are 'int' (int64_t).

Change them all to 'size'.

migrate_set_speed and migrate-set-parameters now accept bandwidth
values between 2^63 and SIZE_MAX (commonly 2^64-1).  They accept
negative values as before, because that's how the QObject input
visitor works for backward compatibility.

So does HMP's migrate_set_speed, except it continues to reject
negative values.

query-migrate-parameters now reports bandwidth values above 2^63-1
correctly instead of their (negative) two's complement.

So does HMP's "info migrate_params".

Signed-off-by: Markus Armbruster <[e-mail]>
---
 hmp.c                 | 2 +-
 migration/migration.c | 9 ++++-----
 qapi-schema.json      | 6 +++---
 3 files changed, 8 insertions(+), 9 deletions(-)

Comments

Juan Quintela Aug. 7, 2017, 4:50 p.m. UTC | #1

Markus Armbruster <[e-mail]> wrote:
> Byte rates should use QAPI type 'size' (uint64_t).
> migrate_set_speed's parameter @value and member @max-bandwidth of
> MigrationParameters and MigrateSetParameters are 'int' (int64_t).
>
> Change them all to 'size'.
>
> migrate_set_speed and migrate-set-parameters now accept bandwidth
> values between 2^63 and SIZE_MAX (commonly 2^64-1).  They accept
> negative values as before, because that's how the QObject input
> visitor works for backward compatibility.
>
> So does HMP's migrate_set_speed, except it continues to reject
> negative values.
>
> query-migrate-parameters now reports bandwidth values above 2^63-1
> correctly instead of their (negative) two's complement.
>
> So does HMP's "info migrate_params".
>
> Signed-off-by: Markus Armbruster <[e-mail]>

Reviewed-by: Juan Quintela <[e-mail]>

> -    if (params->has_max_bandwidth &&
> -        (params->max_bandwidth < 0 || params->max_bandwidth > SIZE_MAX)) {
> +    if (params->has_max_bandwidth && params->max_bandwidth > SIZE_MAX) {


Much nicer for something that we know can't be negative O:-)

Patch

diff --git a/hmp.c b/hmp.c
index 184fb8b..9bcdcb3 100644
--- a/hmp.c
+++ b/hmp.c
@@ -322,7 +322,7 @@  void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
             MigrationParameter_lookup[MIGRATION_PARAMETER_TLS_HOSTNAME],
             params->tls_hostname);
         assert(params->has_max_bandwidth);
-        monitor_printf(mon, "%s: %" PRId64 " bytes/second\n",
+        monitor_printf(mon, "%s: %" PRIu64 " bytes/second\n",
             MigrationParameter_lookup[MIGRATION_PARAMETER_MAX_BANDWIDTH],
             params->max_bandwidth);
         assert(params->has_downtime_limit);
diff --git a/migration/migration.c b/migration/migration.c
index 2d7f3a2..0b47371 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -716,8 +716,7 @@  static bool migrate_params_check(MigrationParameters *params, Error **errp)
         return false;
     }
 
-    if (params->has_max_bandwidth &&
-        (params->max_bandwidth < 0 || params->max_bandwidth > SIZE_MAX)) {
+    if (params->has_max_bandwidth && params->max_bandwidth > SIZE_MAX) {
         error_setg(errp, "Parameter 'max_bandwidth' expects an integer in the"
                          " range of 0 to %zu bytes/second", SIZE_MAX);
         return false;
@@ -1311,7 +1310,7 @@  uint64_t qmp_query_migrate_cache_size(Error **errp)
     return migrate_xbzrle_cache_size();
 }
 
-void qmp_migrate_set_speed(int64_t value, Error **errp)
+void qmp_migrate_set_speed(uint64_t value, Error **errp)
 {
     MigrateSetParameters p = {
         .has_max_bandwidth = true,
@@ -2179,8 +2178,8 @@  static Property migration_properties[] = {
     DEFINE_PROP_INT64("x-cpu-throttle-increment", MigrationState,
                       parameters.cpu_throttle_increment,
                       DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT),
-    DEFINE_PROP_INT64("x-max-bandwidth", MigrationState,
-                      parameters.max_bandwidth, MAX_THROTTLE),
+    DEFINE_PROP_UINT64("x-max-bandwidth", MigrationState,
+                       parameters.max_bandwidth, MAX_THROTTLE),
     DEFINE_PROP_INT64("x-downtime-limit", MigrationState,
                       parameters.downtime_limit,
                       DEFAULT_MIGRATE_SET_DOWNTIME),
diff --git a/qapi-schema.json b/qapi-schema.json
index 2eee676..c18e574 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1116,7 +1116,7 @@ 
             '*cpu-throttle-increment': 'int',
             '*tls-creds': 'StrOrNull',
             '*tls-hostname': 'StrOrNull',
-            '*max-bandwidth': 'int',
+            '*max-bandwidth': 'size',
             '*downtime-limit': 'int',
             '*x-checkpoint-delay': 'int',
             '*block-incremental': 'bool' } }
@@ -1200,7 +1200,7 @@ 
             '*cpu-throttle-increment': 'int',
             '*tls-creds': 'str',
             '*tls-hostname': 'str',
-            '*max-bandwidth': 'int',
+            '*max-bandwidth': 'size',
             '*downtime-limit': 'int',
             '*x-checkpoint-delay': 'int',
             '*block-incremental': 'bool' } }
@@ -2852,7 +2852,7 @@ 
 # <- { "return": {} }
 #
 ##
-{ 'command': 'migrate_set_speed', 'data': {'value': 'int'} }
+{ 'command': 'migrate_set_speed', 'data': {'value': 'size'} }
 
 ##
 # @migrate-set-cache-size: