Re: [libvirt] [PATCHv6 3/7] Support block I/O throttle in XML

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(a)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(a)linux.vnet.ibm.com> >> Signed-off-by: Zhi Yong Wu<wuzhy(a)linux.vnet.ibm.com> >> Signed-off-by: Eric Blake<eblake(a)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: -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list