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

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
>