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

On 11/22/2010 02:35 PM, Cole Robinson wrote: > > Signed-off-by: Cole Robinson <crobinso(a)redhat.com&gt; > --- > 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.