On 09/08/2016 10:30 AM, Ashijeet Acharya wrote:

>>>> +    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. :-)

But it still means we should explicitly check that whatever number the
user inputs is not wrapping around to a smaller actual downtime -
especially since wraparound to a small number CAN be observed within a
human lifespan, for some values of wraparound.  Either give the user an
error (their input was too big; preferred) or silently convert the
user's too-large input to the maximum possible (they won't live long
enough to notice the difference, not ideal, but if reporting an error is
too hard, it is workable).


>>>> +
>>>> +    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.

Okay, I'll take the hint and submit a patch to convert migration to use
the new boxed parameters from QAPI.  It's an independent change, but you
would then rebase your series on top of my patch (or if yours lands
first, my work would remove the additions in yours, for some churn in
the code base, but no real harm done).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org