On Fri, Sep 9, 2016 at 12:02 AM, Eric Blake <ebl...@redhat.com> 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 <ashijeetacha...@gmail.com>
>> ---
>> 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
>
