[libvirt] [PATCH 3/6] conf: domain: Improve vcpus validation reporting

[Cole Robinson]

On 11/23/2010 11:49 AM, Eric Blake wrote:
> On 11/22/2010 02:35 PM, Cole Robinson wrote:
>>
>> Signed-off-by: Cole Robinson <crobinso at redhat.com>
>> ---
>>  src/conf/domain_conf.c |   11 +++++++++--
>>  1 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 11a6280..045934d 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -4569,7 +4569,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>>          def->maxvcpus = 1;
>>      } else {
>>          def->maxvcpus = count;
>> -        if (def->maxvcpus != count || count == 0) {
>> +        if (count == 0) {
> 
> At first glance, I was about to complain: Since def->maxvcpus is an
> unsigned short but count is an int, someone calling setvcpus 0x10001
> will silently overflow and end up setting def->maxvcpus == 1.  In other
> words, you just deleted the 'def->maxvcpus != count' overflow check...
> 
>>              virDomainReportError(VIR_ERR_XML_ERROR,
>>                                   _("invalid maxvcpus %lu"), count);
>>              goto error;
>> @@ -4585,11 +4585,18 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>>          def->vcpus = def->maxvcpus;
>>      } else {
>>          def->vcpus = count;
>> -        if (def->vcpus != count || count == 0 || def->maxvcpus < count) {
>> +        if (count == 0) {
>>              virDomainReportError(VIR_ERR_XML_ERROR,
>>                                   _("invalid current vcpus %lu"), count);
>>              goto error;
>>          }
>> +
>> +        if (def->maxvcpus < count) {
> 
> ...but this new code is an equally effective overflow check.  No
> complaint after all; def is local, so it doesn't matter if we changed
> def->maxvcpus to an invalid value before detecting overflow.  Thanks for
> cleaning this up for me.
> 

Ahh, I didn't realize that check was for overflow, I thought it was
unintentional redundancy :/ Glad it worked out okay in the end!

Thanks,
Cole