On 11/08/2011 05:43 AM, Stefan Berger wrote:
On 11/08/2011 06:00 AM, Hu Tao wrote:
This adds per-device weights to<blkiotune>. Note that the cgroups implementation only supports weights per block device, and not per-file within the device; hence this option must be global to the domain definition rather than tied to individual <device>/<disk> entries:
<domain ...> <blkiotune> <device> <path>/path/to/block</path> <weight>1000</weight> </device> </blkiotune> ...
This patch also adds a parameter --device-weight to virsh command blkiotune for setting/getting blkiotune.weight_device for any hypervisor that supports it. All<device> entries under <blkiotune> are concatenated into a single string attribute under virDomain{Get,Set}BlkioParameters, named "device_weight".
+/** + * VIR_DOMAIN_BLKIO_DEVICE_WEIGHT: + * + * Macro for the blkio tunable weight_device: it represents the + * per-device weight, as a string. The string is parsed as a + * series of /path/to/device,weight elements, separated by ';'.
It looks like the rest of the code swapped over to "separated by ','", so I made that tweak.
+ * This function returns a string representing device weights that is + * suitable for writing to /cgroup/blkio/blkio.weight_device, given + * a list of per-device weights. + */ +#if defined(major)&& defined(minor) +int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices, + int ndevices, + char **result) +{ + int i; + struct stat s; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + for (i = 0; i< ndevices; i++) { + if (stat(devices[i].path,&s) == -1) + return -1; + if ((s.st_mode& S_IFMT) != S_IFBLK) + return -1; + virBufferAsprintf(&buf, "%d:%d %d\n", + major(s.st_rdev), + minor(s.st_rdev), + devices[i].weight); + } + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + return -1; + } + + *result = virBufferContentAndReset(&buf); + return 0;
You fail for more than one reason, but don't output a failure log message, which means the caller has to do it and can't guess why things failed (OOM? not a block device?). I don't see any callers in this patch, so I checked 5/5; there, both callers just reported an internal error stating the string could not be parsed. But internal error for a user-visible message is rather harsh, so I think this is worth improving.
+} +#else +int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices ATTRIBUTE_UNUSED, + int ndevices ATTRIBUTE_UNUSED, + char **result ATTRIBUTE_UNUSED) +{ + return -1;
And if we improve the error reporting above, then we also have to report an error here.
+static int virDomainBlkioDeviceWeightParseXML(xmlNodePtr root, + virBlkioDeviceWeightPtr dw) +{ + char *c; + xmlNodePtr node; + + if (!dw) + return -1;
Dead check - this is a static function, and we know the caller gives us valid inputs.
+ node = root->children; + while (node) { + if (node->type == XML_ELEMENT_NODE) { + if (xmlStrEqual(node->name, BAD_CAST "path")) { + dw->path = (char *)xmlNodeGetContent(node);
Memory leak; if the user (mistakenly) has more than one <path> subelement, then you are blindly overwriting dw->path on the second instance of path.
+ } else if (xmlStrEqual(node->name, BAD_CAST "weight")) { + c = (char *)xmlNodeGetContent(node); + if (virStrToLong_ui(c, NULL, 10,&dw->weight)< 0) { + VIR_FREE(c); + VIR_FREE(dw->path); + return -1;
Missing error reporting - either we must report the error here or in the caller (I elected for here, to match precedence with other functions used by the caller).
+ } + VIR_FREE(c); + } + } + node = node->next; + }
You never validated that path was a mandatory element.
@@ -6711,6 +6813,24 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, &def->blkio.weight)< 0) def->blkio.weight = 0;
+ n = virXPathNodeSet("./blkiotune/device", ctxt,&nodes); + if (n> 0) {
Missing an OOM memory check.
+ if (VIR_ALLOC_N(def->blkio.devices, n)< 0) { + virReportOOMError(); + goto error; + } + + for (i = 0; i< n; i++) { + if (virDomainBlkioDeviceWeightParseXML(nodes[i], +&def->blkio.devices[i])< 0) { + virBlkioDeviceWeightArrayClear(def->blkio.devices, i);
Redundant, if you had been updating def->blkio.ndevices as you go, for consistency with some of the other clients of virXPathNodeSet in this method.
+++ b/tools/virsh.c @@ -4648,6 +4648,8 @@ static const vshCmdOptDef opts_blkiotune[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"weight", VSH_OT_INT, VSH_OFLAG_NONE, N_("IO Weight in range [100, 1000]")}, + {"device-weight", VSH_OT_STRING, VSH_OFLAG_NONE, + N_("per-device IO Weights, in the form of /path/to/device,weight;...")},
Another place where we want to use ',', not ';'.
@@ -4749,6 +4762,10 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) vshPrint(ctl, "%-15s: %d\n", params[i].field, params[i].value.b); break; + case VIR_TYPED_PARAM_STRING: + vshPrint(ctl, "%-15s: %s\n", params[i].field, + params[i].value.s); + break; default: vshPrint(ctl, "unimplemented blkio parameter type\n"); }
Memory leak - we aren't calling virTypedParameterArrayClear to free up any returned strings that we just printed. But we can't blindly call it in the cleanup: label of the function, because...
@@ -4765,15 +4782,32 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
if (weight) { temp->value.ui = weight; - strncpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT, - sizeof(temp->field)); + if (!virStrcpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT, + sizeof(temp->field))) { + virTypedParameterArrayClear(params, nparams); + ret = false; + goto cleanup; + } weight = 0; } + + if (device_weight) { + /* Safe to cast away const here. */ + temp->value.s = (char *)device_weight;
...here we are not storing malloc'd memory there in the first place. Using a judicious strdup simplifies the code.
+ temp->type = VIR_TYPED_PARAM_STRING; + if (!virStrcpy(temp->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT, + sizeof(temp->field))) { + virTypedParameterArrayClear(params, nparams); + ret = false;
ret is already false, we can skip this assignment.
+ goto cleanup; + } + } } - if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) + ret = true; + if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) {
Pre-existing, but as long as we are touching this code, it's better to check for < 0 instead of != 0 for errors on libvirt calls.
+B<device-weights> is a single string listing one or more device/weight +pairs, in the format of /path/to/device,weight;/path/to/device,weight.
Another comma.
+ If I<--live> is specified, affect a running guest. If I<--config> is specified, affect the next boot of a persistent guest. If I<--current> is specified, affect the current guest state. ACK
I won't push this until I've tweaked and tested patch 5/5, but here's what I plan on squashing in provided testing goes well. diff --git i/include/libvirt/libvirt.h.in w/include/libvirt/libvirt.h.in index 0fd9302..ff4f51b 100644 --- i/include/libvirt/libvirt.h.in +++ w/include/libvirt/libvirt.h.in @@ -1241,7 +1241,7 @@ char * virDomainGetSchedulerType(virDomainPtr domain, * * Macro for the blkio tunable weight_device: it represents the * per-device weight, as a string. The string is parsed as a - * series of /path/to/device,weight elements, separated by ';'. + * series of /path/to/device,weight elements, separated by ','. */ #define VIR_DOMAIN_BLKIO_DEVICE_WEIGHT "device_weight" diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c index e4ae64c..8ac8d6f 100644 --- i/src/conf/domain_conf.c +++ w/src/conf/domain_conf.c @@ -604,8 +604,9 @@ VIR_ENUM_IMPL(virDomainStartupPolicy, VIR_DOMAIN_STARTUP_POLICY_LAST, #define VIR_DOMAIN_XML_READ_FLAGS VIR_DOMAIN_XML_INACTIVE -void virBlkioDeviceWeightArrayClear(virBlkioDeviceWeightPtr deviceWeights, - int ndevices) +void +virBlkioDeviceWeightArrayClear(virBlkioDeviceWeightPtr deviceWeights, + int ndevices) { int i; @@ -618,22 +619,26 @@ void virBlkioDeviceWeightArrayClear(virBlkioDeviceWeightPtr deviceWeights, * * This function returns a string representing device weights that is * suitable for writing to /cgroup/blkio/blkio.weight_device, given - * a list of per-device weights. + * a list of per-device weights, or reports an error on failure. */ #if defined(major) && defined(minor) -int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices, - int ndevices, - char **result) +int +virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices, + int ndevices, + char **result) { int i; struct stat s; virBuffer buf = VIR_BUFFER_INITIALIZER; for (i = 0; i < ndevices; i++) { - if (stat(devices[i].path, &s) == -1) - return -1; - if ((s.st_mode & S_IFMT) != S_IFBLK) + if (stat(devices[i].path, &s) == -1 || + (s.st_mode & S_IFMT) != S_IFBLK) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("path '%s' must be a block device"), + devices[i].path); return -1; + } virBufferAsprintf(&buf, "%d:%d %d\n", major(s.st_rdev), minor(s.st_rdev), @@ -641,6 +646,7 @@ int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices, } if (virBufferError(&buf)) { + virReportOOMError(); virBufferFreeAndReset(&buf); return -1; } @@ -649,10 +655,13 @@ int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices, return 0; } #else -int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices ATTRIBUTE_UNUSED, - int ndevices ATTRIBUTE_UNUSED, - char **result ATTRIBUTE_UNUSED) +int +virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices ATTRIBUTE_UNUSED, + int ndevices ATTRIBUTE_UNUSED, + char **result ATTRIBUTE_UNUSED) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("this binary lacks per-device weight support")); return -1; } #endif @@ -669,23 +678,24 @@ int virBlkioDeviceWeightToStr(virBlkioDeviceWeightPtr devices ATTRIBUTE_UNUSED, * * and fills a virBlkioDeviceWeight struct. */ -static int virDomainBlkioDeviceWeightParseXML(xmlNodePtr root, - virBlkioDeviceWeightPtr dw) +static int +virDomainBlkioDeviceWeightParseXML(xmlNodePtr root, + virBlkioDeviceWeightPtr dw) { char *c; xmlNodePtr node; - if (!dw) - return -1; - node = root->children; while (node) { if (node->type == XML_ELEMENT_NODE) { - if (xmlStrEqual(node->name, BAD_CAST "path")) { + if (xmlStrEqual(node->name, BAD_CAST "path") && !dw->path) { dw->path = (char *)xmlNodeGetContent(node); } else if (xmlStrEqual(node->name, BAD_CAST "weight")) { c = (char *)xmlNodeGetContent(node); if (virStrToLong_ui(c, NULL, 10, &dw->weight) < 0) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("could not parse weight %s"), + c); VIR_FREE(c); VIR_FREE(dw->path); return -1; @@ -695,6 +705,11 @@ static int virDomainBlkioDeviceWeightParseXML(xmlNodePtr root, } node = node->next; } + if (!dw->path) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("missing per-device path")); + return -1; + } return 0; } @@ -6813,23 +6828,21 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, &def->blkio.weight) < 0) def->blkio.weight = 0; - n = virXPathNodeSet("./blkiotune/device", ctxt, &nodes); - if (n > 0) { - if (VIR_ALLOC_N(def->blkio.devices, n) < 0) { - virReportOOMError(); - goto error; - } + if ((n = virXPathNodeSet("./blkiotune/device", ctxt, &nodes)) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot extract blkiotune nodes")); + goto error; + } + if (n && VIR_ALLOC_N(def->blkio.devices, n) < 0) + goto no_memory; - for (i = 0; i < n; i++) { - if (virDomainBlkioDeviceWeightParseXML(nodes[i], - &def->blkio.devices[i]) < 0) { - virBlkioDeviceWeightArrayClear(def->blkio.devices, i); - goto error; - } - } - def->blkio.ndevices = n; - VIR_FREE(nodes); + for (i = 0; i < n; i++) { + if (virDomainBlkioDeviceWeightParseXML(nodes[i], + &def->blkio.devices[i]) < 0) + goto error; + def->blkio.ndevices++; } + VIR_FREE(nodes); /* Extract other memory tunables */ if (virXPathULong("string(./memtune/hard_limit)", ctxt, diff --git i/tools/virsh.c w/tools/virsh.c index 4060e3d..30504be 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -4649,7 +4649,7 @@ static const vshCmdOptDef opts_blkiotune[] = { {"weight", VSH_OT_INT, VSH_OFLAG_NONE, N_("IO Weight in range [100, 1000]")}, {"device-weight", VSH_OT_STRING, VSH_OFLAG_NONE, - N_("per-device IO Weights, in the form of /path/to/device,weight;...")}, + N_("per-device IO Weights, in the form of /path/to/device,weight,...")}, {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, @@ -4770,8 +4770,6 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) vshPrint(ctl, "unimplemented blkio parameter type\n"); } } - - ret = true; } else { /* set the blkio parameters */ params = vshCalloc(ctl, nparams, sizeof(*params)); @@ -4783,34 +4781,29 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) if (weight) { temp->value.ui = weight; if (!virStrcpy(temp->field, VIR_DOMAIN_BLKIO_WEIGHT, - sizeof(temp->field))) { - virTypedParameterArrayClear(params, nparams); - ret = false; + sizeof(temp->field))) goto cleanup; - } - weight = 0; } if (device_weight) { - /* Safe to cast away const here. */ - temp->value.s = (char *)device_weight; + temp->value.s = vshStrdup(ctl, device_weight); temp->type = VIR_TYPED_PARAM_STRING; if (!virStrcpy(temp->field, VIR_DOMAIN_BLKIO_DEVICE_WEIGHT, - sizeof(temp->field))) { - virTypedParameterArrayClear(params, nparams); - ret = false; + sizeof(temp->field))) goto cleanup; - } } } - ret = true; - if (virDomainSetBlkioParameters(dom, params, nparams, flags) != 0) { + + if (virDomainSetBlkioParameters(dom, params, nparams, flags) < 0) { vshError(ctl, "%s", _("Unable to change blkio parameters")); - ret = false; + goto cleanup; } } + ret = true; + cleanup: + virTypedParameterArrayClear(params, nparams); VIR_FREE(params); virDomainFree(dom); return ret; diff --git i/tools/virsh.pod w/tools/virsh.pod index a117e84..70e299f 100644 --- i/tools/virsh.pod +++ w/tools/virsh.pod @@ -1040,7 +1040,7 @@ Display or set the blkio parameters. QEMU/KVM supports I<--weight>. I<--weight> is in range [100, 1000]. B<device-weights> is a single string listing one or more device/weight -pairs, in the format of /path/to/device,weight;/path/to/device,weight. +pairs, in the format of /path/to/device,weight,/path/to/device,weight. If I<--live> is specified, affect a running guest. If I<--config> is specified, affect the next boot of a persistent guest. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org