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

Re: [libvirt] [PATCH] blkdeviotune: check for overflow when parsing XML

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 &&

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 ;)


Attachment: signature.asc
Description: Digital signature

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