On Thu, Sep 8, 2016 at 8:11 PM, Juan Quintela <quint...@redhat.com> wrote:
> Eric Blake <ebl...@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.
Oh I get it now.
> 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:-)
>
Haha! 292 years seems sufficient. :-)
>
>>
>>> +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.
Yeah, you expressed your disgust about this in your 'to-do list of
migration mail' too I remember.
>
> 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.
I can try, but I am not sure about it as I am very new to QAPI's methods.
Its better if I embarked upon such a task later with more knowledge of QAPI.
> 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).
I am keeping it to milliseconds for now then!
Juan, do not apply the patch just yet. Let me rectify the small
mistakes regarding grammatical errors in comments and send the updated
patch in a few. Accept it after that.
Thanks
Ashijeet
>
> Thanks, Juan.
