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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Markus Armbruster (armbru@redhat.com) wrote:
> 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 <armbru@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hmp.c | 78 ++++++++++++++++++++++++++++---------------------------------------
>  1 file changed, 32 insertions(+), 46 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index 6d32c40..2993586 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1509,90 +1509,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;
>          }
>      }
> @@ -1602,6 +1587,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
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK