Re: [Patch V2] Fix initialization of current vcpus in libxl driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Eric Blake wrote:
> On 05/24/2011 11:22 AM, Jim Fehlig wrote:
>   
>> The cur_vcpus member of struct libxl_domain_build_info was incorrectly
>> initialized to the number of vcpus, when it should have been interpreted
>> as a bitmap, where bit X corresponds to online/offline status of vcpuX.
>>
>> To complicate matters, cur_vcpus is an int, so only 32 vcpus can be
>> set online.  Add a check to ensure vcpus does not exceed this limit.
>>
>> V2: Eric Blake noted a compilation pitfal when '1 << 32' on an int.
>>     
>
> If you keep the V2 message in your commit log, then s/pitfal/pitfall/
> (personally, I tend to drop V2 messages when pushing - they are handy
> during review to point out what to look for, but once it is part of
> history, I no longer care how many versions it took to get to the final
> pushed version).
>
>   
>>     Account for vcpus == 32.
>>     
>
>   
>> +     * as an int, but its' semantic is a bitmap of online vcpus, so
>>     
>
> s/its'/its/
>
> (That is, drop the spurious apostrophe).
>
>
>   
>>      b_info->hvm = hvm;
>>      b_info->max_vcpus = def->maxvcpus;
>> -    b_info->cur_vcpus = def->vcpus;
>> +    if (def->vcpus == 32)
>> +        b_info->cur_vcpus = 1 << 31;
>>     
>
> Still wrong.  You want 32 bits, not one bit:
>
> b_info->cur_vcpus = (uint32_t) -1;
>   

Err, right - I should address _all_ previous comments.

>   
>> +    else
>> +        b_info->cur_vcpus = (1 << def->vcpus) - 1;
>>      if (def->clock.ntimers > 0 &&
>>          def->clock.timers[0]->name == VIR_DOMAIN_TIMER_NAME_TSC) {
>>          switch (def->clock.timers[0]->mode) {
>>     
>
> ACK with those changes.
>   

And this time I have.  Changes made and pushed.

Thanks,
Jim

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]