Re: [Qemu-devel] [PATCH v3] Move max-bandwidth and downtime-limit into migrate_set_parameter for both hmp and qmp

[Juan Quintela <quintela@redhat.com>]

Eric Blake <eblake@redhat.com> wrote:

>> +    if (has_max_bandwidth) {
>> +        s->parameters.max_bandwidth = max_bandwidth;
>> +        if (s->to_dst_file) {
>> +            qemu_file_set_rate_limit(s->to_dst_file,
>> +                                s->parameters.max_bandwidth / XFER_LIMIT_RATIO);
>> +        }
>> +    }
>> +    if (has_downtime_limit) {
>> +        downtime_limit *= 1000000; /* convert to nanoseconds */
>
> Are you sure this won't overflow?

Ashijeet, Eric mere means that if downtime_limit is bigger that
INT64_MAX/1000000, then you get an overflow with the multiplication.
Notice that if it overflows, the value is already quite big O:-)

2^63/1000/1000/1000/3600/24/365
292.47

Allowing "only" 292 years of downtime should be enough for the time
being (famous last words) O:-)


>
>> +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,
>
> Ugg. This looks gross.  No need to name a bunch of variables set to
> false, when you can instead use QAPI's boxed conventions to pass a
> pointer to a struct, and rely on 0-initialization of the struct to leave
> all the parameters that you don't care about unmentioned.

We should change qmp_migrate_set_parameters to your new api.  I fully
agree that this is gross, but it is the way it was written, nothing to
do with this patch, really.

Ashijeet, if you want to do this change in a different patch before this
change, I am all for it, but that is independent of your change.

With all the other suggestions of Eric, I agree, or I don't care.
(If time is an int in milliseconds or a float, I don't really care one
way or another.  Whatever libvirt preffers).

Thanks, Juan.