8:31 a.m.
These patches fix the parsing and the returned error if parsing of vCPUs
failed. If a overflowing number was used for the count, the domain might
disappear after the restart of the daemon.
Peter Krempa (2):
conf: Check if number of vCPUs fits in the storage variable
conf: Improve error messages if parsing of vCPU count fails
src/conf/domain_conf.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
--
1.8.1.1
8:31 a.m.
New subject: [libvirt] [PATCH 1/2] conf: Check if number of vCPUs fits in the storage variable
The count of vCPUs for a domain is extracted as a usingned long variable
but is stored in a unsigned short. If the actual number was too large,
a faulty number was stored.
---
src/conf/domain_conf.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0b9ba13..3e95ec9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9085,7 +9085,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
def->maxvcpus = 1;
} else {
def->maxvcpus = count;
- if (count == 0) {
+ if (count == 0 || (unsigned short) count != count) {
virReportError(VIR_ERR_XML_ERROR,
_("invalid maxvcpus %lu"), count);
goto error;
@@ -9101,7 +9101,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
def->vcpus = def->maxvcpus;
} else {
def->vcpus = count;
- if (count == 0) {
+ if (count == 0 || (unsigned short) count != count) {
virReportError(VIR_ERR_XML_ERROR,
_("invalid current vcpus %lu"), count);
goto error;
--
1.8.1.1
9:07 a.m.
New subject: [libvirt] [PATCH 1/2] conf: Check if number of vCPUs fits in the storage variable
On 01/22/2013 09:31 AM, Peter Krempa wrote:
The count of vCPUs for a domain is extracted as a usingned long
variable
but is stored in a unsigned short. If the actual number was too large,
a faulty number was stored.
---
src/conf/domain_conf.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 0b9ba13..3e95ec9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9085,7 +9085,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
def->maxvcpus = 1;
} else {
def->maxvcpus = count;
- if (count == 0) {
+ if (count == 0 || (unsigned short) count != count) {
maxvcpus is a 'unsigned short' and count is an 'unsigned long', thus if
def->maxvcpus != count after this point, then we have the overflow,
right? Or would the compiler "adjust" that comparison behind our back
on an if check?
virReportError(VIR_ERR_XML_ERROR,
_("invalid maxvcpus %lu"), count);
goto error;
@@ -9101,7 +9101,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
def->vcpus = def->maxvcpus;
} else {
def->vcpus = count;
- if (count == 0) {
+ if (count == 0 || (unsigned short) count != count) {
Same comment as 'maxvcpus'
virReportError(VIR_ERR_XML_ERROR,
_("invalid current vcpus %lu"), count);
goto error;
ACK - I think what you've done is right, although perhaps someone with a
bit more knowledge of what the compiler does could pipe in (I'm curious
too).
John
10:47 a.m.
New subject: [libvirt] [PATCH 1/2] conf: Check if number of vCPUs fits in the storage variable
On 01/22/13 16:07, John Ferlan wrote:
On 01/22/2013 09:31 AM, Peter Krempa wrote:
> The count of vCPUs for a domain is extracted as a usingned long variable
> but is stored in a unsigned short. If the actual number was too large,
> a faulty number was stored.
> ---
> src/conf/domain_conf.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 0b9ba13..3e95ec9 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -9085,7 +9085,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
> def->maxvcpus = 1;
> } else {
> def->maxvcpus = count;
> - if (count == 0) {
> + if (count == 0 || (unsigned short) count != count) {
maxvcpus is a 'unsigned short' and count is an 'unsigned long', thus if
def->maxvcpus != count after this point, then we have the overflow,
right? Or would the compiler "adjust" that comparison behind our back
on an if check?
You mean changing the explicit typecast with checking of def->maxvcpus?
This works in gcc too, but I'm afraid I have -O0 as a default for
libvirt. I'm not sure if a compiler is allowed to optimize such a
comparison, but the explicit typecast is probably safer and it is more
noticeable to a possible future reader of that piece of code.
> virReportError(VIR_ERR_XML_ERROR,
> _("invalid maxvcpus %lu"), count);
> goto error;
> @@ -9101,7 +9101,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
> def->vcpus = def->maxvcpus;
> } else {
> def->vcpus = count;
> - if (count == 0) {
> + if (count == 0 || (unsigned short) count != count) {
Same comment as 'maxvcpus'
> virReportError(VIR_ERR_XML_ERROR,
> _("invalid current vcpus %lu"), count);
> goto error;
>
ACK - I think what you've done is right, although perhaps someone with a
bit more knowledge of what the compiler does could pipe in (I'm curious
too).
For now, I will probably push this as is. We still can fix this in the
future if there will be a cleaner solution.
Thanks for the review.
Peter
John
3:53 a.m.
New subject: [libvirt] [PATCH 1/2] conf: Check if number of vCPUs fits in the storage variable
At Tue, 22 Jan 2013 17:47:36 +0100,
Peter Krempa wrote:
On 01/22/13 16:07, John Ferlan wrote:
> On 01/22/2013 09:31 AM, Peter Krempa wrote:
>> The count of vCPUs for a domain is extracted as a usingned long variable
May be too late, but there's a typo
s/usingned/unsigned
Claudio
--
AV-Test GmbH, Henricistraße 20, 04155 Leipzig, Germany
Phone: +49 341 265 310 19
Web:<http://www.av-test.org>
Eingetragen am / Registered at: Amtsgericht Stendal (HRB 114076)
Geschaeftsfuehrer (CEO): Andreas Marx, Guido Habicht, Maik Morgenstern
6:50 a.m.
New subject: [libvirt] [PATCH 1/2] conf: Check if number of vCPUs fits in the storage variable
On 01/22/2013 08:07 AM, John Ferlan wrote:
On 01/22/2013 09:31 AM, Peter Krempa wrote:
> The count of vCPUs for a domain is extracted as a usingned long variable
> but is stored in a unsigned short. If the actual number was too large,
> a faulty number was stored.
> + if (count == 0 || (unsigned short) count != count) {
maxvcpus is a 'unsigned short' and count is an 'unsigned long', thus if
def->maxvcpus != count after this point, then we have the overflow,
right? Or would the compiler "adjust" that comparison behind our back
on an if check?
For unsigned types, the C standard guarantees that overflow wraps
around, and that casting a larger type down to a smaller type in order
to compare the same number is required to tell you if overflow happened,
at all optimization levels. This code is valid.
For signed types, the C standard says overflow leads to unspecified
behavior, so all bets are off. Thankfully, this isn't dealing with
signed types.
ACK - I think what you've done is right, although perhaps someone
with a
bit more knowledge of what the compiler does could pipe in (I'm curious
too).
I think the ACK stands.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8:31 a.m.
New subject: [libvirt] [PATCH 2/2] conf: Improve error messages if parsing of vCPU count fails
---
src/conf/domain_conf.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3e95ec9..75450ad 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9087,7 +9087,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
def->maxvcpus = count;
if (count == 0 || (unsigned short) count != count) {
virReportError(VIR_ERR_XML_ERROR,
- _("invalid maxvcpus %lu"), count);
+ _("invalid maximum number of vCPUs '%lu'"),
count);
goto error;
}
}
@@ -9103,13 +9103,14 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
def->vcpus = count;
if (count == 0 || (unsigned short) count != count) {
virReportError(VIR_ERR_XML_ERROR,
- _("invalid current vcpus %lu"), count);
+ _("invalid current number of vCPUs ;%lu'"),
count);
goto error;
}
if (def->maxvcpus < count) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("maxvcpus must not be less than current vcpus (%d <
%lu)"),
+ _("maxvcpus must not be less than current vcpus "
+ "(%d < %lu)"),
def->maxvcpus, count);
goto error;
}
--
1.8.1.1
9:07 a.m.
New subject: [libvirt] [PATCH 2/2] conf: Improve error messages if parsing of vCPU count fails
On 01/22/2013 09:31 AM, Peter Krempa wrote:
---
src/conf/domain_conf.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3e95ec9..75450ad 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9087,7 +9087,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
def->maxvcpus = count;
if (count == 0 || (unsigned short) count != count) {
virReportError(VIR_ERR_XML_ERROR,
- _("invalid maxvcpus %lu"), count);
+ _("invalid maximum number of vCPUs '%lu'"),
count);
goto error;
}
}
@@ -9103,13 +9103,14 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
def->vcpus = count;
if (count == 0 || (unsigned short) count != count) {
virReportError(VIR_ERR_XML_ERROR,
- _("invalid current vcpus %lu"), count);
+ _("invalid current number of vCPUs ;%lu'"),
count);
s/;%lu/'%lu
goto error;
}
if (def->maxvcpus < count) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("maxvcpus must not be less than current vcpus (%d <
%lu)"),
+ _("maxvcpus must not be less than current vcpus "
+ "(%d < %lu)"),
def->maxvcpus, count);
goto error;
}
ACK with the change
11:04 a.m.
New subject: [libvirt] [PATCH 2/2] conf: Improve error messages if parsing of vCPU count fails
On 01/22/13 16:07, John Ferlan wrote:
On 01/22/2013 09:31 AM, Peter Krempa wrote:
> ---
> src/conf/domain_conf.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 3e95ec9..75450ad 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -9087,7 +9087,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
> def->maxvcpus = count;
> if (count == 0 || (unsigned short) count != count) {
> virReportError(VIR_ERR_XML_ERROR,
> - _("invalid maxvcpus %lu"), count);
> + _("invalid maximum number of vCPUs
'%lu'"), count);
> goto error;
> }
> }
> @@ -9103,13 +9103,14 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
> def->vcpus = count;
> if (count == 0 || (unsigned short) count != count) {
> virReportError(VIR_ERR_XML_ERROR,
> - _("invalid current vcpus %lu"), count);
> + _("invalid current number of vCPUs ;%lu'"),
count);
s/;%lu/'%lu
> goto error;
> }
>
> if (def->maxvcpus < count) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("maxvcpus must not be less than current vcpus (%d
< %lu)"),
> + _("maxvcpus must not be less than current vcpus
"
> + "(%d < %lu)"),
> def->maxvcpus, count);
> goto error;
> }
>
ACK with the change
Bah, right. Thanks for the review.
Peter
4321
days inactive
4322
days old
8 comments
4 participants
participants (4)
-
Claudio Bley -
Eric Blake -
John Ferlan -
Peter Krempa