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@xxxxxxxxxxxxxxxxxx>
>>
>> Enable block I/O throttle for per-disk in XML, as the first
>> per-disk IO tuning parameter.
>>
>> Signed-off-by: Lei Li <lilei@xxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Zhi Yong Wu <wuzhy@xxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> [...]
> 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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list