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:

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 <address@hidden>
> ---
>  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

signature.asc
Description: OpenPGP digital signature