Re: [libvirt] [PATCH 4/8] Support block I/O throtte in XML

subject line: s/throtte/throttle/ On 11/15/2011 02:02 AM, Lei Li wrote: I'm now applying this before 3/8. The ctxt parameter is usually passed right after node. I adjusted here and all callers. Indentation. I'm not sure the mark field buys us anything. If 0 means unlimited, then either we have a limit (at least one field is non-zero) or we don't (all 6 fields are 0). Why are you passing a NULL ctxt? That's a guaranteed NULL deref the xmlStrEqual(cur->name, BAD_CAST "iotune")) line hits, during any attempt to hot-plug or hot-unplug a disk with iotuning set. Again, I'm not sure if a mark buys us anything. This hunk belongs with 3/8 (qemu implementation), not here. This looks like leftovers from a failed experiment. Here's what I'm squashing in, so far: diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c index fb86c18..a2702c5 100644 --- i/src/conf/domain_conf.c +++ w/src/conf/domain_conf.c @@ -2394,9 +2394,9 @@ cleanup: static virDomainDiskDefPtr virDomainDiskDefParseXML(virCapsPtr caps, xmlNodePtr node, + xmlXPathContextPtr ctxt, virBitmapPtr bootMap, - unsigned int flags, - xmlXPathContextPtr ctxt) + unsigned int flags) { virDomainDiskDefPtr def; xmlNodePtr cur, child; @@ -2597,45 +2597,33 @@ virDomainDiskDefParseXML(virCapsPtr caps, } } else if (xmlStrEqual(cur->name, BAD_CAST "iotune")) { if (virXPathULongLong("string(./devices/disk/iotune/total_bytes_sec)", - ctxt, &def->blkdeviotune.total_bytes_sec) < 0) { + ctxt, &def->blkdeviotune.total_bytes_sec) < 0) { def->blkdeviotune.total_bytes_sec = 0; - } else { - def->blkdeviotune.mark = 1; } if (virXPathULongLong("string(./devices/disk/iotune/read_bytes_sec)", - ctxt, &def->blkdeviotune.read_bytes_sec) < 0) { + ctxt, &def->blkdeviotune.read_bytes_sec) < 0) { def->blkdeviotune.read_bytes_sec = 0; - } else { - def->blkdeviotune.mark = 1; } if (virXPathULongLong("string(./devices/disk/iotune/write_bytes_sec)", - ctxt, &def->blkdeviotune.write_bytes_sec) < 0) { + ctxt, &def->blkdeviotune.write_bytes_sec) < 0) { def->blkdeviotune.write_bytes_sec = 0; - } else { - def->blkdeviotune.mark = 1; } - if (virXPathULongLong("string(./devices/disk/iotune/total_iops_sec)", - ctxt, &def->blkdeviotune.total_iops_sec) < 0) { + if (virXPathULongLong("string(./devices/disk/iotune/total_iops_sec)", + ctxt, &def->blkdeviotune.total_iops_sec) < 0) { def->blkdeviotune.total_iops_sec = 0; - } else { - def->blkdeviotune.mark = 1; } if (virXPathULongLong("string(./devices/disk/iotune/read_iops_sec)", - ctxt, &def->blkdeviotune.read_iops_sec) < 0) { + ctxt, &def->blkdeviotune.read_iops_sec) < 0) { def->blkdeviotune.read_iops_sec = 0; - } else { - def->blkdeviotune.mark = 1; } if (virXPathULongLong("string(./devices/disk/iotune/write_iops_sec)", - ctxt, &def->blkdeviotune.write_iops_sec) < 0) { + ctxt, &def->blkdeviotune.write_iops_sec) < 0) { def->blkdeviotune.write_iops_sec = 0; - } else { - def->blkdeviotune.mark = 1; } if ((def->blkdeviotune.total_bytes_sec && def->blkdeviotune.read_bytes_sec) @@ -6135,8 +6123,8 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(virCapsPtr caps, if (xmlStrEqual(node->name, BAD_CAST "disk")) { dev->type = VIR_DOMAIN_DEVICE_DISK; - if (!(dev->data.disk = virDomainDiskDefParseXML(caps, node, - NULL, flags, NULL))) + if (!(dev->data.disk = virDomainDiskDefParseXML(caps, node, ctxt, + NULL, flags))) goto error; } else if (xmlStrEqual(node->name, BAD_CAST "lease")) { dev->type = VIR_DOMAIN_DEVICE_LEASE; @@ -7205,9 +7193,9 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, for (i = 0 ; i < n ; i++) { virDomainDiskDefPtr disk = virDomainDiskDefParseXML(caps, nodes[i], + ctxt, bootMap, - flags, - ctxt); + flags); if (!disk) goto error; @@ -9648,7 +9636,12 @@ virDomainDiskDefFormat(virBufferPtr buf, def->dst, bus); /*disk I/O throttling*/ - if (def->blkdeviotune.mark) { + if (def->blkdeviotune.total_bytes_sec || + def->blkdeviotune.read_bytes_sec || + def->blkdeviotune.write_bytes_sec || + def->blkdeviotune.total_iops_sec || + def->blkdeviotune.read_iops_sec || + def->blkdeviotune.write_iops_sec) { virBufferAddLit(buf, " <iotune>\n"); if (def->blkdeviotune.total_bytes_sec) { virBufferAsprintf(buf, " <total_bytes_sec>%llu</total_bytes_sec>\n", diff --git i/src/conf/domain_conf.h w/src/conf/domain_conf.h index 04cd7d8..9180769 100644 --- i/src/conf/domain_conf.h +++ w/src/conf/domain_conf.h @@ -344,7 +344,6 @@ struct _virDomainDiskDef { unsigned long long total_iops_sec; unsigned long long read_iops_sec; unsigned long long write_iops_sec; - unsigned int mark; } blkdeviotune; char *serial; diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index 449b685..85b34bb 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -1866,39 +1866,6 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, } } - /*block I/O throttling*/ - if (disk->blkdeviotune.mark) { - if (disk->blkdeviotune.total_bytes_sec) { - virBufferAsprintf(&opt, ",bps=%llu", - disk->blkdeviotune.total_bytes_sec); - } - - if (disk->blkdeviotune.read_bytes_sec) { - virBufferAsprintf(&opt, ",bps_rd=%llu", - disk->blkdeviotune.read_bytes_sec); - } - - if (disk->blkdeviotune.write_bytes_sec) { - virBufferAsprintf(&opt, ",bps_wr=%llu", - disk->blkdeviotune.write_bytes_sec); - } - - if (disk->blkdeviotune.total_iops_sec) { - virBufferAsprintf(&opt, ",iops=%llu", - disk->blkdeviotune.total_iops_sec); - } - - if (disk->blkdeviotune.read_iops_sec) { - virBufferAsprintf(&opt, ",iops_rd=%llu", - disk->blkdeviotune.read_iops_sec); - } - - if (disk->blkdeviotune.write_iops_sec) { - virBufferAsprintf(&opt, ",iops_wr=%llu", - disk->blkdeviotune.write_iops_sec); - } - } - if (virBufferError(&opt)) { virReportOOMError(); goto error; diff --git i/src/util/xml.h w/src/util/xml.h index 5742f19..c492063 100644 --- i/src/util/xml.h +++ w/src/util/xml.h @@ -50,8 +50,6 @@ xmlNodePtr virXPathNode(const char *xpath, int virXPathNodeSet(const char *xpath, xmlXPathContextPtr ctxt, xmlNodePtr **list); -int virXMLStringToULongLong (const char *stringval, - unsigned long long *value); char * virXMLPropString(xmlNodePtr node, const char *name); -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org