[Qemu-devel] [PATCH v2 06/10] hmp: Clean up and simplify hmp_migrate_set_parameter()

[Markus Armbruster]

The bulk of hmp_migrate_set_parameter()'s code sets one member of
MigrationParameters according to the command's arguments.  It uses a
string visitor for integer and boolean members, but not for string and
size members.  It calls visit_type_bool() right away, but delays
visit_type_int() some.  The delaying requires a flag variable and a
bit of trickery: we set all integer members instead of just the one we
want, and rely on the has_FOOs to mask the unwanted ones.

Clean this up as follows.  Don't delay calling visit_type_int().  Use
the string visitor for strings, too.  This involves extra allocations
and cleanup, but doing them is simpler and cleaner than avoiding them.

Sadly, using the string visitor for sizes isn't possible, because it
defaults to Bytes rather than Mebibytes.  Add a comment explaining
that.

Signed-off-by: Markus Armbruster <address@hidden>
Reviewed-by: Daniel P. Berrange <address@hidden>
Reviewed-by: Dr. David Alan Gilbert <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
---
 hmp.c | 78 ++++++++++++++++++++++++++++---------------------------------------
 1 file changed, 32 insertions(+), 46 deletions(-)

diff --git a/hmp.c b/hmp.c
index bf1de74..dae4604 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1554,90 +1554,75 @@ 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");
     Visitor *v = string_input_visitor_new(valuestr);
+    MigrationParameters *p = g_new0(MigrationParameters, 1);
     uint64_t valuebw = 0;
-    int64_t valueint = 0;
-    bool valuebool = false;
     Error *err = NULL;
-    bool use_int_value = false;
     int i, ret;
 
     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:
-                p.has_compress_level = true;
-                use_int_value = true;
+                p->has_compress_level = true;
+                visit_type_int(v, param, &p->compress_level, &err);
                 break;
             case MIGRATION_PARAMETER_COMPRESS_THREADS:
-                p.has_compress_threads = true;
-                use_int_value = true;
+                p->has_compress_threads = true;
+                visit_type_int(v, param, &p->compress_threads, &err);
                 break;
             case MIGRATION_PARAMETER_DECOMPRESS_THREADS:
-                p.has_decompress_threads = true;
-                use_int_value = true;
+                p->has_decompress_threads = true;
+                visit_type_int(v, param, &p->decompress_threads, &err);
                 break;
             case MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL:
-                p.has_cpu_throttle_initial = true;
-                use_int_value = true;
+                p->has_cpu_throttle_initial = true;
+                visit_type_int(v, param, &p->cpu_throttle_initial, &err);
                 break;
             case MIGRATION_PARAMETER_CPU_THROTTLE_INCREMENT:
-                p.has_cpu_throttle_increment = true;
-                use_int_value = true;
+                p->has_cpu_throttle_increment = true;
+                visit_type_int(v, param, &p->cpu_throttle_increment, &err);
                 break;
             case MIGRATION_PARAMETER_TLS_CREDS:
-                p.has_tls_creds = true;
-                p.tls_creds = (char *) valuestr;
+                p->has_tls_creds = true;
+                visit_type_str(v, param, &p->tls_creds, &err);
                 break;
             case MIGRATION_PARAMETER_TLS_HOSTNAME:
-                p.has_tls_hostname = true;
-                p.tls_hostname = (char *) valuestr;
+                p->has_tls_hostname = true;
+                visit_type_str(v, param, &p->tls_hostname, &err);
                 break;
             case MIGRATION_PARAMETER_MAX_BANDWIDTH:
-                p.has_max_bandwidth = true;
+                p->has_max_bandwidth = true;
+                /*
+                 * Can't use visit_type_size() here, because it
+                 * defaults to Bytes rather than Mebibytes.
+                 */
                 ret = qemu_strtosz_MiB(valuestr, NULL, &valuebw);
                 if (ret < 0 || valuebw > INT64_MAX
                     || (size_t)valuebw != valuebw) {
                     error_setg(&err, "Invalid size %s", valuestr);
-                    goto cleanup;
+                    break;
                 }
-                p.max_bandwidth = valuebw;
+                p->max_bandwidth = valuebw;
                 break;
             case MIGRATION_PARAMETER_DOWNTIME_LIMIT:
-                p.has_downtime_limit = true;
-                use_int_value = true;
+                p->has_downtime_limit = true;
+                visit_type_int(v, param, &p->downtime_limit, &err);
                 break;
             case MIGRATION_PARAMETER_X_CHECKPOINT_DELAY:
-                p.has_x_checkpoint_delay = true;
-                use_int_value = true;
+                p->has_x_checkpoint_delay = true;
+                visit_type_int(v, param, &p->x_checkpoint_delay, &err);
                 break;
             case MIGRATION_PARAMETER_BLOCK_INCREMENTAL:
-                p.has_block_incremental = true;
-                visit_type_bool(v, param, &valuebool, &err);
-                if (err) {
-                    goto cleanup;
-                }
-                p.block_incremental = valuebool;
+                p->has_block_incremental = true;
+                visit_type_bool(v, param, &p->block_incremental, &err);
                 break;
             }
 
-            if (use_int_value) {
-                visit_type_int(v, param, &valueint, &err);
-                if (err) {
-                    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;
-                p.downtime_limit = valueint;
-                p.x_checkpoint_delay = valueint;
+            if (err) {
+                goto cleanup;
             }
 
-            qmp_migrate_set_parameters(&p, &err);
+            qmp_migrate_set_parameters(p, &err);
             break;
         }
     }
@@ -1647,6 +1632,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict 
*qdict)
     }
 
  cleanup:
+    qapi_free_MigrationParameters(p);
     visit_free(v);
     if (err) {
         error_report_err(err);
-- 
2.7.5