3:17 a.m.
According to docs/schemas/domaincommon.rng and _virDomainBlockIoTuneInfo
all the iotune values are interpreted as unsigned long long, however
according to qemu_monitor_json.c, qemu silently truncates numbers
larger than LLONG_MAX. There's really not much of a usage for such
large numbers anyway yet. This patch provides the same overflow
check during domain XML parsing phase as it does during setting
a blkdeviotune element in qemu_driver.c and thus reports an error when
a larger number than LLONG_MAX is detected.
---
src/conf/domain_conf.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 22a7f7e..4cc900f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5711,6 +5711,18 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
def->blkdeviotune.write_iops_sec = 0;
}
+ if (def->blkdeviotune.total_bytes_sec > LLONG_MAX ||
+ def->blkdeviotune.read_bytes_sec > LLONG_MAX ||
+ def->blkdeviotune.write_bytes_sec > LLONG_MAX ||
+ def->blkdeviotune.total_iops_sec > LLONG_MAX ||
+ def->blkdeviotune.read_iops_sec > LLONG_MAX ||
+ def->blkdeviotune.write_iops_sec > LLONG_MAX) {
+ virReportError(VIR_ERR_OVERFLOW,
+ _("block I/O throttle limit must "
+ "be less than %llu"), LLONG_MAX);
+ goto error;
+ }
+
if ((def->blkdeviotune.total_bytes_sec &&
def->blkdeviotune.read_bytes_sec) ||
(def->blkdeviotune.total_bytes_sec &&
--
1.9.3
4:46 a.m.
On Tue, Aug 26, 2014 at 10:17:22AM +0200, Erik Skultety wrote:
According to docs/schemas/domaincommon.rng and
_virDomainBlockIoTuneInfo
all the iotune values are interpreted as unsigned long long, however
according to qemu_monitor_json.c, qemu silently truncates numbers
larger than LLONG_MAX. There's really not much of a usage for such
large numbers anyway yet. This patch provides the same overflow
check during domain XML parsing phase as it does during setting
a blkdeviotune element in qemu_driver.c and thus reports an error when
a larger number than LLONG_MAX is detected.
---
src/conf/domain_conf.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 22a7f7e..4cc900f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5711,6 +5711,18 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
def->blkdeviotune.write_iops_sec = 0;
}
+ if (def->blkdeviotune.total_bytes_sec > LLONG_MAX ||
+ def->blkdeviotune.read_bytes_sec > LLONG_MAX ||
+ def->blkdeviotune.write_bytes_sec > LLONG_MAX ||
+ def->blkdeviotune.total_iops_sec > LLONG_MAX ||
+ def->blkdeviotune.read_iops_sec > LLONG_MAX ||
+ def->blkdeviotune.write_iops_sec > LLONG_MAX) {
+ virReportError(VIR_ERR_OVERFLOW,
+ _("block I/O throttle limit must "
+ "be less than %llu"), LLONG_MAX);
+ goto error;
+ }
+
if ((def->blkdeviotune.total_bytes_sec &&
def->blkdeviotune.read_bytes_sec) ||
(def->blkdeviotune.total_bytes_sec &&
--
1.9.3
Oh, c**p, so if someone has that in the XML, then such domain will
disappear with the upgrade of libvirt daemon, right.
We should instead put this in the starting phase (and any API that
touches that data, hotplug, blkdeviotune, etc.), so such domains don't
disappear. Fortunately that makes sense, too, because only QEMU
truncates the value, but in case any other hypervisor may accept the
values properly (in the future for example).
Test case in tests/qemuxml2argvtest.c could be nice way to show that
this is fixed ;)
Martin
6:28 a.m.
Thank you very much for review. You're right, except for the part about
adding a test case I think, it's fine to do it locally, but you can't
predict data type sizes on all architectures, so in terms of creating a
static XML description, you can't rely on a specific number being
long/short enough to pass/fail the test.
On 08/26/2014 11:46 AM, Martin Kletzander wrote:
On Tue, Aug 26, 2014 at 10:17:22AM +0200, Erik Skultety wrote:
> According to docs/schemas/domaincommon.rng and _virDomainBlockIoTuneInfo
> all the iotune values are interpreted as unsigned long long, however
> according to qemu_monitor_json.c, qemu silently truncates numbers
> larger than LLONG_MAX. There's really not much of a usage for such
> large numbers anyway yet. This patch provides the same overflow
> check during domain XML parsing phase as it does during setting
> a blkdeviotune element in qemu_driver.c and thus reports an error when
> a larger number than LLONG_MAX is detected.
> ---
> src/conf/domain_conf.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 22a7f7e..4cc900f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5711,6 +5711,18 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr
> xmlopt,
> def->blkdeviotune.write_iops_sec = 0;
> }
>
> + if (def->blkdeviotune.total_bytes_sec > LLONG_MAX ||
> + def->blkdeviotune.read_bytes_sec > LLONG_MAX ||
> + def->blkdeviotune.write_bytes_sec > LLONG_MAX ||
> + def->blkdeviotune.total_iops_sec > LLONG_MAX ||
> + def->blkdeviotune.read_iops_sec > LLONG_MAX ||
> + def->blkdeviotune.write_iops_sec > LLONG_MAX) {
> + virReportError(VIR_ERR_OVERFLOW,
> + _("block I/O throttle limit must "
> + "be less than %llu"), LLONG_MAX);
> + goto error;
> + }
> +
> if ((def->blkdeviotune.total_bytes_sec &&
> def->blkdeviotune.read_bytes_sec) ||
> (def->blkdeviotune.total_bytes_sec &&
> --
> 1.9.3
>
Oh, c**p, so if someone has that in the XML, then such domain will
disappear with the upgrade of libvirt daemon, right.
We should instead put this in the starting phase (and any API that
touches that data, hotplug, blkdeviotune, etc.), so such domains don't
disappear. Fortunately that makes sense, too, because only QEMU
truncates the value, but in case any other hypervisor may accept the
values properly (in the future for example).
Test case in tests/qemuxml2argvtest.c could be nice way to show that
this is fixed ;)
Martin
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
3741
days inactive
3741
days old
2 comments
2 participants
participants (2)
-
Erik Skultety -
Martin Kletzander