On 11/24/2011 12:54 AM, Daniel Veillard wrote:
On Wed, Nov 23, 2011 at 02:44:45PM -0700, Eric Blake wrote:
From: Lei Li <lilei@linux.vnet.ibm.com>
Enable block I/O throttle for per-disk in XML, as the first per-disk IO tuning parameter.
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> Signed-off-by: Eric Blake <eblake@redhat.com> [...]
This code is buggy I'm afraid. It's supposed to apply to virDomainDiskDefParseXML() which will go down in the tree searching for the informations on a given disk. By using an XPath query on the passed context without updating it with the current node, you go back to the domain root search and scan the fulls set of devices for iotune parameter, then if there is more than one disk with such param you will *concatenate* the string and get an erroneous value. 2 ways to fix this: - reset the node from ctxt to the current node of the disk parsed and use "iotune/total_bytes_sec" expressions (may also fit in the 80 char line ...)
We've done this elsewhere, so it's pretty easy to copy.
ACK, once fixed.
Here's what I'm squashing in. I still have to actually test response on qemu, though, before I'm comfortable pushing (it may be that the code as-is, while passing its self-test on xml handling, fails to gracefully handle old qemu that lacks support for this feature). diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c index a2702c5..caf4cf5 100644 --- i/src/conf/domain_conf.c +++ w/src/conf/domain_conf.c @@ -2400,6 +2400,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, { virDomainDiskDefPtr def; xmlNodePtr cur, child; + xmlNodePtr save_ctxt = ctxt->node; char *type = NULL; char *device = NULL; char *snapshot = NULL; @@ -2431,6 +2432,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, return NULL; } + ctxt->node = node; + type = virXMLPropString(node, "type"); if (type) { if ((def->type = virDomainDiskTypeFromString(type)) < 0) { @@ -2596,47 +2599,59 @@ virDomainDiskDefParseXML(virCapsPtr caps, child = child->next; } } else if (xmlStrEqual(cur->name, BAD_CAST "iotune")) { - if (virXPathULongLong("string(./devices/disk/iotune/total_bytes_sec)", - ctxt, &def->blkdeviotune.total_bytes_sec) < 0) { + if (virXPathULongLong("string(./iotune/total_bytes_sec)", + ctxt, + &def->blkdeviotune.total_bytes_sec) < 0) { def->blkdeviotune.total_bytes_sec = 0; } - if (virXPathULongLong("string(./devices/disk/iotune/read_bytes_sec)", - ctxt, &def->blkdeviotune.read_bytes_sec) < 0) { + if (virXPathULongLong("string(./iotune/read_bytes_sec)", + ctxt, + &def->blkdeviotune.read_bytes_sec) < 0) { def->blkdeviotune.read_bytes_sec = 0; } - if (virXPathULongLong("string(./devices/disk/iotune/write_bytes_sec)", - ctxt, &def->blkdeviotune.write_bytes_sec) < 0) { + if (virXPathULongLong("string(./iotune/write_bytes_sec)", + ctxt, + &def->blkdeviotune.write_bytes_sec) < 0) { def->blkdeviotune.write_bytes_sec = 0; } - if (virXPathULongLong("string(./devices/disk/iotune/total_iops_sec)", - ctxt, &def->blkdeviotune.total_iops_sec) < 0) { + if (virXPathULongLong("string(./iotune/total_iops_sec)", + ctxt, + &def->blkdeviotune.total_iops_sec) < 0) { def->blkdeviotune.total_iops_sec = 0; } - if (virXPathULongLong("string(./devices/disk/iotune/read_iops_sec)", - ctxt, &def->blkdeviotune.read_iops_sec) < 0) { + if (virXPathULongLong("string(./iotune/read_iops_sec)", + ctxt, + &def->blkdeviotune.read_iops_sec) < 0) { def->blkdeviotune.read_iops_sec = 0; } - if (virXPathULongLong("string(./devices/disk/iotune/write_iops_sec)", - ctxt, &def->blkdeviotune.write_iops_sec) < 0) { + if (virXPathULongLong("string(./iotune/write_iops_sec)", + ctxt, + &def->blkdeviotune.write_iops_sec) < 0) { def->blkdeviotune.write_iops_sec = 0; } - if ((def->blkdeviotune.total_bytes_sec && def->blkdeviotune.read_bytes_sec) - || (def->blkdeviotune.total_bytes_sec && def->blkdeviotune.write_bytes_sec)) { + if ((def->blkdeviotune.total_bytes_sec && + def->blkdeviotune.read_bytes_sec) || + (def->blkdeviotune.total_bytes_sec && + def->blkdeviotune.write_bytes_sec)) { virDomainReportError(VIR_ERR_XML_ERROR, - _("total and read/write bytes_sec cannot be set at the same time")); + _("total and read/write bytes_sec " + "cannot be set at the same time")); goto error; } - if ((def->blkdeviotune.total_iops_sec && def->blkdeviotune.read_iops_sec) - || (def->blkdeviotune.total_iops_sec && def->blkdeviotune.write_iops_sec)) { + if ((def->blkdeviotune.total_iops_sec && + def->blkdeviotune.read_iops_sec) || + (def->blkdeviotune.total_iops_sec && + def->blkdeviotune.write_iops_sec)) { virDomainReportError(VIR_ERR_XML_ERROR, - _("total and read/write iops_sec cannot be set at the same time")); + _("total and read/write iops_sec " + "cannot be set at the same time")); goto error; } } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) { @@ -2933,6 +2948,7 @@ cleanup: virStorageEncryptionFree(encryption); VIR_FREE(startupPolicy); + ctxt->node = save_ctxt; return def; no_memory: -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org