9:01 a.m.
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:
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org